Exemplars: P2/P3 follow-ups from #2536 review rounds

[jordan-simonovski]

Follow-up backlog from the review rounds on #2536. Splitting these out so the PR
can merge on its P0/P1s rather than re-litigating the same batch every round.

Why this is safe to defer: the overlay is behind NEXT_PUBLIC_ENABLE_EXEMPLARS,
off by default, and additionally per-chart behind enableExemplars. None of the
below is reachable in a default deployment.

Context on the numbers. The deep reviewer returns a similar-sized batch every
run against the whole feature surface — roughly 1 P0/P1 and ~15 P2 each time,
across four runs. It is not a growing defect count; it is a full re-review each
time, with overlap. Several items below started as round-1 P3s and were re-graded
to P2 in later rounds because they were never fixed, which is the argument for
tracking them here instead of descoping them again.

Themes

• Accessibility. ExemplarDot has no keyboard focus, no touch path, and no
  ARIA; the hover card is the only route to "Inspect trace", so those users cannot
  reach the feature at all. Flagged since round 1.
• useExemplarTraceMeta hardening. Hand-quoted database/table names, no time
  predicate on the trace lookup, a query key that omits the source expressions the
  SQL interpolates, and Number(row.durationMs) on a non-numeric expression.
• Marker placement edges. Thinning runs before clamping; the one-bucket
  tolerance applies to the lower bound as well as the upper; the maxExemplars <= 0
  branch dedupes differently from the budgeted branch.
• Backend consistency. Three separate implementations answer "cap exemplars per
  time bucket" (client, SQL, render), and the code documents that the two backends
  therefore show different marker sets for the same chart.
• API surface. /query_exemplars maps every thrown error to 400 bad_data; a
  PromQL chart on a ClickHouse-backed connection passes every client gate and gets
  an empty success; enableExemplars is settable via MCP on tiles where it does
  nothing, with no warning back to the caller.
• Schema and docs. maxExemplars accepts up to 1000 while EXEMPLAR_QUERY_LIMIT
  is 200; ExemplarSchema.attributes is never populated or read; several comments in
  types.ts and telemetry-generator describe a pipeline that no longer exists.
• Disputed. Round 2 asked for histogram_quantile(...) * 1000 to be rejected as
  a unit mismatch and it was; round 4 flags that rejection, since it is the standard
  seconds-to-milliseconds idiom. Needs a decision, not a fix.

Full list

Verbatim from the two most recent deep reviews. Line numbers are from those runs
and have since moved.

• packages/app/src/components/Exemplars/exemplarPoints.ts:129 — The window split triggers on ordered.length > ceil(maxExemplars * 0.75) rather than > maxExemplars, so at the default budget of 12 a chart with 12 populated buckets renders only 9 markers and three buckets get none, contradicting the docstring's "more buckets than the marker budget".

  • Fix: Gate the split on ordered.length <= maxExemplars and keep windowCount only as the split width so every bucket that fits the budget still emits its rank-0 marker.

• packages/app/src/HDXMultiSeriesTimeChart/useExemplarMarkers.ts:135 — A hovered marker's React key and data x are fixed to its timestamp while xAxisDomain shifts every live-tail tick, so the same <g> node slides out from under a stationary cursor with no mouseleave: the card keeps rendering at the x/y captured at mouseenter and isExemplarHovered stays true, suppressing the series tooltip until the pointer moves.

  • Fix: Re-read the hovered point's current x/y from exemplarPoints on every recompute to reposition the card, and re-validate the pointer against the marker's current node so hover-end fires when it no longer sits underneath.

• packages/app/src/HDXMultiSeriesTimeChart/MemoChart.tsx:202 — ChartComponent swaps between AreaChart and BarChart, a different element type that fully remounts every ExemplarDot without dispatching mouseleave, and the reset guards only fire when the key is absent from exemplarPoints, so switching display type while a card is pinned orphans it over markers that no longer exist.

  • Fix: Add an effect keyed on displayType that clears the hovered and pinned exemplar state before the chart subtree remounts.

