Repository navigation
feat: Run PromQL tiles from MCP query_tile - #3528
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 0a18b0c 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 |
3b41987 to
13c6080
Compare
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Duplicate code analysis3 new clones introduced by this PR.
If one of these duplicates a helper that already exists, import it instead jscpd compared this branch against |
| seriesLabels.set(series_name, metric); | ||
| for (const [ts, value] of values) { | ||
| data.push( | ||
| isBucketed |
There was a problem hiding this comment.
🔵 minor — Table and heatmap PromQL tiles return line-chart rows, not what the dashboard shows
In the app, table tiles go through toTableRows: one column per label plus Timestamp/Value, capped at MAX_TABLE_ROWS, with limit sent on instant queries. Histogram heatmaps go through toHistogramRows, which groups by le and rejects series without le (both in packages/app/src/utils/promqlChartQuery.ts). Here both kinds fall into the generic series_name/value shape. No row/series limit is applied and heatmap le is never checked. Branch on displayType === Table and heatmap?.mode === 'histogram' like queryPromqlChartConfig does, ideally through the shared helper suggested above, or document the difference in the tool description.
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
There was a problem hiding this comment.
Fixed in the stacked follow-up #3529: MCP now uses the app's shared table and histogram row builders, so table tiles return a column per label (capped at MAX_TABLE_ROWS, with limit on instant queries) and histogram heatmaps return rows keyed by le.
PR Review
6 finding(s): 🔴 0 critical · 🟠 2 major · 🔵 4 minor 6 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. The diff was reviewed by the reviewers listed below plus orchestrator-level analysis of the full diff. I independently traced the credential-redaction paths ( 🟡 P2 — recommended
🔵 P3 nitpicks (5)
Reviewers (4): testing, maintainability, previous-comments, learnings-researcher (plus orchestrator analysis of correctness, security, and the router refactor). Testing gaps:
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 |
E2E Test Results✅ All tests passed • 463 passed • 1 skipped • 1170s
Tests ran across 4 shards in parallel. |
c641b76 to
34d2d98
Compare
34d2d98 to
410c23d
Compare
410c23d to
3a8803f
Compare
3a8803f to
4561886
Compare
4561886 to
21ebb04
Compare
21ebb04 to
0a18b0c
Compare
| result: SampledSeries[]; | ||
| }; | ||
|
|
||
| function toSamples( |
There was a problem hiding this comment.
🟠 major — PromQL result normalization and row shaping copied from the app instead of shared
toSamples copies normalizeInstantResult and toSampledSeries, and toPromqlTileResult copies toChartRows and reduceBucketRows, all from packages/app/src/utils/promqlChartQuery.ts. SampledSeries/PromqlExpressionSamples restate the app's SampledSeries/ExpressionResult types, and the string-result error message is copied word for word. The REQUIRED DRY rule says not to add a second definition. The PR promises 'values match the tile', so any later change to the app's naming, reducing or bucketing will quietly make MCP disagree with the dashboard. Move these helpers into common-utils/src/core/promql.ts and import them in both the app and the API.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| const reduces = displayTypeSupportsReducer(config); | ||
| const reducer = getQueriedPromqlSeries(config)[0]?.reducer; | ||
|
|
||
| const isBucketed = !reduces && expressions.some(e => e.isBucketed); |
There was a problem hiding this comment.
🔵 minor — Table and histogram tiles get chart rows the dashboard never shows, and instant table queries have no row cap
For DisplayType.Table the app uses toTableRows: one column per label, plus Timestamp/Value, capped at MAX_TABLE_ROWS. Its instant query also sends limit: MAX_TABLE_ROWS to Prometheus. Heatmaps in histogram mode use toHistogramRows (rows keyed by le). See packages/app/src/utils/promqlChartQuery.ts:98,350-356. MCP sends both through series_name/value rows and puts no limit on the instant request, so a large vector from a Prometheus endpoint is loaded whole before trimToolResponse cuts it. Branch on the display type the same way the app does (best done with the shared helpers), and pass a series limit for table instant 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
0a18b0c to
9850e84
Compare
teeohhem
left a comment
There was a problem hiding this comment.
Looks good. Tested locally
clickstack_query_tile and clickstack_query_tiles now return data for PromQL tiles instead of an error. Each expression runs against the source's backend: a Prometheus endpoint, ClickHouse's Prometheus HTTP API, or the ClickHouse table function on servers older than 26.6. Granularity, alignment, instant versus range queries and reducers follow the dashboard, so the agent sees the numbers the tile shows. Rows match the app's chart rows, with each series' labels listed once in seriesLabels. Number, pie and bar tiles return one reduced value per series. Dashboard variables apply. The backend choice moves from the Prometheus router into controllers/promql.ts so MCP can share it. The router's streaming proxy and the buffered fetch MCP uses now build their requests with one helper, buildPrometheusRequest. The router behaves as before.
9850e84 to
e03116a
Compare
Summary
clickstack_query_tileandclickstack_query_tilesreturn data for PromQL tiles instead of "PromQL tiles cannot be queried through MCP yet", so an agent can check the PromQL tiles it builds.autogranularity (floored at the source'sminAutoGranularity), bucket alignment, and instant vs range queries, so values match the tile.{ __hdx_time_bucket, series_name, value }, with series named by the legend template.seriesLabelslists each series' labels once.variableValues.promql.queryspan withhyperdx.promql.backendandhyperdx.promql.query_kind.routers/api/prometheus.tstocontrollers/promql.tsso MCP and the router share them. The router behaves as before; its tests pass unchanged apart from an import path.buildPrometheusRequestbuilds every Prometheus request (URL, params, Basic auth, redacted target) for both the router's streaming proxy and MCP's buffered fetch. The one visible change: a host with malformed percent-encoding in its userinfo gets the proxy's "Invalid Connection host" 400.How to test
The expressions below use
count by (job) ({__name__=~".+"}), which counts series perjoband returns data for any metrics in the table.Tile results
clickstack_save_dashboard:{ "name": "PromQL", "tiles": [ { "name": "Series", "config": { "configType": "promql", "displayType": "line", "sourceId": "<id>", "expressions": [ { "expression": "count by (job) ({__name__=~\".+\"})" }, { "expression": "count({__name__=~\".+\"})", "alias": "Total" } ], "legendTemplate": "{{job}}" } } ] }clickstack_query_tilewith the dashboard and tile IDs andstartTimeone hour ago. It returns rows{ "__hdx_time_bucket", "series_name", "value" }from both expressions (one series per job, plusTotal), bucketed at 1 minute, and aseriesLabelsentry per series.clickstack_query_tilewithstartTimeabout an hour ago, at 30 seconds past a minute (e.g....T10:00:30Z). The first bucket is at10:00:00. Turn alignment off withclickstack_patch_dashboard(the config is replaced, not merged, so send all of it) and query again: the first sample is at10:00:30.clickstack_query_tileson the dashboard. The tile reportsstatus: "ok"andhasData: true.Filters
joblabel filter and use it in the tile withclickstack_save_dashboard(it replaces the tiles and filters, so the tile keeps itsid):{ "id": "<dashboard id>", "name": "PromQL", "tiles": [ { "id": "<tile id>", "name": "Series", "config": { "configType": "promql", "displayType": "line", "sourceId": "<id>", "expressions": [ { "expression": "count by (job) ({__name__=~\".+\", job=~\"$job\"})" } ], "legendTemplate": "{{job}}" } } ], "filters": [ { "type": "PROMETHEUS_LABEL", "name": "Job", "sourceId": "<id>", "label": "job", "variableName": "job" } ] }clickstack_query_tilewith this added to the dashboard and tile IDs. Only that job's series comes back.{ "variableValues": [{ "name": "job", "values": ["<one job from step 2>"] }] }References