Skip to content

feat: Exemplars for metric & PromQL charts#2536

Open
jordan-simonovski wants to merge 15 commits into
mainfrom
feat/exemplars
Open

feat: Exemplars for metric & PromQL charts#2536
jordan-simonovski wants to merge 15 commits into
mainfrom
feat/exemplars

Conversation

@jordan-simonovski

@jordan-simonovski jordan-simonovski commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Why

Engineers staring at a latency spike on a chart have no way to jump to a trace that caused it.
This change adds exemplars, clickable markers overlaid on time charts, each linking to a representative trace.
Works for metric and PromQL sources.

Also added some test-telemetry infrastructure to build and validate the feature against more complex data sets.

CleanShot.2026-06-29.at.13.50.52.mp4

Exemplar overlay (app)

  • Diamond markers on HDXMultiSeriesTimeChart, plotted at the trace's own value, thinned to a configurable target across the visible range (the slowest/most-notable trace per window).
  • Hover card showing trace metadata (service, span, duration, status) fetched from a configurable exemplar trace source, with an Inspect trace button that deep-links straight to that trace. The card flips/clamps to stay on-screen near chart edges.
  • Opt-in "Exemplars" toggle in the chart editor (next to "As Ratio" for builder/metric charts; in the PromQL editor for PromQL charts) — persisted on the chart config, not a runtime toggle.

Two data backends

  • Metric sources (ClickHouse): renderMetricExemplarsChartConfig reads the OTel metric tables' Exemplars.* columns, honoring the chart's time range, metric name, and filters.
  • PromQL sources (native Prometheus): new /v1/prometheus/query_exemplars proxy + Prometheus responses normalized to a shared Exemplar shape (defensive about trace_id/traceID label spellings).
  • Fetched in parallel via a new useExemplars hook, gated so it's a no-op unless the toggle is enabled.

Fully-OTLP coherent metrics (collector)

  • Added the spanmetrics connector to the collector build and wired it into the OpAMP-generated collector config (ENABLE_SPAN_METRICS, off by default). It derives traces.span.metrics.* (calls + duration histogram) with exemplars from spans, so the duration histogram lands in ClickHouse with Exemplars.* populated — no synthetic/seeded data.
  • Optionally remote-writes those metrics (with exemplars) to a Prometheus endpoint (SPAN_METRICS_PROM_RW_ENDPOINT), so the native PromQL exemplar path is testable against the same real data.

Telemetry generator (telemetry-generator/)

  • A Node service emitting realistic OTLP traces (6 services, weighted attribute pools, nested spans and several failure scenarios) with backfill + live emission. Replaces ad-hoc ClickHouse seeding; wired into docker-compose.dev.yml. The spanmetrics connector turns its traces into coherent metric exemplars.

Team setting

  • maxExemplars (Team Settings → Chart Settings; 0 = unlimited) controls overlay density.

Scoping

  • Exemplars are restricted to single, non-ratio, histogram (latency) metric series — the exemplar value shares the chart's y-axis unit. Toggle hidden and renderer returns null otherwise.

Out of scope (separate tickets)

  • Heatmap exemplars — needs trace-source exemplar generation + a uPlot overlay.
  • Ratio + Group By — the ratio engine isn't group-aware; tracked as a separate bugfix PR.

Testing

  • common-utils: SQL renderer tests (metric exemplar query shape; null for ratio/multi-series/non-histogram/non-metric).
  • app: normalizePrometheusExemplars label-variant tests; DBTimeChart updated to mock the new exemplars data path.
  • api: query_exemplars route integration tests (native proxy + ClickHouse-backed empty result).
  • Verified end-to-end in an isolated stack: the spanmetrics connector emits a traces.span.metrics.duration histogram with real exemplars (trace id + actual latency), 100% resolving back to seeded traces.
  • make ci-lint / per-package tsc + unit suites green.

Changesets

  • exemplar-mode-metrics.md — @hyperdx/common-utils, @hyperdx/api, @hyperdx/app (minor)
  • span-metrics-connector.md — @hyperdx/api, @hyperdx/otel-collector (minor)

