Repository navigation
feat(app): add a duration heatmap view to trace search - #3184
alex-fedotyev wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 46b3d9e 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 |
 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| </IsolatedChartSyncProvider> | ||
| ) : ( | ||
| <ChartCard style={{ height: '100%', minHeight: 0 }}> | ||
| <DBHeatmapChart |
There was a problem hiding this comment.
🟠 major — Heatmap mode blanks to "Loading…" and recreates the uPlot chart on every live-tail tick
Results mode leaves isLive at its default true (DBSearchPage.tsx:1095; the force-off effect at :1100 only covers delta/pattern, which is why the delta-view heatmap never hit this). Every DEFAULT_REFRESH_FREQUENCY = 10s tick calls refresh() with new Date(Date.now() - interval), new Date() (DBSearchPage.tsx:747), so searchedTimeRange — and therefore heatmapChartConfig.dateRange — is a brand-new absolute range each tick. That produces new minMaxConfig/bucketConfig query keys (DBHeatmapChart.tsx:770,794), and unlike DBTimeChart (which passes placeholderData: prev => prev, DBTimeChart.tsx:509) the heatmap queries have no placeholder, so isMinMaxLoading is true and HeatmapContainer replaces the whole chart with the Loading... text (DBHeatmapChart.tsx:860). key={JSON.stringify(config)} (DBHeatmapChart.tsx:873) then destroys and recreates the uPlot instance, and the recomputed min/max makes the y-axis jump. Fix: add placeholderData: (prev: any) => prev to both useQueriedChartConfig calls in HeatmapContainer (mirroring DBTimeChart), or force live tail off while traceChartMode === 'heatmap' the way the analysisMode effect at DBSearchPage.tsx:1100 does.
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
There was a problem hiding this comment.
Confirmed and fixed in 46b3d9e. I took the second option: results mode now pins live tail off while the heatmap is the visible chart, matching what delta and pattern modes already do (which is why the delta-view heatmap never hit this). placeholderData alone would stop the Loading flash but not the teardown, since key={JSON.stringify(config)} still changes when the tick moves the date range.
| // (undefined -> id) doesn't count as a change and a shared link that pins | ||
| // ?traceChartMode=heatmap survives. | ||
| const prevSourceIdRef = useRef<string | undefined>(undefined); | ||
| useEffect(() => { |
There was a problem hiding this comment.
🔵 minor — Reset effect discards the user's Heatmap choice even when the new source can render it
The effect fires on any searchedSource?.id change, so switching from trace source A to trace source B — both with a durationExpression, so both render the toggle via traceRedMetricsSource — silently flips the view back to RED. The sibling view toggle analysisMode (:1067) is deliberately not reset on source change even though delta needs a trace source too; that branch just renders nothing (:2637). Drop the effect (both the switch and the chart are already hidden when traceRedMetricsSource == null, so a lingering ?traceChartMode=heatmap is inert), or gate the reset on the new source not supporting the heatmap.
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
| histogramTimeChartConfig={ | ||
| histogramTimeChartConfig | ||
| } | ||
| heatmapChartConfig={{ |
There was a problem hiding this comment.
🔵 minor — heatmapChartConfig is a fresh object literal every render, so the tile-config memo never memoizes
The inline {...chartConfig, dateRange: searchedTimeRange, with: aliasWith} gets a new identity on every render, which is the single dep of useMemo in TraceRedMetricsChart.tsx:121-135 — so toHeatmapChartConfig and both buildHeatmap*Config calls rerun each render, exactly the failure the sibling durationFormat memo comment at TraceRedMetricsChart.tsx:86 warns about. The page already memoizes the near-identical shape as filtersChartConfig (:2035, which adds only orderBy: undefined) and histogramTimeChartConfig (:1926). Hoist one useMemo for {...chartConfig, dateRange: searchedTimeRange, with: aliasWith} and pass it to both this callsite and the DBSearchHeatmapChart one at :2641.
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
| } as const; | ||
|
|
||
| return ( | ||
| return mode === 'red' ? ( |
There was a problem hiding this comment.
🔵 minor — No test covers the mode branch, the heatmap tile config, or the new query-key prefix
DBSearchPageQueryKey.test.tsx exists specifically to pin the queryKeyPrefix contract for DBTimeChart/SearchTotalCountChart, but nothing asserts that the heatmap's two keys now carry it — and DBSearchHeatmapChart.test.tsx:55 mocks DBHeatmapChart and toHeatmapChartConfig wholesale, so it cannot catch a regression there either. Add (a) an assertion in DBSearchPageQueryKey.test.tsx that both heatmap queryKeys start with the passed prefix, and (b) a TraceRedMetricsChart test that mode="heatmap" renders the heatmap (not the three tiles) with valueExpression from getDurationMsExpression and numberFormat === DURATION_HEATMAP_NUMBER_FORMAT, since the ms-expression/format pairing is the one thing that silently produces wrong axis units if it drifts.
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
| * are fixed here rather than read from the delta view's settings drawer, so | ||
| * this stays a plain duration heatmap independent of that view's query state. | ||
| * | ||
| * @source packages/app/src/components/Search/DBSearchHeatmapChart.tsx (tile config) |
There was a problem hiding this comment.
🔵 minor — @source tag used for a same-package partial copy raises the advisory ratchet count above baseline
The conventions reserve @source packages/... for "when an entire file has to be copied across packages"; this is a same-package, partial mirror of one config block. scripts/ci/ratchet.mjs:24 counts /@source packages\//g and the app baseline is 0 (scripts/ci/ratchet-baseline.json:12), so every CI run will now print ! app/@source: 1 > baseline 0 (advisory, non-fatal). Replace it with a plain doc comment naming the twin — which is what the DRY table's "Two genuinely must coexist" row actually asks for — or run yarn ratchet:update to re-baseline deliberately.
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
PR Review
4 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 3 minor 2 posted as inline comment(s) on the changed lines. 2 listed below. Findings outside the changed lines2 minor
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. No P0/P1 ship-blockers. The core mechanics verify cleanly: the 🟡 P2 — recommended
🔵 P3 nitpicks (3)
Reviewers (11): correctness, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, performance, adversarial, previous-comments, agent-native, learnings-researcher. Testing gaps: no test asserts the heatmap queries carry 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 |
The service dashboard and the LLM expressions each spelled out their own "is this an error" predicate as lower(<status>) = 'error'. Collectors that emit the long OTel form (STATUS_CODE_ERROR) therefore fell outside it, so error rates and error counts on those dashboards undercounted. Pull the predicate into one exported helper in source.ts and match both forms. The logs path passes a severity-text expression, which never takes the long form, so it is unaffected.
7c3a45a to
108e06a
Compare
e19f185 to
f73ecf0
Compare
| prevSourceIdRef.current !== undefined && | ||
| prevSourceIdRef.current !== searchedSource?.id | ||
| ) { | ||
| setTraceChartMode('red'); |
There was a problem hiding this comment.
🔵 minor — Every source switch adds ?traceChartMode=red to the URL, even for log sources
The app uses nuqs 1.17.0, where clearOnDefault is off by default. setTraceChartMode('red') therefore writes the default value into the URL each time the source changes, including sources that have no RED view. Call setTraceChartMode(null) to clear the param instead.
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
| histogramTimeChartConfig={ | ||
| histogramTimeChartConfig | ||
| } | ||
| heatmapChartConfig={{ |
There was a problem hiding this comment.
🔵 minor — The inline heatmapChartConfig object makes the heatmap useMemo recompute on every render
heatmapChartConfig={{...chartConfig, dateRange, with: aliasWith}} creates a new object on each DBSearchPage render. TraceRedMetricsChart lists it as a dependency of the toHeatmapChartConfig memo, so the memo never reuses its result, even while in RED mode. Memoize the config in DBSearchPage with useMemo over chartConfig, searchedTimeRange and aliasWith, or build it inside TraceRedMetricsChart from its inputs.
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
| // real id has been seen, so the async source resolution on first load | ||
| // (undefined -> id) doesn't count as a change and a shared link that pins | ||
| // ?traceChartMode=heatmap survives. | ||
| const prevSourceIdRef = useRef<string | undefined>(undefined); |
There was a problem hiding this comment.
🔵 minor — New mode-switch and reset logic has no tests
No test renders TraceRedMetricsChart in either mode. None checks that the heatmap queries carry queryKeyPrefix, which is what lets live tail wait on them, and none covers the source-change reset effect (a first-load undefined→id transition must keep heatmap; a change from one id to another must reset). Add component tests, for example next to components/Search/__tests__/traceRedMetrics.test.ts, that assert the rendered chart and query keys for each mode, plus a hook-level test for the reset effect.
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
| // can't disable live tail for a search that shows the histogram. | ||
| const isHeatmapChartVisible = | ||
| analysisMode === 'results' && | ||
| traceRedMetricsSource != null && |
There was a problem hiding this comment.
🟠 major — Resume live tail button undoes the heatmap's live-tail pin
The effect only calls setIsLive(false) when isHeatmapChartVisible changes. Once it runs, shouldShowLiveModeHint becomes true and ResumeLiveTailButton shows up right next to the new switch (line ~2704). Clicking it, or picking a live range in the time picker (setIsLive(true) at ~2125), turns live back on while the heatmap is still visible, and the effect doesn't run again. Each tick then rebuilds the uPlot instance, which is the problem the comment says this prevents. Fix: hide the resume button and the live option while isHeatmapChartVisible, or leave isLive alone and gate useLiveUpdate's pause on isHeatmapChartVisible.
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
| // can't disable live tail for a search that shows the histogram. | ||
| const isHeatmapChartVisible = | ||
| analysisMode === 'results' && | ||
| traceRedMetricsSource != null && |
There was a problem hiding this comment.
🔵 minor — Switching back to RED leaves live tail off
The pin writes isLive=false into the URL, and nothing restores it. A user who looks at the heatmap for a moment and switches back to RED is left on a frozen time range with live tail off. Pausing live updates through useLiveUpdate's pause while the heatmap is visible, instead of overwriting isLive, would bring live tail back when the user switches to RED.
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
| @@ -90,6 +109,30 @@ export function TraceRedMetricsChart({ | |||
| () => durationConfig(base, durationExpression, durationFormat), | |||
| [base, durationExpression, durationFormat], | |||
| ); | |||
| /** | |||
There was a problem hiding this comment.
🔵 minor — Duration heatmap tile config is copied from DBSearchHeatmapChart and marked with @source
This toHeatmapChartConfig({...cfg, select:[{valueExpression: getDurationMsExpression(source), countExpression:'count()', heatmapScaleType:'log'}], numberFormat: DURATION_HEATMAP_NUMBER_FORMAT}) repeats the defaults in DBSearchHeatmapChart.tsx:52-54,145-158. The conventions reserve @source for whole-file ports across packages, and scripts/ci/ratchet.mjs counts the tag as licensed duplication. For logic shared by two components in the same package, the conventions say to put it in a sibling module. Fix: add a helper, e.g. durationHeatmapConfig(cfg, source) next to getDurationMsExpression in source.ts or in traceRedMetrics.ts, have both callers use it, and remove the @source tag.
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
| @@ -167,5 +210,16 @@ export function TraceRedMetricsChart({ | |||
| </ChartCard> | |||
| </Flex> | |||
| </IsolatedChartSyncProvider> | |||
| ) : ( | |||
There was a problem hiding this comment.
🔵 minor — No tests for the heatmap mode or the source-change reset
The PR adds a mode branch, the heatmap config wiring and the prefixed query keys to TraceRedMetricsChart, plus the ref-guarded source-change reset and the live-tail pin in DBSearchPage. None of it has a unit test. Add tests for: mode='heatmap' renders DBHeatmapChart with queryKeyPrefix and a getDurationMsExpression value; undefined→id keeps heatmap; id A→id B resets to red.
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
The helper was also applied to the log-severity expression, where the legacy status value it matches can never appear and the values severity text does use for errors (fatal, crit and friends) are a different set. Mixing the two vocabularies in one predicate made it wrong in both directions on the log path. Rename it to errorStatusPredicateSql, document it as span-status only, and let the log caller keep its own predicate, unchanged from before this branch.
Trace search showed a single count histogram above the results. For trace sources with a duration column it now shows Throughput, Errors and Duration side by side, under a shared sync scope so hovering one moves the cursor on all three. Each chart is built from the same base config the histogram uses, in a pure module so the aggregations can be unit tested without rendering. Every aggregation is over a raw column so materialized views can satisfy it; the error ratio and the duration unit are applied at display instead. Logs and other sources keep the existing histogram.
Picks up errorStatusPredicateSql from the base branch.
The significant-fields heatmap built { output: 'duration', factor: 0.001 }
inline. The factor is only correct next to getDurationMsExpression, which
applies the source's durationPrecision and leaves the value in ms, so give
it a name in source.ts and document that pairing.
No behavior change.
Adds a RED/Heatmap switch to the trace results chart area. Heatmap renders the duration distribution over time using the same tile the dashboard does. The mode is URL state so a reload or shared link reopens the same view, and it resets to RED when the source changes. DBHeatmapChart takes the queryKeyPrefix prop DBTimeChart already had, so the heatmap queries sit in the same namespace as the RED tiles and live tail waits on them instead of stacking a fresh pair each tick.
The heatmap keys its uPlot instance on the chart config, so a live-tail tick (which moves the absolute time range) tore the chart down and rebuilt it with a recomputed y-axis every 10s, blanking it to Loading in between. Delta and pattern modes already pin live tail off, which is why the delta-view heatmap never hit this. Results mode defaults to live, so the heatmap view opts out for itself while it is the rendered chart.
108e06a to
3c58c91
Compare
ce233e9 to
46b3d9e
Compare
| const isHeatmapChartVisible = | ||
| analysisMode === 'results' && | ||
| traceRedMetricsSource != null && | ||
| traceChartMode === 'heatmap'; |
There was a problem hiding this comment.
🟠 major — Resume Live Tail gets around the heatmap live-tail pin
The pin effect only runs when isHeatmapChartVisible changes. Once live has been pinned off, shouldShowLiveModeHint renders ResumeLiveTailButton right next to the Heatmap switch (line ~2706). Clicking it, or picking a live interval (the setIsLive(true) path at ~2125), turns live back on and the effect never runs again. The heatmap then tears down and rebuilds on every tick, because it keys on JSON.stringify(config) in DBHeatmapChart.tsx:873, which this effect was meant to prevent. Fix: hide or disable ResumeLiveTailButton and the live interval option while isHeatmapChartVisible. Alternatively, have the live-enable handlers leave the heatmap first (for example setTraceChartMode('red')), or pass isLive && !isHeatmapChartVisible to useLiveUpdate.
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
| histogramTimeChartConfig={ | ||
| histogramTimeChartConfig | ||
| } | ||
| heatmapChartConfig={{ |
There was a problem hiding this comment.
🔵 minor — Inline heatmapChartConfig literal means the heatmap useMemo never reuses its result
heatmapChartConfig={{...chartConfig, dateRange: searchedTimeRange, with: aliasWith}} creates a new object on every DBSearchPage render. It is a dependency of the toHeatmapChartConfig useMemo in TraceRedMetricsChart.tsx:314, so that memo recomputes every render and does no work. Fix: build the config once with useMemo in DBSearchPage, keyed on chartConfig, searchedTimeRange and aliasWith, and pass the memoized value. Otherwise drop the useMemo.
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
…3221) ## Summary Fixes the heatmap part of #3205: during a dashboard refresh, heatmap tiles now keep the current heatmap on screen and pulse, instead of replacing it with "Loading...". - Both heatmap queries (the min/max bounds and the buckets) keep their previous result as placeholder data, the same way the time chart and the number, bar and pie tiles do. - The bucket query waits for the refreshed bounds before it runs, so a refresh doesn't also query the new range bucketed with the previous range's min/max. - The heatmap, and the "Not enough data points" message, pulse while either query is still showing previous data. `Heatmap` takes an optional `className` so the pulse goes on its existing container rather than a new wrapper, which would affect its size measurement. - New tests in `DBHeatmapChart.render.test.tsx`: both queries keep their previous data, the tile pulses during a refresh and stops once fresh data loads, and the bucket query waits for fresh bounds. Three of the four fail without the change. While checking the rest of the app for the same pattern, I found two more places: the Kubernetes dashboard's Pods, Nodes and Namespaces tables still switch to skeleton rows on every refresh, and the Services dashboard tiles (`DBListBarChart`, `DBHistogramChart`, the slowest events tile) keep their data but don't pulse. Happy to take those as follow-ups if you'd like. ### How to test on Vercel preview **Preview routes:** /dashboards **Steps:** 1. Create a dashboard with a Heatmap tile on the demo traces source (for example on `Duration`) and a Line time chart. 2. Choose a large time range so each query takes a moment. 3. Click "Refresh dashboard". 4. Verify the heatmap stays on screen and pulses while its queries run, like the line chart, and stops pulsing once the new heatmap appears. ### References - Linear Issue: N/A - Related PRs: #3211, #3212. #3184 also edits the `queryKey` of these two heatmap queries, so whichever lands second needs a small rebase. - Closes part of #3205 Checks run locally: `jest` for the heatmap suites and `ChartPreviewPanel` (50 passed), `eslint` on changed files (0 errors, no new warnings), `prettier --check`, and `tsc --noEmit` for `packages/app`.
3c58c91 to
2c78a82
Compare
Summary
Builds on the RED metrics tiles by adding a RED/Heatmap switch to the trace search results view. Heatmap replaces the three tiles with a duration-distribution heatmap over time, the same tile the dashboard renders (
DBHeatmapChart+toHeatmapChartConfig), without the significant-fields comparison panel.The choice lives in the URL as
traceChartMode, like the other view toggles on this page, so a reload or a shared link reopens the same view. It resets to RED when the source actually changes, so returning to a trace source after viewing another one does not silently reopen in Heatmap; the guard uses a sentinel ref so the async source resolution on first load does not count as a change and a shared?traceChartMode=heatmaplink survives.DBHeatmapChartpicks up thequeryKeyPrefixpropDBTimeChartalready had. The RED tiles namespace their queries so the slow quantile scan stays out of the reported search latency while live tail still waits on them; without the prop the heatmap kept its own keys and slipped past that gate, so a live tick could fire a fresh pair of heatmap queries without waiting for the previous one. Both heatmap queries now carry the prefix.The heatmap value expression comes from
getDurationMsExpression, which applies the source'sdurationPrecision, paired with theDURATION_HEATMAP_NUMBER_FORMATconstant added in #3182. Value, count and scale are fixed here rather than read from the delta view's settings drawer, so this stays a plain duration heatmap independent of that view's query state; the config carries an@sourcenote pointing at the delta-view tile it mirrors.Test plan
tsc --noEmit,eslintwith no new warnings, and the productionbuild:clickhouse.Notes
minorchangeset.main, so its diff stays scoped to the switch plus the heatmap wiring instead of repeating the whole feature. It needs both feat(app): show RED metrics on the trace search results view #3183 and refactor(app): name the duration-heatmap number format #3182, so its branch carries refactor(app): name the duration-heatmap number format #3182's commit too (thesource.tsconstant and theDBSearchHeatmapChartdedupe below). Once feat(app): show RED metrics on the trace search results view #3183 lands I will retarget this tomainand rebase, which drops both of those from the diff. Please do not merge it before feat(app): show RED metrics on the trace search results view #3183.