feat(app): exemplar overlay for metric and PromQL time charts - #2808
feat(app): exemplar overlay for metric and PromQL time charts#2808jordan-simonovski wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: c048ae2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
Greptile SummaryAdds an opt-in exemplar overlay for metric and PromQL time charts, placing trace-linked samples on the chart’s shared axis.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and the previously reported stale exemplar-card lifecycle issues are addressed by direct cleanup when markers or plot subtrees disappear and coordinate re-anchoring when active markers move.
|
| Filename | Overview |
|---|---|
| packages/app/src/components/DBTimeChart/useExemplarCard.ts | Implements hover, pin, delayed-close, re-anchoring, navigation, and plot-lifecycle cleanup for exemplar cards; the previously reported stale-card paths are addressed. |
| packages/app/src/HDXMultiSeriesTimeChart/useExemplarMarkers.ts | Computes rendered exemplar markers, applies axis clamps, tracks the active marker, and closes cards when their marker leaves the rendered set. |
| packages/app/src/components/Exemplars/ExemplarDot.tsx | Renders interactive exemplar markers and reports active-marker coordinate changes needed to keep cards anchored after resizes and rescaling. |
| packages/app/src/components/DBTimeChart/DBTimeChart.tsx | Integrates exemplar retrieval, notices, card state, toolbar configuration, and marker callbacks into time-chart rendering. |
| packages/app/src/hooks/useExemplars/useExemplars.tsx | Fetches eligible metric or PromQL exemplars using quantized query ranges and chart-series constraints. |
| packages/common-utils/src/core/renderChartConfig.ts | Preserves the exemplar chart-config option through shared chart rendering and normalization. |
Sequence Diagram
sequenceDiagram
participant Chart as DBTimeChart
participant Hook as useExemplarCard
participant Plot as MemoChart
participant Marker as ExemplarDot
participant Card as ExemplarHoverCard
Chart->>Hook: load eligible exemplars
Chart->>Plot: render exemplars and lifecycle callbacks
Plot->>Marker: render sampled marker
Marker->>Hook: hover/select with pixel coordinates
Hook->>Card: show hover or pinned trace card
Marker->>Hook: report changed coordinates
Hook->>Card: re-anchor active card
Plot->>Hook: marker removed from rendered set
Hook->>Card: close stale card
Reviews (10): Last reviewed commit: "fix(app): clear exemplar cards when the ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
<!-- deep-review --> Deep Review
✅ No critical issues found in the code I was able to read — this is not a clean bill of health for the PR, since coverage was partial. 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Notes on what I did verifyThree things I checked that are correct, recorded so they are not re-flagged on a future pass:
Reviewers (1 of 13 reported): learnings-researcher. Dispatched but no results returned — none of their analysis is in this report: correctness, adversarial, security, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, performance, reliability, api-contract, agent-native. Coverage limitations:
Recommendation: re-run this review in an environment with working |
186c54f to
082981c
Compare
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes 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
|
E2E Test Results✅ All tests passed • 269 passed • 1 skipped • 1000s
Tests ran across 4 shards in parallel. |
9e57b96 to
240df2b
Compare
240df2b to
28ac9fb
Compare
|
<!-- deep-review --> Deep Review
✅ No critical issues found by the one reviewer that completed. 🟡 P2 — recommended
🔵 P3 nitpicks (1)
Reviewers (1 of 12 completed): testing. Dispatched but not returned: correctness, adversarial, security, reliability, performance, api-contract, frontend-races, maintainability, kieran-typescript, project-standards, agent-native. Testing gaps:
Two unverified leads worth chasing before merge — surfaced during scope reconstruction, not confirmed by a reviewer, so deliberately left unscored:
|
Deep ReviewWhat blocked itEvery This reproduces for trivial commands (
Why no findings were emittedStage 1 of the review skill (scope detection) requires
Reviewing guessed file paths at To unblock
The untracked Reviewers (0): none dispatched — halted at scope detection. Testing gaps: not assessed; the PR's own stated verification ( |
78e6486 to
ce508d5
Compare
Time charts on metric and PromQL sources can overlay exemplars — individual trace-linked points — behind NEXT_PUBLIC_ENABLE_EXEMPLARS, off by default, and per-chart behind enableExemplars. Hovering a marker shows the exemplar's own value and time plus trace metadata, with a button to open the trace. The shared common-utils primitives ship here rather than separately: every one of those exports is consumed only by the app, so landing them alone would fail the unused-export check. The rule the whole feature turns on is that a marker sits at the trace's own measurement on the chart's shared axis, so it is only honest when the chart draws one line in the same unit. That is enforced in four places: a single non-ratio histogram series with no group by; an aggregation that leaves the axis on the observation scale (a count of observations is not a duration); for PromQL an expression that plots a duration, with the duration call spanning the whole expression; and a rendered-series count taken from the main query rather than the exemplar response, since Prometheus only returns series that carry a sampled exemplar and so cannot say how many lines are drawn. Where a marker cannot be drawn honestly it is dropped, not moved. Out of the rendered window by more than one bucket, or below a fitted y-axis floor, and it does not render — with the count surfaced on the chart, because a silently thinning overlay is worse than an explained one. The exception is above the ceiling, where pinning reads as "at least this high" and the card carries the real number. Includes the four rounds of review fixes from #2536: the hover card showing value and time (both clamps cite it as their justification), the PromQL aggregation parsing that decides series identity, the ClickHouse scan's ex_TimeUnix bound, the query-key quantisation, and the card lifecycle across zoom, live tail and display-type switches. make ci-lint and make ci-unit pass (5350 tests). Dashboard E2E 80 passed, 0 failed, 2 unrelated flakes on listing-page specs. Stacked on the chart-file refactor so this reads as a feature diff.
Addresses a review finding on the exemplar overlay. Both cards are positioned from pixel coordinates captured when the pointer touched the marker, so a zoom, range switch or rescale leaves them beside the wrong diamond. The effect that handles this returned early when nothing was pinned, which left a hover-only card behind — and the hover card has less recourse than the pinned one, because a marker sliding out from under a stationary cursor fires no mouseleave, so nothing else closes it. It sat at stale coordinates and suppressed the series tooltip until the pointer moved. Keyed on the quantised range alone now, so both cards close together. Live-tail ticks still fall inside one bucket and leave a pinned card alone. Tests drive the range change through RTL's `wrapper` option rather than a plain rerender: rerendering a differently-shaped tree remounts the chart, which clears the cards by itself and would have let all three cases pass with the effect deleted. Also restores the exemplar-clamp coverage in useChartScales, which the split PR could not carry because the clamp only exists on this branch.
Addresses a follow-up review finding on the previous fix. Keying the card cleanup on the quantised date range was wrong in both directions. Too coarse: a zoom inside the 30-second bucket moved every marker and left the cards behind at stale coordinates, pointing at empty space or someone else's trace. And in the wrong place: the 30-second quantum belongs to the exemplar query key, not to the card lifecycle, so borrowing it coupled two unrelated concerns. Moved into useExemplarMarkers and keyed on the rendered x-domain, which is what actually maps a data point to the pixels a card was positioned from. That domain is already floored to the chart's granularity, so a live-tail tick inside the current bucket still leaves an open card alone — the behaviour the earlier fix was protecting — while crossing a bucket closes it, which is right, because there the markers really do jump a bucket's width. The y-range is deliberately excluded: it shifts on nearly every live-tail refetch, the vertical drift is small, and a marker pushed out of range is already caught by the clamps feeding the existing guards. Tests moved with the behaviour, onto the hook that now owns it.
Main ratcheted the app warning ceiling from 740 to 663 against its own content. This branch adds roughly fifteen files, which puts it five over. Three of the eight were real and are fixed here: empty function bodies in the useExemplars tests, replaced with a named never-settling promise and a comment saying what the empty act() is for. The remaining five are left and the ceiling moved to match. Four are assertion patterns in DBTimeChart's test that came across unchanged in the file split, and `jest.mocked` is not a drop-in for them — it restores the real signatures, which the partial mock returns in that file do not satisfy. The fifth is a setState-in-effect warning matching a pattern main carries about twenty times. common-utils goes 92 to 93 for one more security/detect-object-injection, on the metric-table lookup in the exemplar query builder. The file already carries two of the same indexing pattern.
…ble delay Addresses a review finding on the previous fix. The domain-change effect reused onExemplarHoverEnd, which schedules a close the card's own mouseenter cancels — that delay exists so the cursor can travel from a marker into the card. So a cursor resting in the card when the markers moved cancelled the cleanup and held the card open at stale coordinates, still suppressing the series tooltip: the exact state the fix was meant to prevent. Moving markers now go through their own signal with an immediate close for both cards. The delay stays where it belongs, on pointer-leave.
Addresses a review finding, and replaces two earlier attempts at the same thing. A card is anchored to the pixel coordinates the marker's SVG shape reported when the pointer touched it. Both previous fixes closed the card when something that moves markers happened, and both picked the wrong thing to watch: first the quantised date range, which was too coarse to catch a zoom inside the bucket, then the rendered x-domain, which missed a y-axis rescale and a container resize entirely. Guessing at the causes was the mistake. Recharts recomputes cx/cy for every cause, so the marker itself is the only place that knows. It now reports its position and the card re-anchors, which covers all of them and does not need to enumerate any. Only the marker the card is describing reports, so it is one call per change rather than one per marker per frame. Following the marker is also better than closing: a card the user deliberately pinned survives a live-tail tick, a zoom and a resize, which is what the earlier quantisation was trying to buy and kept trading away. The guards for a marker leaving the rendered set are unchanged — that is a real disappearance, and there the card still closes.
Addresses a review finding on the previous fix. Loading, error, formatting-error and empty-data states replace the chart's whole subtree, so the marker layer unmounts and nothing can report a position. The card state survived that, so recovering left the card pinned at coordinates belonging to the previous chart instance until a remounted marker happened to report — and if the marker was gone from the new data, until the rendered-set guard fired. Cleared on the same signal now, alongside the existing display-type case: both are "the markers a card was anchored to no longer exist". The branch condition is derived once and shared with the JSX rather than restated, so the cleanup cannot drift from the branches that cause it.
ce508d5 to
c048ae2
Compare
1 participant
The feature itself. Stacked on #2807 (the chart-file refactor) — review that first; this diff is the feature only.
Fourth of the PRs replacing #2536.
What it does
Time charts on metric and PromQL sources can overlay exemplars — individual trace-linked points. Hovering a marker shows the exemplar's own value and time plus trace metadata, with a button to open the trace.
Off by default for the whole deployment behind
NEXT_PUBLIC_ENABLE_EXEMPLARS, and per-chart behindenableExemplars.The rule the whole feature turns on
A marker sits at the trace's own measurement on the chart's shared axis. So it is only honest when the chart draws one line, in the same unit. That is enforced in four places, and most of the review findings on the original PR were cases where one of them leaked:
histogram_quantile(...) * 1000renders a different unit than its exemplars carry).Where a marker cannot be drawn honestly, it is dropped
Not moved. Outside the rendered window by more than one bucket, or below a fitted y-axis floor, and it does not render — with the count surfaced on the chart, because an overlay that silently thins out is worse than one that explains itself. The exception is above the ceiling, where pinning reads as "at least this high" and the hover card carries the real number.
Note on
common-utilsThose primitives ship in this PR rather than their own. Every one of the exemplar exports is consumed only by the app — landing them alone would fail the unused-export check with no consumer.
Included from the original PR's review rounds
Four rounds, roughly 1 P0/P1 and 15 P2s each. The substantive ones: the hover card now shows value and time (both clamps cite it as their justification and it showed neither); the PromQL aggregation parsing that decides series identity, including label matchers and quoted label names; the ClickHouse scan's
ex_TimeUnixbound, which was previously filtering on the row time while projecting the exemplar time; the query-key quantisation, which now fetches the window it keys; and the card lifecycle across zoom, live tail and display-type switches.Remaining P2/P3s are tracked in #2804 rather than blocking this.
Verification
make ci-lint,make ci-unit(5,350 tests), dashboard E2E 80 passed / 0 failed (2 flakes on listing-page specs that draw no chart).