Notes / caveats

  • ENABLE_SPAN_METRICS is off by default — no production behavior change; it's enabled in local dev.
  • HyperDX collectors enforce ingest auth with scheme: '' (raw token, no Bearer prefix) — set <INGESTION_API_KEY> accordingly.

Storybook: exemplar components

Extracted the two exemplar UI pieces — the ExemplarDot chart marker and the ExemplarHoverCard trace popover — out of HDXMultiSeriesTimeChart and DBTimeChart into a focused components/Exemplars/ directory, with a Storybook story for each. The card story covers every state (full/partial metadata, loading, trace-not-found, no-trace-source-configured) and the dot story renders the marker in isolation, so both can be reviewed across light/dark and both brand themes without a live ClickHouse query. Behaviour-preserving; ExemplarDot also gains a real props type in place of any.

Merged main to bring the branch current — this includes the recharts 2→3 upgrade, for which the exemplar ReferenceDot marker was adjusted to the v3 API.

Review hardening & feature gate (latest)

Follow-up addressing review feedback (Greptile P1 + Deep Review P2s) and gating the feature for a safe rollout:

  • Feature-flagged. The whole overlay — editor toggle, PromQL toggle, data fetch, and team setting — is now behind NEXT_PUBLIC_ENABLE_EXEMPLARS: off by default, enabled in local dev. It can ship dark while we finish testing against real data.
  • Filter scoping (Greptile P1). The metric-name predicate is now ANDed separately from the user filter group, so a chart using filtersLogicalOperator: 'OR' can no longer let the exemplar scan match other metrics.
  • Single-series enforced end-to-end. PromQL queries returning more than one series drop the overlay (a marker's value can't be attributed or scaled across series); the PromQL result is capped client-side (no native limit param); and enableExemplars is cleared when a chart leaves single-series so a stale flag can't linger.
  • Outliers no longer distort the axis. The y-axis follows the visible series and each marker clamps to the series max (the hover card still shows the true duration), so one slow trace can't flatten the p99 line. The series hover tooltip is suppressed while hovering a marker so the two cards don't overlap, and the marker has a higher-contrast outline in light mode.
  • Tests. Added SQL OR-filter and PromQL multi-series regression tests; the exemplar bucketing is extracted to a unit-tested pure helper.

Note: the exemplar marker value was verified correct — it matches the span's real duration to the nanosecond; the earlier "off by an order of magnitude" appearance was a single slow outlier stretching the y-axis, addressed above.

@changeset-bot

changeset-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4d3ef3a

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

@vercel

vercel Bot commented Jun 29, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jul 25, 2026 6:36am
hyperdx-storybook Ready Ready Preview, Comment Jul 25, 2026 6:36am

Request Review

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Jun 29, 2026
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Critical-path files (3):
    • packages/api/src/config.ts
    • packages/api/src/models/team.ts
    • packages/otel-collector/builder-config.yaml
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 34
  • Production lines changed: 2246 (+ 865 in test files, excluded from tier calculation)
  • Branch: feat/exemplars
  • Author: jordan-simonovski

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

@jordan-simonovski
jordan-simonovski marked this pull request as draft June 29, 2026 05:57
@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds exemplar markers for metric and PromQL time charts. The main changes are:

  • Exemplar fetching and chart overlay UI behind a feature flag.
  • ClickHouse metric exemplar SQL for eligible single-series histogram charts.
  • Prometheus query_exemplars proxy support.
  • Optional spanmetrics collector wiring for coherent trace-linked metric exemplars.
  • Team and chart settings for exemplar display limits and trace lookup.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The metric exemplar query now keeps metric-name scoping separate from user filter grouping.
  • The spanmetrics remote-write config now uses a resolved endpoint value before emitting the exporter.

Important Files Changed

