Repository navigation
feat: Add the clickstack_promql MCP tool and PromQL prompt docs - #3531
karl-power merged 3 commits into
Conversation
🦋 Changeset detectedLatest commit: a694da0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // table tile, which lays each series out with a column per label. | ||
| const tile = | ||
| input.queryType === 'instant' | ||
| ? buildTile('MCP PromQL', 24, 6, { |
There was a problem hiding this comment.
🔵 minor — Instant queries drop granularity, so $__interval and $__rate_interval cannot be set and can differ from a saved tile
The instant branch builds a table tile without granularity, so $__interval and $__rate_interval in an instant expression (e.g. avg_over_time(x[$__interval])) always use the auto step. A saved PromQL table tile does accept granularity (externalPromqlChartConfigBaseSchema, packages/api/src/utils/zod.ts:690), so the same expression saved 'as is' with a granularity returns different values. This contradicts the tool description and the prompt's note that granularity sets $__interval. Fix: pass granularity: input.granularity in both branches, and change the description to say it only sets the step for range queries.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| @@ -36,6 +37,17 @@ const SERVER_INSTRUCTIONS = [ | |||
| 'dashboard authoring.', | |||
| ].join('\n'); | |||
|
|
|||
| const PROMQL_INSTRUCTIONS = | |||
There was a problem hiding this comment.
🔵 minor — Server instructions ban clickstack_sql for PromQL sources, but the source hints recommend it
PROMQL_INSTRUCTIONS tells the agent to query PromQL sources with clickstack_promql, "not the builder tools or clickstack_sql". But PROMQL_SOURCE_USAGE (mcp/utils/promql.ts) and the query_guide section both point to clickstack_sql with timeSeriesTags/timeSeriesData as the way to inspect raw tags and samples of a TimeSeries table. Fix: reword the instruction to "prefer clickstack_promql; clickstack_sql only for raw TimeSeries inspection" so the agent gets one consistent rule.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 1 posted as inline comment(s) on the changed lines. 2 listed below. Findings outside the changed lines2 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
12ddc8e to
cfb5dcf
Compare
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
| // ─── Schema ────────────────────────────────────────────────────────────────── | ||
|
|
||
| const promqlSchema = z.object({ | ||
| sourceId: z |
There was a problem hiding this comment.
🔵 minor — A sourceId that isn't an ObjectId throws a ZodError from buildTile instead of returning a user error
The tool accepts z.string() here, but buildTile parses the config with externalDashboardTileSchemaWithId, whose PromQL base schema uses objectIdSchema for sourceId (packages/api/src/utils/zod.ts:688). A value like "metrics" therefore throws from .parse(). That turns into a raw Zod error, and withToolTracing counts it as a server error, so the agent never sees the friendly "Source not found … call clickstack_list_sources" message. Fix: use objectIdSchema.describe(...) for sourceId so bad IDs are rejected when the input is validated.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| 'startTime and endTime and returns time series rows. "instant" evaluates ' + | ||
| 'it once at endTime and returns one row per series with a column per label.', | ||
| ), | ||
| granularity: z |
There was a problem hiding this comment.
🔵 minor — Restates the PromQL tile's granularity and expression schemas instead of deriving them
z.union([SQLIntervalSchema, z.literal('auto')]) and z.string().max(10000) copy externalPromqlChartConfigBaseSchema.granularity and externalPromqlRangeExpressionSchema.expression in packages/api/src/utils/zod.ts:690 and :659. The PR says the tool and the tile accept identical values, but these copies can drift apart. Fix: export those two schemas (or their .shape fields) from zod.ts and reuse them here, as the DRY rule in the repository conventions requires.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
E2E Test Results✅ All tests passed • 465 passed • 1 skipped • 1211s
Tests ran across 4 shards in parallel. |
| 'find it, and clickstack_describe_source for its metric and label names.', | ||
| ), | ||
| expression: z | ||
| .string() |
There was a problem hiding this comment.
🔵 minor — An empty or whitespace expression returns 'No data found' instead of an error
Use z.string().trim().min(1).max(10000) for expression. Right now getQueriedPromqlSeries (common-utils/src/core/promql.ts:43) filters out blank expressions, so runPromqlTile runs zero queries, still probes the backend, and formatQueryResult then tells the agent to widen the time range. The real problem is that the expression was empty.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| 'it once at endTime and returns one row per series with a column per label.', | ||
| ), | ||
| granularity: z | ||
| .union([SQLIntervalSchema, z.literal('auto')]) |
There was a problem hiding this comment.
🔵 minor — granularity and expression restate the PromQL tile schema rules instead of reusing them
Export the field schemas from externalPromqlChartConfigBaseSchema / externalPromqlRangeExpressionSchema in packages/api/src/utils/zod.ts (lines 659 and 690) and reuse them here. Today the union([SQLIntervalSchema, literal('auto')]) and the string().max(10000) rules are written twice. The PR says values copy unchanged between this tool and saved tiles, so the two copies have to stay identical, and the repo's DRY rule (REQUIRED) asks for one source of truth.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const requested = requestedUrls().find(url => | ||
| url.pathname.endsWith('/query_range'), | ||
| ); | ||
| expect(Number(requested?.searchParams.get('step'))).toBeGreaterThan(0); |
There was a problem hiding this comment.
🔵 minor — The auto-step test only checks step > 0, so it can't catch a broken auto granularity
Assert the exact step the auto rule should give for the 60s range and the source's minAutoGranularity (or compare it with buildPromqlTileQueries for the same line-tile config). As written, the test still passes if auto falls back to a fixed default or ignores the floor.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Deep ReviewScope: New MCP tool ✅ No critical issues found. Correctness, security, and api-contract reviewers each traced the full 🔵 P3 nitpicks (7)
Reviewers (9): correctness, security, api-contract, testing, maintainability, project-standards, kieran-typescript, previous-comments, learnings-researcher. Coverage notes:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
164f4a7 to
5c65189
Compare
| const result = await promql({ | ||
| sourceId: source._id.toString(), | ||
| granularity: '1 second', | ||
| startTime: new Date((START - 86_400) * 1000).toISOString(), |
There was a problem hiding this comment.
🔵 minor — Auto-step test only asserts step > 0, so a wrong auto step still passes
The PR describes the auto step as about 80 points, floored at the source's minAutoGranularity. This test only checks that some step was sent, so any regression in that logic, such as ignoring the floor or using the wrong bucket count, would pass. Assert the exact step expected for the fixed 60s range. Then add a case with minAutoGranularity set on the source and assert the step is floored to it.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
5c65189 to
c3e1cd4
Compare
c3e1cd4 to
8aa7eb1
Compare
8aa7eb1 to
4fb784a
Compare
4fb784a to
a694da0
Compare
| const result = await promql({ | ||
| sourceId: source._id.toString(), | ||
| granularity: '1 second', | ||
| startTime: new Date((START - 86_400) * 1000).toISOString(), |
There was a problem hiding this comment.
🔵 minor — Auto-step test only checks step > 0, so a wrong auto step still passes
Compute the expected step for the 60s range, as getTimeChartGranularity/promqlStepSeconds would with the source's minAutoGranularity, and assert that exact value. Also add a case with minAutoGranularity set to cover the floor the PR mentions. As written, a hardcoded default step, a missing floor or options.granularity being ignored would all still pass.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
a694da0 to
0b9a851
Compare
| const result = await promql({ | ||
| sourceId: source._id.toString(), | ||
| granularity: '1 second', | ||
| startTime: new Date((START - 86_400) * 1000).toISOString(), |
There was a problem hiding this comment.
🔵 minor — Auto-step test only asserts step > 0, so a broken auto step or a missing minAutoGranularity floor still passes
Give the source a minAutoGranularity (or use a range with a known ~80-point step) and assert the exact step value sent to /query_range.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
0af7f93 to
d15bec9
Compare
An agent could describe PromQL sources and save PromQL tiles, but had no way to run an expression without building a dashboard first. clickstack_promql runs one expression against a PromQL source over a time range, as a range query (time series rows plus seriesLabels) or an instant query (one row per series, a column per label). The tool wraps the expression in a one-tile config and runs it through runConfigTile, the path clickstack_query_tile uses, so results, macros, the auto step and errors match a saved tile with the same expression and granularity. The query_guide and create_dashboard prompts gain PromQL sections, the server instructions point PromQL sources at the tool, and the source hints lead with it. All of it is hidden while PromQL is disabled, so nothing new is visible there.
buildTile parses the tile with the external schema, which requires an ObjectId sourceId, so a source name threw a raw ZodError that was logged as a server error. Checking the shape in the tool schema returns a schema error to the agent instead.
…with tiles Pass granularity on instant queries too, so $__interval matches a saved table tile with the same expression. Use PromqlQueryTypeSchema for queryType instead of restating it. Reword the server instructions so clickstack_sql is positioned as the raw TimeSeries inspection route, as the source hints already say, rather than banned.
d15bec9 to
19fb692
Compare
Summary
Adds
clickstack_promql, so an agent can ask a PromQL source a question directly instead of saving a dashboard tile and querying that. Also teaches the agent about PromQL through the prompts and instructions. With PromQL disabled, none of it is visible.clickstack_promql(sourceId, expression, queryType?, granularity?, startTime?, endTime?). Arangequery (the default) returns{ __hdx_time_bucket, series_name, value }rows plusseriesLabels, likeclickstack_timeseries. Aninstantquery evaluates atendTimeand returns one row per series with a column per label plusValue, capped at 10,000 series.runConfigTile, the same path asclickstack_query_tileandclickstack_sql. So the$__interval,$__rate_intervaland$__rangemacros, theautostep (about 80 points, floored at the source'sminAutoGranularity), the 11,000-point cap, error mapping and the 50 KB result trim are all shared, and an expression that works in the tool can be saved as a tile as is.granularitytakes the"<number> <unit>"format PromQL tiles andclickstack_timeseriesuse, so values copy between the tool andclickstack_save_dashboardunchanged.query_guideprompt gains aPROMQL SOURCESsection (discovery flow,rate()/increase(),histogram_quantile, label matchers, the macros, common mistakes) andcreate_dashboardgains aPROMQL TILESsection (tile shape,legendTemplate,PROMETHEUS_LABELfilters). Both live in a newpromqlContent.tssocontent.tsdoes not grow.clickstack_promql, and thepromqlUsagehint inclickstack_list_sourcesandclickstack_describe_sourceleads with the tool, with theclickstack_sql/timeSeriesTagsroute kept as the raw-inspection fallback.MCP.mdlists the tool and gets a "PromQL Sources" section.How to test
clickstack_list_sources. The PromQL source is listed andpromqlUsagestarts with "Query them with clickstack_promql".clickstack_promqlwithstartTimeone hour ago:{ "sourceId": "<id>", "expression": "count by (job) ({__name__=~\".+\"})" }{ "__hdx_time_bucket", "series_name", "value" }rows with aseriesLabelsentry per job, at about one point a minute."granularity": "5 minute". Points are five minutes apart.{ "sourceId": "<id>", "expression": "count by (job) ({__name__=~\".+\"})", "queryType": "instant" }jobcolumn and aValuecolumn.rate(. The error is the backend's parse error."granularity": "1 second"withstartTimea day ago. The error says the Prometheus resolution cap of 11,000 points was exceeded.linePromQL tile withclickstack_save_dashboardand callclickstack_query_tileon it with the same time range. The rows match step 2.query_guideprompt: it has a== PROMQL SOURCES ==section. Fetchcreate_dashboard: it has== PROMQL TILES ==. The server instructions (the MCP inspector shows them on connect) end with the PromQL line.ENABLE_PROMQL=false).clickstack_promqlis gone from the tool list, and the prompts and instructions no longer mention it.References