• packages/app/src/components/Exemplars/ExemplarHoverCard.tsx:147 — useExemplarCard destructures only data and isLoading from useExemplarTraceMeta, so a failed trace query and a source whose exemplarTraceSourceId is not a Trace kind both arrive as meta === undefined, isLoading === false and render "Trace not found in source", reporting a misconfiguration or query error as a missing trace.

  • Fix: Thread isError and an unsupported-source state through to the card and render a distinct "could not load trace details" message for each.

• packages/app/src/HDXMultiSeriesTimeChart/useChartScales.ts:120 — hasSelection alone (without fitYAxisToData) switches yAxisDomain to a numeric pair whose lower bound is the data minimum, so one click on the sole legend entry of a single-series chart makes clampExemplarY return null for every below-floor marker and silently reverts the overlay to a max envelope with no notice.

  • Fix: Fit the y-axis floor only when fitYAxisToData is set, or surface the count of markers dropped by clampExemplarY in the exemplar notice.

• packages/app/src/hooks/useExemplars/exemplarNormalize.ts:23 — collapsesHistogramBuckets tests the literal substring histogram_quantile( while isPromqlExemplarEligible allows \s* before the paren, so histogram_quantile (0.95, …) passes the toggle gate, keeps le in the group key, and returns dropped: 'multiple-series' with a notice telling the user to aggregate to a single line they already have.

  • Fix: Make collapsesHistogramBuckets use the same whitespace-tolerant, literal-stripped regex the eligibility gate uses so the two checks cannot disagree about one expression.

• packages/api/src/routers/api/prometheus.ts:232 — The pipeline() catch cannot distinguish a client disconnect from an upstream failure and always returns 502, so ordinary tab closes and live-tail supersessions increment hyperdx.prometheus.query_errors, the counter whose own docblock scopes it to backend health for alerts and SLOs.

  • Fix: Detect a client-initiated close in the pipeline catch and return the already-written upstream status so recordProxyOutcome does not count it.

• packages/api/src/routers/api/prometheus.ts:165 — proxyToPrometheus never receives req, so the only abort on the outbound fetch is the 90s AbortSignal.timeout; the client forwards its abort signal precisely because live-tail supersedes the request every tick, yet the API keeps executing the superseded upstream query to completion.

  • Fix: Pass req in and combine a signal from its close event with the timeout via AbortSignal.any so a client disconnect cancels the upstream fetch.

• packages/app/src/hooks/useExemplars/quantize.ts:13 — EXEMPLAR_KEY_QUANTUM_MS is 30s while EXEMPLAR_STALE_TIME_MS is 60s, and fetchRange is part of the query key, so the key changes twice per stale window and the declared 60s staleness tolerance can never suppress a fetch on a live-tail chart.

  • Fix: Derive one constant from the other so the key quantum is at least the stale time.

• packages/app/src/hooks/useExemplars/useExemplarTraceMeta.ts:42 — The hover query is WHERE TraceId = {traceId:String} with no time predicate and enabled the instant a traceId appears, so sweeping the cursor across a marker cluster fires one unbounded trace-table lookup per 9px hit circle crossed with no debounce, even though the exemplar's own timestamp and an EXEMPLAR_TRACE_WINDOW_MS precedent are already available.

  • Fix: Bound the query with a window around the exemplar's timestamp and debounce the hovered traceId by the same grace the close timer already uses.

• packages/app/src/HDXMultiSeriesTimeChart/useExemplarMarkers.ts:135 — The hover-reset, pin-reset, and suppressNextClickRef guards have no tests, and neither do useExemplarCard's quantized auto-unpin, Escape handler, or the two URL shapes in navigateToExemplarTrace, despite the hook's own docblock stating every bug in this layer came from a marker outliving its data.

  • Fix: Add renderHook tests that drop a hovered and a pinned marker from the rerendered exemplars array, assert the reset callbacks fire, and cover the same-window vs changed-window unpin cases.

• packages/app/src/components/DBTimeChart/DBTimeChart.tsx:317 — plottedSeriesCount filters out isDashed comparison lines to fix a bug the surrounding comment describes, but DBTimeChart.test.tsx mocks @/hooks/useExemplars wholesale and never inspects its arguments, so re-counting the dashed previous-period line would pass every existing test.

  • Fix: Spy on useExemplars in one test, render with one solid plus one isDashed series, and assert plottedSeriesCount === 1.

• packages/api/src/routers/api/prometheus.ts:556 — prometheus.test.ts covers only the extracted pure resolveExemplarWindow, leaving the handler's isPrometheusEndpoint branch selection, the ClickHouse-backed empty-success response, and the deliberate 5xx-only error-counter rule unexercised.

  • Fix: Add a route-level test with mocked getConnectionById and fetch asserting the narrowed start is proxied, the ClickHouse branch returns data: [] without fetching, and a 400 does not increment the error counter.

• packages/api/src/routers/api/prometheus.ts:190 — The new proxy route reaches a user-supplied connection.host with no protocol or private-IP check and with fetch's default redirect: 'follow', while the sibling connection-test path in clickhouseProxy.ts already applies isPrivateIp to the same field.

  • Fix: Validate connection.host with isPrivateIp at write time and pass redirect: 'manual' in proxyToPrometheus.

• packages/app/src/components/DBTimeChart/useExemplarCard.ts:86 — 'traceSourceId' in source is never true for SourceKind.Metric or SourceKind.Promql (neither MetricSourceSchema nor PromqlSourceSchema declares that field, and zod strips unknown keys), so the documented fallback is dead code and a chart without an explicit exemplarTraceSourceId can never resolve trace metadata.

  • Fix: Resolve the fallback by hopping through the metric source's logSourceId to that log source's traceSourceId, or drop the fallback and require exemplarTraceSourceId.

• packages/app/src/HDXMultiSeriesTimeChart/useExemplarMarkers.ts:134 — Thinning runs before clamping and the 30s fetch quantum can place exemplars up to 45s past the rendered x-domain at 15-second granularity, so marker-budget slots are spent on points that clampExemplarX then drops, permanently raising a notice that blames a fitted y-axis floor on a correctly configured chart.

  • Fix: Trim exemplars to the drawn x-domain before thinning, and exclude quantization-surplus drops from the count reported through onExemplarsDropped.

• packages/app/src/hooks/useExemplars/useExemplarTraceMeta.ts:50 — The per-hover trace lookup filters only on TraceId with no time predicate and sets no retry, so each hovered marker can trigger a full trace-table scan retried three times with backoff before the card can report failure.

  • Fix: Pass the exemplar's timestamp in and add a bounded timestampValueExpression BETWEEN predicate, and set retry: 1 to match the sibling hook.

• packages/app/src/components/Exemplars/exemplarPoints.ts:270 — The one-bucket tolerance is applied to the lower bound as well as the upper one, so exemplars fetched by quantizeStart's widened window are snapped forward onto the first plotted bucket and drawn at a time they did not occur, without being counted as dropped.

  • Fix: Make the tolerance one-sided — if (x < min || x > max + tolerance) return null; — since only the end-exclusive upper bound needs it.

• packages/app/src/hooks/useExemplars/exemplarNormalize.ts:106 — The full Prometheus body is materialised and run through one ExemplarSchema.safeParse per exemplar on the UI thread before any cap applies, and is discarded wholesale when the response spans multiple series.

  • Fix: Decide the multiple-series drop from seriesLabels before parsing any exemplar, and short-circuit once the parsed count exceeds a hard multiple of EXEMPLAR_QUERY_LIMIT.

• packages/api/src/routers/api/prometheus.ts:646 — A PromQL chart on a ClickHouse-backed connection passes every client gate and receives {status:'success', data: []}, so the toggle is on, no markers appear, no notice explains it, and a proxy round-trip is paid per quantised window.

  • Fix: Return a distinguishable marker such as unsupported: true for the non-Prometheus branch and surface it as an exemplar notice, or gate the editor toggle on connection.isPrometheusEndpoint.

• packages/app/src/hooks/useExemplars/useExemplarTraceMeta.ts:36 — Database and table names from the source document are hand-quoted with backticks and spliced into the query, unlike every sibling renderer which binds them as {Identifier} parameters, so a name containing a backtick terminates the quoted identifier.

  • Fix: Build the FROM clause through the parameterized { Identifier: ... } path that renderFrom uses instead of string concatenation.

• packages/api/src/routers/api/prometheus.ts:633 — The new route adds another entry point into proxyToPrometheus, which fetches a member-writable connection.host server-side with no scheme or address validation and default redirect-following, then streams the body back to the caller.

  • Fix: Harden proxyToPrometheus once for all four callers by rejecting non-http(s) schemes, blocking loopback/link-local/RFC1918 targets, and setting redirect: 'manual'.

• packages/api/src/routers/api/prometheus.ts:647 — The handler's catch maps every thrown error to HTTP 400 bad_data and increments prometheusQueryErrors unconditionally, so a Mongo failure in getConnectionById reads as a client mistake while a malformed timestamp pollutes the counter that recordProxyOutcome deliberately keeps 5xx-only.

  • Fix: Return 5xx for errors that are not recognised input-validation failures, and restrict the counter increment to those.

• packages/app/src/components/Exemplars/ExemplarDot.tsx:51 — The marker hard-codes hex fallbacks and reuses --color-text-default for its stroke, which agent_docs/data_viz_colors.md explicitly forbids in chart components ("No new hex strings in chart components").

  • Fix: Source the fill from getChartColorWarning() and the outline from a chart border token rather than a text token or a literal hex.

• packages/app/src/components/Exemplars/ExemplarDot.tsx:37 — The marker <g> carries only onMouseEnter/onMouseLeave/onClick with no tabIndex, role, aria-label, or key handler, and the card only appears on marker hover, so the entire exemplar-to-trace path is unreachable without a pointer.

  • Fix: Give the marker a focusable role with an accessible label and open the card on focus and Enter/Space.

• packages/app/src/hooks/useExemplars/exemplarNormalize.ts:9 — The hooks layer imports labelDistinguishesSeries/promqlSeriesLabelRule directly from @/components/Exemplars/promqlSeriesLabels, which components/Exemplars/index.ts does not re-export despite documenting itself as the folder's public surface.

  • Fix: Move promqlSeriesLabels.ts into hooks/useExemplars/ since it is pure string logic, or re-export it from the barrel.

• packages/api/src/mcp/tools/dashboards/schemas.ts:537 — enableExemplars and exemplarTraceSourceId are settable through the MCP dashboard tools, but clickstack_query_tile and clickstack_timeseries only run the tile's main series query, so an agent can enable the overlay and never read back whether markers exist or what traces they point at.

  • Fix: Extend the tile/timeseries query tools to run the exemplar query and return the resulting Exemplar[].

• packages/app/src/HDXMultiSeriesTimeChart/useExemplarMarkers.ts:57 — Neither this hook nor packages/app/src/components/DBTimeChart/useExemplarCard.ts has any test, leaving the clamp wiring, drop-count reporting, hover/pin reset-on-disappear, post-zoom click swallow, Escape-to-close, and range-quantized unpin entirely unverified while DBTimeChart.test.tsx mocks the data hooks away.

  • Fix: Add renderHook tests for both hooks covering the reset guards, the drop-count effect, the click-swallow branch, and the pin/hover precedence rules.

• packages/api/src/routers/api/prometheus.ts:570 — packages/api/src/routers/api/__tests__/prometheus.test.ts covers only the pure helpers, so the new handler's branch dispatch, missing-param 400s, connection-not-found 404, unauthenticated rejection, and the 502/504 proxy paths have no route-level coverage.

  • Fix: Add supertest cases for the Prometheus-proxy and ClickHouse branches plus the 400/404/502/504 paths.