Filename Overview
packages/common-utils/src/core/renderChartConfig.ts Adds exemplar eligibility checks and scoped metric exemplar SQL generation.
packages/api/src/config.ts Adds spanmetrics and Prometheus remote-write configuration flags.
packages/api/src/opamp/controllers/opampController.ts Adds optional spanmetrics connector and remote-write exporter generation.
packages/app/src/hooks/useExemplars.tsx Adds gated exemplar fetching for metric and PromQL chart sources.
packages/app/src/components/ChartEditor/PromqlChartEditor.tsx Adds the PromQL exemplar toggle and trace source selection.

Reviews (11): Last reviewed commit: "fix(exemplars): address review P2s on el..." | Re-trigger Greptile

Comment thread packages/api/src/opamp/controllers/opampController.ts Outdated
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 242 passed • 1 skipped • 996s

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

Tests ran across 4 shards in parallel.

View full report →

@jordan-simonovski
jordan-simonovski marked this pull request as ready for review June 29, 2026 20:58
Comment thread packages/api/src/config.ts Outdated
…ated collector config, so the collector container no longer needs SPAN_METRICS_PROM_RW_ENDPOINT in its own environment.
Comment thread packages/common-utils/src/core/renderChartConfig.ts
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Thirteen reviewer agents ran against the exemplars feature surface. Note on method: Bash, Grep/Glob, and network fetch were all unavailable in this environment (bwrap sandbox failure), so no git diff could be computed — scope was reconstructed from the working tree and reviewers worked Read-only. Findings are therefore anchored to current file contents rather than diff hunks, and a small number of pre-existing-vs-introduced calls were resolved by reading surrounding code and comments instead of git blame.

🔴 P0/P1 — must fix

  • packages/api/src/opamp/controllers/opampController.ts:373 — The generated OpAMP config declares the connector as span_metrics, but connector/spanmetricsconnector registers the component type spanmetrics, so with ENABLE_SPAN_METRICS=true the remote config fails to decode and — because docker/otel-collector/config.yaml supplies no receivers/exporters/pipelines of its own — the collector has no valid config to run at all.
    • Fix: Rename the connector id to spanmetrics, update both pipeline references at lines 403–405 and 421 plus the span_metrics?: key in the config type at line 108, mirroring how routingconnector is referenced as routing/logs.
    • correctness, adversarial

