[tommyzli]

Summary

follow up of #3155, expanding the coverage from just SQL queries to promql ones too

Screenshots or video

Before	After

How to test on Vercel preview

Preview routes:

Steps:

References

• Linear Issue: HDX-5490
• Related PRs:

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Oct 6, 2026 8:32pm UTC
hyperdx-storybook	Ready	Preview	Oct 6, 2026 8:32pm UTC

[changeset-bot]

🦋 Changeset detected

Latest commit: fbdfc73

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/common-utils	Minor
@hyperdx/api	Minor
@hyperdx/app	Minor
@hyperdx/otel-collector	Minor

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

[github-actions]

Duplicate code analysis

No new duplicate code introduced by this PR.

---

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 128 in total (1.79%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Adds query attribution tracking to PromQL requests.

The PR appears safe to merge; no new actionable failure was established.

Summary

The PR attributes browser-originated PromQL queries to their dashboard, tile, or search across ClickHouse query paths. Since the previous review, it also adds a persistent browser preference for dismissing the missing-correlated-log-source warning.

• Passes attribution from chart and metadata requests through the Prometheus API router to ClickHouse.
• Adds attribution coverage and metric drill-down warning tests.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Browser[Browser chart or metadata query] -->|Attribution header| API[Prometheus API router]
  API -->|Scoped attribution| Fallback[ClickHouse table function]
  API -->|Log comment and query ID| Proxy[ClickHouse Prometheus HTTP API]

Loading

_{Reviews (7) · Last reviewed commit: "Merge branch 'main' into promql-query-at..."}

[github-actions]

E2E Test Results

✅ All tests passed • 398 passed • 1 skipped • 1164s

Status	Count
✅ Passed	398
❌ Failed	0
⚠️ Flaky	1
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

• Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)
• Touches API routes or data models — hidden complexity risk

Additional context: touches the query rendering engine lightly (93 lines, under the 150-line bar for Tier 4)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats

• Production files changed: 9
• Production lines changed: 222 (+ 298 in test files, excluded from tier calculation)
• Branch: promql-query-attribution
• Author: tommyzli

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

PR Review

✅ No issues found.

[github-actions]

Deep Review

Scope: PR #3300 "Promql query attribution" — extends query-attribution tracking from SQL to PromQL. The browser sends an x-hyperdx-query-attribution header; a new Express middleware parses it into an AsyncLocalStorage scope, and the router tags the ClickHouse Prometheus HTTP API proxy (log_comment URL param + X-ClickHouse-Query-Id header) and the <26.6 table-function fallback. Diff reviewed against base 76e5cf78.

✅ No critical issues found.

The attribution path is defensively built and well-covered: browser values are an allowlist-sanitized hint re-encoded by buildLogComment before reaching ClickHouse, log_comment/query_id are not in CALLER_SETTABLE_PARAM_KEYS so a caller cannot override server-pinned values or the execution limits, server-owned trace/label are stripped from the browser payload before the merge, and the incoming header is size-capped before JSON.parse. Integration tests cover both the proxy and table-function paths (via a mocked getServerVersion) and assert the server's label wins over the browser's. The prior-review concern (@pulpdrew: untagged Prometheus label/metadata lookups) is fully addressed — usePromqlMetricNames, usePromqlLabelNames, and usePromqlLabelFilterValues now thread useMetadataQueryAttribution() through to prometheusApi.labels/labelValues, and no untagged call sites remain.

🟡 P2 — recommended

• packages/common-utils/src/clickhouse/attribution.ts:245 — parseLogComment has no dedicated test asserting that the browser-supplied trace/label are dropped at the middleware boundary; the end-to-end int test covers the proxy path but the unit boundary that enforces "server owns trace/label" is only indirectly exercised.
  • Fix: Add a unit test asserting the prometheus.ts middleware strips trace and label from a parsed header before opening the withQueryAttribution scope.

🔵 P3 nitpicks (2)

• packages/common-utils/src/clickhouse/attribution.ts:246 — the incoming size guard uses value.length (UTF-16 code units) against MAX_LOG_COMMENT_BYTES, so a multibyte header can exceed the intended byte budget while passing the check; impact is negligible because the outbound buildLogComment re-enforces an ASCII-only byte cap.
  • Fix: Use Buffer.byteLength(value) (or document that the pre-parse cap is an approximate code-unit bound) for consistency with the byte-accurate cap in buildLogComment.
• packages/common-utils/src/clickhouse/attribution.ts:160 — the refactored mergeQueryAttribution now rebuilds a full object literal each layer, so the returned object always contains every key (with undefined values) rather than only set keys; behavior is unchanged for all current consumers but the all-keys-present shape is a subtle difference from the prior conditional-assignment form.
  • Fix: No action needed; optionally note the shape change in a comment so a future Object.keys/toStrictEqual consumer is not surprised.

---

Reviewers (2): previous-comments, orchestrator diff analysis (correctness/security/reliability/api-contract/testing lenses)

Note on coverage: Of the dispatched reviewer fan-out, the previous-comments reviewer returned (clean — prior feedback verified as addressed); the remaining lens agents had not reported before output was finalized. The findings above reflect the orchestrator's own verified read of the full diff and supporting modules (attribution.ts, node.ts, prometheus.ts, queryAttribution.tsx, api.ts) and all queryPromqlChartConfig call sites, not those agents' independent analysis.

Testing gaps: No unit-level assertion that parseLogComment's output has trace/label dropped at the middleware boundary; the UTF-16-vs-byte size-cap boundary is untested (low value).

_{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}

[pulpdrew]

LGTM besides the one question

[pulpdrew]

LGTM