🟡 P2 — recommended

  • packages/app/src/hooks/useExemplars.tsx:154 — The hook returns a freshly allocated [] on every render when disabled, and DBTimeChart forwards it as the exemplars prop to memo(MemoChart), so the shallow comparison always fails and every time chart in the app re-renders recharts on each parent state change even with the feature flag off.
    • Fix: Return a module-level frozen empty-array constant (or memoize the return value) so the prop identity is stable when there are no exemplars.
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:1401 — Only the marker's y value is clamped; xAxisDomain[1] is reduced to the last bucket start when dateRangeEndInclusive is false (line 1103), so exemplars in the final partial granularity window fall outside the domain and ReferenceDot's default ifOverflow="discard" silently drops them — the most recent window, which is what a live investigation needs.
    • Fix: Clamp or filter the marker's x against xAxisDomain the same way annotationElements already does directly below at line 1121.
  • packages/app/src/hooks/useExemplars.tsx:51normalizePrometheusExemplars discards the entire response when data.length > 1, but Prometheus /query_exemplars resolves selectors and returns one entry per matching series, so the canonical latency query histogram_quantile(0.95, sum(rate(..._bucket[5m])) by (le)) returns one entry per le bucket and yields zero markers with no error.
    • Fix: Merge entries that differ only by le (they are the same chart series split by bucket) and reject only genuinely distinct series.
    • adversarial, maintainability
  • packages/app/src/components/ChartEditor/PromqlChartEditor.tsx:67 — The PromQL exemplars switch renders for any expression and display type with no isExemplarEligible check, and the only PromQL-side scoping is the multi-series drop, so enabling it on rate(http_requests_total[5m]) plots duration-valued exemplars that clampExemplarY pins inside a requests/sec axis as if they were real points on that scale.
    • Fix: Gate the PromQL toggle and fetch on a latency-shaped expression (outermost histogram_quantile, or a _bucket selector), or drop the overlay instead of clamping when the exemplar unit cannot be shown to match the axis.
    • correctness, adversarial, maintainability
  • packages/app/src/hooks/useExemplars.tsx:105 — The exemplar query sets no placeholderData while the main series query uses placeholderData: (prev) => prev (DBTimeChart.tsx:375), and dateRange is part of the query key, so each live-tail tick mints a new key, empties exemplarPoints, and the reset effect at HDXMultiSeriesTimeChart.tsx:1089 force-closes the hover card the user is reaching for.
    • Fix: Add placeholderData: (prev) => prev and a small staleTime to the exemplar query, and quantize the dateRange used in its key so sub-minute ticks don't invalidate it.
  • packages/common-utils/src/core/renderChartConfig.ts:2413ORDER BY value DESC LIMIT 200 ranks the entire ARRAY JOIN output for the selected range, so the limit bounds the result set but not the scan or the sort, and because the cap is global rather than per bucket the surviving rows cluster in a few bad windows and starve the per-window thinning that computeExemplarPoints then applies.
    • Fix: Reduce per time bucket before the global cap (e.g. LIMIT n BY <bucket>, or GROUP BY toStartOfInterval(...) with argMax(ex_TraceId, ex_Value)), following the pre-aggregate-then-rank shape of renderSeriesLimitCte.
    • performance, correctness
  • packages/api/src/opamp/controllers/opampController.ts:414 — The remote-write endpoint is inlined into the generated collector config as a resolved literal, unlike every sibling exporter which uses ${env:...} indirection (lines 266–269, 349), so an endpoint carrying credentials in its URL travels in the /v1/opamp response body and through the logger.debug dump of the whole config.
    • Fix: Emit endpoint: '${env:SPAN_METRICS_PROM_RW_ENDPOINT}' and keep the API-side boolean as the gate, so the secret stays in the collector's environment.
  • packages/app/src/components/DBTimeChart.tsx:442useExemplars computes and returns isError, but the consumer destructures only { exemplars }, so a failing exemplar query — for example an UNKNOWN_IDENTIFIER against a metric table without the Exemplars.* nested columns — renders identically to "no exemplars in this range" with no diagnosable signal.
    • Fix: Destructure isError and surface it non-fatally on the chart so a failed scan is distinguishable from an empty result.
    • correctness, adversarial
  • packages/app/src/hooks/useExemplars.tsx:111 — The PromQL branch calls prometheusApi.queryExemplars without forwarding react-query's context.signal, while the ClickHouse branch passes abort_signal: context.signal at line 142, so a superseded request keeps running against the API's 90s upstream proxy timeout after the key changes or the chart unmounts.
    • Fix: Thread an AbortSignal parameter through prometheusApi.queryExemplars into prometheusFetch's server.post call and pass context.signal from the hook.
    • reliability, adversarial, correctness
  • docker-compose.dev.yml:46SPAN_METRICS_PROM_RW_ENDPOINT is set on the otel-collector service, but packages/api/src/config.ts:71 reads it from the API process (and its own comment says the collector does not need it), so IS_SPAN_METRICS_PROM_RW_ENABLED is always false under yarn dev and the new Prometheus and telemetry-generator services never exercise the native exemplar path they were added for.
    • Fix: Move SPAN_METRICS_PROM_RW_ENDPOINT into the environment the API process loads (packages/api/.env.development) and drop the ineffective entry from the collector service block.
    • correctness, reliability, adversarial, security
  • packages/api/src/opamp/controllers/opampController.ts:364 — The spanmetrics block is gated only on a server-side env flag with no per-agent capability or version check, and agentAcceptsRemoteConfig verifies only the generic AcceptsRemoteConfig bit, so enabling the flag pushes a config referencing the newly added connector to collectors built before builder-config.yaml included it.
    • Fix: Gate the connector and its pipeline on a reported agent capability or minimum collector version, or document that all connected collectors must be rebuilt before the flag is enabled.
  • packages/api/src/utils/zod.ts:266enableExemplars and exemplarTraceSourceId are absent from externalDashboardLineChartConfigSchema/externalDashboardBarChartConfigSchema and from mcpLineTileSchema/mcpBarTileSchema (packages/api/src/mcp/tools/dashboards/schemas.ts:498), so the fields are stripped from any API-key or MCP dashboard write and the feature can only ever be turned on by a human in the chart editor.
    • Fix: Add both fields to the external v2 line/bar config schemas and the corresponding MCP tile schemas so agents can author exemplar-enabled tiles.
  • packages/api/src/opamp/controllers/__tests__/opampController.test.ts:12 — The jest.mock('@/config') getter set omits IS_SPAN_METRICS_ENABLED, IS_SPAN_METRICS_PROM_RW_ENABLED, and SPAN_METRICS_PROM_RW_ENDPOINT, so the entire new config branch reads as disabled in every test and the wrong connector type name shipped undetected.
    • Fix: Add the three flags to the mock and assert the generated connectors/exporters/pipelines keys with the flag on and their absence with it off, cross-checked against the component types in builder-config.yaml.
    • testing, security, adversarial
  • packages/app/src/hooks/useExemplars.tsx:43ExemplarSchema is declared in common-utils/types.ts:1383 as the shared contract for both backends but is only consumed via z.infer; prometheusFetch<T> asserts the untrusted proxy body without validation, and both normalizePrometheusExemplars and mapClickhouseExemplars hand-build objects with Number()/String() coercions instead of parsing.
    • Fix: safeParse each candidate through ExemplarSchema at the two normalizer boundaries and drop entries that fail rather than coercing them into the render path.
  • packages/app/src/components/__tests__/DBTimeChart.test.tsx:42@/hooks/useExemplars is mocked wholesale to constant empty values, leaving the new exemplarTraceSourceId fallback chain, navigateToExemplarTrace URL construction, and hover-card open/close wiring (DBTimeChart.tsx:446–510, 887–926) with no coverage.
    • Fix: Add cases asserting the trace-source fallback precedence, both deep-link shapes, and the hover-card props passed to the mocked chart.
  • .changeset/exemplar-mode-metrics.md:12 — The changeset, which becomes published release notes, states that markers snap onto the series line, but the renderer deliberately plots each exemplar at its own value and clamps it to the axis (HDXMultiSeriesTimeChart.tsx:1068), and it does not mention that the feature is gated off by default.
    • Fix: Correct the description to say markers sit at the trace's own value clamped into the rendered domain, and note the enabling env flag.
🔵 P3 nitpicks (13)
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:653 — The prop default hardcodes 12 instead of importing DEFAULT_MAX_EXEMPLARS from @/defaults, leaving two copies of the same default to drift.
    • Fix: Import and use DEFAULT_MAX_EXEMPLARS as the prop default.
  • packages/common-utils/src/core/renderChartConfig.ts:2411 — A stray word (ponytail:) sits in the comment explaining the exemplar cap, reading as an editing artifact in otherwise carefully annotated code.
    • Fix: Rewrite the sentence without the stray token.
  • packages/app/src/components/Exemplars/exemplarPoints.ts:12finiteOrNull uses !isNaN(v) so it admits ±Infinity despite its name, and exemplar.timestamp is never validated even though it feeds x and the bucket key, so a missing timestamp collapses all such exemplars into one @NaN bucket and emits NaN SVG coordinates.
    • Fix: Use Number.isFinite and apply the same guard to timestamp before bucketing and plotting.
  • packages/app/src/components/Exemplars/ExemplarDot.tsx:35 — The marker wires only onMouseEnter/onMouseLeave with no tabIndex, focus handlers, touch handler, or ARIA, and the card is the only route to "Inspect trace", so keyboard and touch users cannot reach the feature at all.
    • Fix: Add focus/blur handlers mirroring the hover handlers, a tabIndex, Escape-to-dismiss, and a tap path.
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:995cx/cy are captured once at mouseenter, so when a refresh moves a still-hovered marker without unmounting it the diamond shifts while the card stays at the stale coordinates.
    • Fix: Re-emit the hover position from the existing exemplarPoints effect when the hovered key survives with new coordinates.
  • packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx:173 — The stale-flag effect clears enableExemplars on any transient shape change, so briefly switching display type to Table and back permanently unchecks the toggle with no feedback.
    • Fix: Defer the clearing to save time in the form-to-config conversion, where the final shape is known.
  • packages/app/src/components/DBTimeChart.tsx:455hoveredExemplar lives outside the graphResults.length === 0 branch that renders the card and chart, so an empty refetch unmounts both without any hover-end firing and the next non-empty result re-shows the card at stale coordinates.
    • Fix: Clear hoveredExemplar when the chart body is not rendered.
  • packages/app/src/HDXMultiSeriesTimeChart.tsx:1396 — Exemplar dots render from exemplarPoints unconditionally, so with compareToPreviousPeriod on and only the previous-period series selected the markers still draw, clamped against the other series' range.
    • Fix: Skip the dots when the exemplars' series is absent from visibleLineData.
  • packages/common-utils/src/types.ts:1701maxExemplars is z.number().optional() with no .int()/.min()/.max(), so the 0–1000 range exists only as HTML input attributes and a direct PATCH accepts negatives or fractions that silently mean "unlimited" or one marker.
    • Fix: Tighten both the read and update schemas to z.number().int().min(0).max(1000).
  • telemetry-generator/src/index.js:4 — The docblock says the service writes metric data points and exemplars straight to ClickHouse because the collector has no spanmetrics connector, but the file contains no ClickHouse client and this change adds that connector; the same stale framing is repeated at docker-compose.dev.yml:181.
    • Fix: Rewrite both comments to describe the OTLP-traces-only generator with the connector deriving metrics.
  • telemetry-generator/package.json:1 — The new service sits at the repo root outside the packages/* workspace glob, uses npm install in its Dockerfile against a Yarn-workspace repo, and carries an unscoped name with no engines, with the knip.json ignore entry masking the non-membership.
    • Fix: Move it under packages/ as a workspace member with a scoped name, or document the root placement as deliberate.
  • packages/app/src/components/Exemplars/ExemplarHoverCard.tsx:7 — The four body states are encoded as three independently settable props whose valid combinations are enforced only by the render-time ternary chain, so contradictory combinations type-check.
    • Fix: Replace them with a single discriminated-union state prop.
  • packages/app/src/components/DBTimeChart.tsx:442 — The exemplar hook is called without the chart's own enabled && !isLoadingMe gate used for the main query at line 386, and it runs even when the component renders the empty-state branch where no chart mounts.
    • Fix: Pass the chart's gate into the hook so disabled and empty tiles issue no exemplar query.

Reviewers (13): correctness, adversarial, security, performance, reliability, api-contract, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, agent-native, learnings-researcher.

Testing gaps:

  • No assertion that the OpAMP-generated config only references component types packages/otel-collector/builder-config.yaml actually builds — this would have caught the connector type name.
  • useExemplarTraceMeta, the editor toggle and its stale-flag effect, visibleSeriesMax, and the ReferenceDot clamp wiring have no coverage.
  • normalizePrometheusExemplars covers only two of the four accepted trace/span label spellings and no response-shape drift (absent or non-array data, non-numeric value/timestamp, multi-le bucket responses).
  • No test exercises a query-key transition or refetch while a marker is hovered, which is where the overlay-blanking and stale-position behaviors live.
  • No coverage of the /v1/prometheus/query_exemplars upstream error, timeout, or non-2xx paths, nor of the maxExemplars team-setting round trip including the 0 = unlimited branch.

@jordan-simonovski
jordan-simonovski marked this pull request as draft July 8, 2026 23:54
# Conflicts:
#	knip.json
#	packages/app/src/HDXMultiSeriesTimeChart.tsx
#	packages/app/src/components/ChartEditor/utils.ts
#	packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx
#	packages/app/src/components/DBTimeChart.tsx
#	packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx
#	packages/common-utils/src/types.ts
Move the inline ExemplarDot marker and ExemplarHoverCard popover out of the
oversized HDXMultiSeriesTimeChart and DBTimeChart into a focused
components/Exemplars/ directory, and add a Storybook story for each so the
marker and every hover-card state (full/partial metadata, loading, not-found,
no-trace-source) can be developed and reviewed in isolation across light/dark
and both brand themes.

Behaviour-preserving: the extracted component bodies are unchanged; ExemplarDot
also gains a real props type in place of `any`.
…crushing the axis

Addresses review feedback (Greptile P1 + Deep Review P2s) and hardens the
exemplar overlay while it's still being tested.

Correctness / scoping:
- Metric exemplar SQL now ANDs the metric-name predicate separately instead of
  appending it to the user filter group, so a chart with filtersLogicalOperator
  'OR' can no longer let the exemplar scan match other metrics.
- Prometheus normaliser drops the overlay when the query returns >1 series
  (exemplars are single-series only), and the PromQL result is capped
  client-side (no native limit param) to bound render/thinning work.
- enableExemplars is cleared when a chart leaves single-series so a stale flag
  can't persist on a config that no longer supports it.

Feature gate:
- Whole feature (editor toggle, PromQL toggle, data fetch, team setting) gated
  behind NEXT_PUBLIC_ENABLE_EXEMPLARS; on in dev, off by default elsewhere.

Rendering:
- Exemplar outliers no longer stretch the y-axis: the domain follows the visible
  series and each marker is clamped to the series max (its hover card still shows
  the true value), so one slow trace can't flatten the p99 line.
- The series hover tooltip is suppressed while hovering an exemplar so the two
  cards don't overlap; the chart resets this against the rendered points so a
  refetch/re-thinning that unmounts the hovered marker can't leave it stuck.
- Higher-contrast marker outline for light mode.

Maintainability / tests:
- Exemplar bucketing extracted to a pure, unit-tested helper; Prometheus
  exemplar response type de-duplicated. Added SQL OR-filter and multi-series
  regression tests.
The exemplars work pushed packages/app over the --max-warnings 740 floor.
Fix five real warnings (drop an unnecessary offsetParent cast, avoid an
empty-function stub, rename a useId/ref binding, narrow traceSourceId with
an 'in' check instead of a cast) and disable no-unnecessary-use-prefix in
DBSearchPageQueryKey.test.tsx, where jest.mock factories must mirror the
real hook names.
Resolves the five P2 findings from the PR review, plus two follow-ons the
re-review surfaced in the fixes themselves.

- Exemplar eligibility (single, non-ratio, non-grouped histogram series) now
  lives in one predicate, isExemplarEligible, which both the SQL renderer and
  the chart editor delegate to. The missing Group By guard came from the rule
  being reimplemented per call site: a grouped chart pooled exemplars from every
  group into one unattributable set, so the renderer now returns null for it.
- Exemplar markers are clamped into both ends of the y-axis domain the chart
  actually renders, not just the top. A fitYAxisToData floor can sit above an
  individual duration when the series is a high quantile, and recharts'
  ReferenceDot discards out-of-domain markers — which silently emptied the
  overlay. Extracted as pure helpers so the bounds are testable.
- The stale-flag cleanup effect no longer keys on NEXT_PUBLIC_ENABLE_EXEMPLARS,
  so opening a saved exemplar chart with the deployment flag off suppresses the
  overlay instead of stripping the persisted config on the next save. It also
  waits for the async source to resolve (shape unknown != shape wrong, which
  stripped config on every cold load) and leaves exemplarTraceSourceId intact,
  so re-enabling after a Group By typo is lossless.
- Tests: the useExemplars hook (gating, ClickHouse row mapping, the PromQL
  result cap, error surfacing), isExemplarEligible's matrix including the
  enum-casing hazard, the y-clamp bounds, the Group By and non-histogram null
  branches, and the query_exemplars 400/404 guards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant