Repository navigation
feat(app): show RED metrics on the trace search results view - #2826
alex-fedotyev wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: de63586 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 378 passed • 1 skipped • 1106s
Tests ran across 4 shards in parallel. |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
575ea72 to
fc7e432
Compare
Deep Review✅ No critical issues found. This is a well-tested, well-scoped visualization change: the RED aggregations are pure/unit-tested builders, every aggregation is over a raw column (MV-friendly), and the prior Greptile P2 (avg-over-boolean error rate) is fully resolved — rate mode now aggregates 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers (9 dispatched): correctness, testing, maintainability, project-standards, kieran-typescript, performance, julik-frontend-races, adversarial, previous-comments. Only Testing gaps: Consider an assertion tying the duration color-pin aliases and error-rate helper aliases to the config builders (see P3); DBSearchPage integration behavior (source-change reset to RED, live-tail pause on 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 |
fc7e432 to
759003d
Compare
759003d to
ecb2726
Compare
ecb2726 to
a99b07b
Compare
PR Review
6 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 5 minor 6 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
a89c8a8 to
28264c2
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // reopen in Heatmap. Skips the initial mount so a shared link that pins | ||
| // ?traceChartMode=heatmap survives a page load. | ||
| const prevSourceIdRef = useRef(searchedSource?.id); | ||
| useEffect(() => { |
There was a problem hiding this comment.
🟠 major — Source-change reset fires on the first source resolution, so ?traceChartMode=heatmap never survives a load
searchedSource comes from useResolvedSourceParam → useSources() (a react-query fetch), so on a fresh page load the first render has searchedSource === undefined and prevSourceIdRef is seeded with undefined. When the sources arrive the id changes from undefined → the real id and the effect runs setTraceChartMode('red'), discarding the heatmap value the shared link carried — the exact case the comment says it skips. It also writes traceChartMode=red into the URL of every search page (nuqs 1.17 does not clear-on-default), including log sources where the param is meaningless. Seed the ref with a sentinel and only reset when the previous id was defined, e.g. const prevSourceIdRef = useRef<string | null | undefined>(undefined) plus if (prevSourceIdRef.current !== undefined && prevSourceIdRef.current !== searchedSource?.id) setTraceChartMode('red'), always assigning the ref.
| ({ x, y, payload, index, visibleTicksCount }: XAxisTickContentProps) => { | ||
| const textAnchor = compactTickAnchor(index, visibleTicksCount); | ||
| return ( | ||
| <Text |
There was a problem hiding this comment.
🟠 major — Compact x-axis tick drops the axis-supplied fill, so RED tick labels render in the SVG default color
Recharts passes the tick renderer fill (derived from the axis stroke, default #666) along with x/y/payload; this function destructures only four props and renders a bare <Text>, so the label gets no fill and falls back to the SVG default black — unreadable on the dark theme's --color-bg-body, and inconsistent with the y-axis ticks on the same tile. It also loses the recharts-cartesian-axis-tick-value class the default path sets (used as a selector in tests/page objects). Set the color explicitly, as the repo's other custom tick does (TruncatedTick, packages/app/src/components/PropertyComparisonChart.tsx:131 uses fill="var(--color-text-muted)"), or accept the full props object and spread it onto <Text> before overriding textAnchor.
| * misconfigured source drops the Errors tile rather than emitting | ||
| * `lower( ) = 'error'`). Mirrors the service dashboard's | ||
| * `lower(StatusCode) = 'error'` definition. */ | ||
| export function errorConditionSql( |
There was a problem hiding this comment.
🟠 major — Errors tile reads zero for trace sources whose StatusCode is STATUS_CODE_ERROR
lower(<status>) = 'error' matches Error/ERROR but not the legacy OTel collector value STATUS_CODE_ERROR — the very value this repo's own e2e fixture writes (packages/app/tests/e2e/seed-clickhouse.ts:357) and that other app code explicitly handles (packages/app/src/components/aiSummarize/logic.ts:83; the TODO at packages/app/src/components/DBTraceWaterfallChart.tsx:1084). On such a source the Errors tile silently shows a flat 0% rate and an empty volume chart, a regression from the replaced histogram, which grouped by StatusCode and therefore did surface those spans. Widen the shared predicate in errorPredicateSql (packages/app/src/source.ts:491) to also accept the STATUS_CODE_ERROR form, e.g. lower(x) IN ('error', 'status_code_error').
|
|
||
| // One decimal so sub-1% rates read (e.g. 0.4%) instead of rounding to 0%, | ||
| // while the axis still auto-scales to the data. | ||
| export const ERROR_RATE_FORMAT: NumberFormat = { |
There was a problem hiding this comment.
🔵 minor — ERROR_RATE_FORMAT duplicates the app-wide error-rate percent format
ERROR_RATE_PERCENTAGE_NUMBER_FORMAT (packages/app/src/ChartUtils.tsx:425) is the existing {output: 'percent'} format for error-rate charts, used by HttpTab.tsx:240, ServiceDashboardEndpointSidePanel.tsx:144 and the LLM dashboards — this file already imports INTEGER_NUMBER_FORMAT from the same module. Import it instead; if one decimal is genuinely wanted, change mantissa there so every error-rate chart stays consistent.
| // source that exposes a duration column; otherwise the single histogram | ||
| // stays. Derived once, as the narrowed source or null, so the stats-row | ||
| // toggle and the chart branch can't drift apart on which one renders. | ||
| const traceRedMetricsSource = |
There was a problem hiding this comment.
🔵 minor — traceRedMetricsSource restates the predicate already computed for showDelta
showDelta at DBSearchPage.tsx:2471 computes the same "trace source with a durationExpression" test inline; the two must now be kept in sync by hand. Pass showDelta={traceRedMetricsSource != null} and drop the inline ternary.
| // auto-scaled y-axis) and clamp to 100%, since an error rate cannot | ||
| // exceed 1 by definition. | ||
| alias: 'Error rate', | ||
| valueExpression: `least(if(${TOTAL_SPANS_ALIAS} > 0, ${ERROR_SPANS_ALIAS} / ${TOTAL_SPANS_ALIAS}, 0), 1)`, |
There was a problem hiding this comment.
🔵 minor — least(..., 1) clamp in the error-rate expression is unreachable
error_spans is countIf(<error cond>) over the same rows as total_spans = count(), so the ratio can never exceed 1 (the sample-weight rendering scales both counts identically). Drop the least(...) wrapper and its test case, keeping only the if(total_spans > 0, ...) zero-division guard.
28264c2 to
54a597b
Compare
| showDisplaySwitcher: false, | ||
| showMVOptimizationIndicator: false, | ||
| showDateRangeIndicator: false, | ||
| queryKeyPrefix, |
There was a problem hiding this comment.
🟠 major — RED charts share QUERY_KEY_PREFIX, so live tail now stalls on the p95/p99 query and the search executed metric measures it
commonProps forwards queryKeyPrefix (= 'search', DBSearchPage.tsx:207) to all three tiles. DBSearchPage.tsx:1588 computes isAnyQueryFetching = useIsFetching({queryKey: [QUERY_KEY_PREFIX]}) > 0, which feeds useLiveUpdate({pause: isAnyQueryFetching, ...}) (DBSearchPage.tsx:1634 — the interval is only started when !pause, see useLiveUpdate at line 751) and useSearchTelemetry (line 1593), whose latency_ms is emitted as search executed. For a trace source the count histogram is replaced by three queries, one of them avg + quantile(0.95) + quantile(0.99) over the duration column; live tail on trace sources now refreshes only after the slowest of them settles, and the reported search latency becomes the RED-tile latency rather than the results query's. Give the RED tiles their own prefix (e.g. 'search-red') so they stay out of the live-tail/telemetry gate, or pass queryKeyPrefix only to the tile that stands in for the histogram.
| verticalAnchor="start" | ||
| fontSize={11} | ||
| fontFamily="IBM Plex Mono, monospace" | ||
| fill="var(--color-text-muted)" |
There was a problem hiding this comment.
🔵 minor — Compact x-axis ticks hardcode --color-text-muted, so the X labels no longer match the Y labels on the same chart
renderCompactXTick sets fill="var(--color-text-muted)", but the Y axis (line 1743) and the non-compact X axis (line 1736) pass only {fontSize, fontFamily} and inherit recharts' default tick color; there is no global .recharts-cartesian-axis-tick-value override in packages/app/styles. On the RED tiles the time labels will therefore render in a different color from the value labels beside them (most visibly in the light theme, where --color-text-muted is --mantine-color-dark-6). This also breaks the documented chart-color rule ("Don't hard-code series colors or reuse --color-text-* for charts"). Drop the fill so the compact ticks inherit the same color as every other axis, or move the token onto both axes at once.
| // derived from the source's durationPrecision, convert the unit. Falls back | ||
| // to getDurationMsExpression only for the heatmap tile below. | ||
| const durationExpression = source.durationExpression ?? ''; | ||
| const durationFormat = getTraceDurationNumberFormat(source, { |
There was a problem hiding this comment.
🔵 minor — durationFormat and the inline heatmapChartConfig are new objects every render, so both useMemos never hit
getTraceDurationNumberFormat returns a fresh object literal (source.ts:555), so durationFormat changes identity on every render and the duration memo at line 100 always recomputes, handing DBTimeChart (a memo) a new config each time — which then rebuilds queriedConfig and the number-format maps and re-renders the whole recharts tree. The same applies to heatmapChartConfig, constructed inline in JSX at DBSearchPage.tsx:2640. Since the search page re-renders on every keystroke and live-tail tick, wrap the format in useMemo here and memoize the heatmap config at the callsite.
|
|
||
| // One decimal so sub-1% rates read (e.g. 0.4%) instead of rounding to 0%, | ||
| // while the axis still auto-scales to the data. | ||
| export const ERROR_RATE_FORMAT: NumberFormat = { |
There was a problem hiding this comment.
🔵 minor — ERROR_RATE_FORMAT restates the existing ERROR_RATE_PERCENTAGE_NUMBER_FORMAT
ChartUtils.tsx:425 already exports ERROR_RATE_PERCENTAGE_NUMBER_FORMAT: { output: 'percent', mantissa: 0 }, used by the Services and LLM dashboards for exactly this metric; this file (which already imports INTEGER_NUMBER_FORMAT from the same module) declares a second copy that differs only in mantissa. Use { ...ERROR_RATE_PERCENTAGE_NUMBER_FORMAT, mantissa: 1 }, or add the extra digit to the shared constant so all error-rate charts read the same.
| * collector form ('STATUS_CODE_ERROR'); severity-text values never take the | ||
| * latter, so it's a no-op there. | ||
| */ | ||
| export function errorPredicateSql(statusOrSeverityExpression: string): string { |
There was a problem hiding this comment.
🔵 minor — errorPredicateSql silently widens the error definition for the Services and LLM dashboards, outside this PR's stated scope
Switching lower(x) = 'error' to lower(x) IN ('error', 'status_code_error') is applied at serviceDashboard.ts:190 and llm/lib/expressions.ts:367,397, so every existing Request Error Rate / error-count tile on the Services dashboard and the LLM dashboards will start counting legacy STATUS_CODE_ERROR spans — a numbers change users will notice on pages this PR otherwise doesn't touch. The changeset only mentions trace-search RED metrics. Either call it out in .changeset/red-metrics-trace-search.md, or land the shared predicate as its own change.
| // chosen view; the switch lives inline in the search stats row. | ||
| const [traceChartMode, setTraceChartMode] = useQueryState( | ||
| 'traceChartMode', | ||
| parseAsStringEnum<'red' | 'heatmap'>(['red', 'heatmap']).withDefault('red'), |
There was a problem hiding this comment.
🔵 minor — The RED/Heatmap mode union is restated instead of using the exported TraceChartMode
TraceRedMetricsChart.tsx:29 exports TraceChartMode = 'red' | 'heatmap' but nothing imports it; the same two literals are written again in parseAsStringEnum<'red' | 'heatmap'>(['red', 'heatmap']) here, in the onChange narrowing at line 2599, and in the SegmentedControl data at line 2603. Import TraceChartMode and type the query state with it (parseAsStringEnum<TraceChartMode>) so adding a third mode is a single edit.
| ): BuilderChartConfigWithDateRange | undefined { | ||
| const groupBy = statusCodeExpression?.trim(); | ||
| const errorCondition = errorConditionSql(groupBy); | ||
| if (!groupBy || errorCondition == null) { |
There was a problem hiding this comment.
🔵 minor — errorCondition == null in the errorsConfig guard is unreachable
errorConditionSql returns undefined only when its (already-trimmed) argument is falsy, and it is called here with the same trimmed groupBy, so errorCondition == null is exactly !groupBy. Drop the second clause: if (!groupBy) return undefined; — and errorCondition is then known to be a string, removing the need for the reader to check.
54a597b to
c75de5f
Compare
| * caller passes a duration NumberFormat derived from the source's precision | ||
| * (via getTraceDurationNumberFormat) so unit conversion happens at display. | ||
| */ | ||
| export function durationConfig( |
There was a problem hiding this comment.
🟠 major — Duration tile renders 0 ms for buckets with no spans, so latency lines dip to zero
durationConfig inherits fillNulls from the search config, which never sets it; shouldFillNullsWithZero (packages/app/src/ChartUtils.tsx:1456) treats undefined as "fill", and formatResponseForTimeChart writes 0 into every generated empty bucket (ChartUtils.tsx:959-981). SQL returns no row for a bucket with no matching spans, so on any sparse trace search (e.g. filtered to one endpoint, or a 30-day range with gaps) the Avg/p95/p99 lines drop to 0 ms between real points and the user reads it as "latency collapsed". Set fillNulls: false on the config returned by durationConfig so gaps render as breaks; count-based tiles (Throughput, Errors volume) can keep the zero fill.
| @@ -2556,6 +2599,21 @@ export function DBSearchPage() { | |||
| enableParallelQueries | |||
| /> | |||
| <Group gap="sm" align="center"> | |||
| {traceRedMetricsSource != null && ( | |||
| <SegmentedControl | |||
There was a problem hiding this comment.
🔵 minor — RED/Heatmap toggle stays visible when the chart area is suppressed by a query error
The SegmentedControl is inside the stats row, which renders whenever chartConfig && histogramTimeChartConfig, while the chart itself is gated on !hasQueryError at line 2637. Type an invalid WHERE clause on a trace source and the toggle renders above the error panel with nothing to switch. Gate it on !hasQueryError as well.
| * collector form ('STATUS_CODE_ERROR'); severity-text values never take the | ||
| * latter, so it's a no-op there. | ||
| */ | ||
| export function errorPredicateSql(statusOrSeverityExpression: string): string { |
There was a problem hiding this comment.
🔵 minor — errorPredicateSql claims to be the single error definition but the service map still hardcodes = 'Error'
useServiceMap.tsx:178 (countIf(ServerSpans.statusCode = 'Error')) and ServiceMapTooltip.tsx:80 (the "Errors" drill-down ? = 'Error') were not migrated, so after this change the Services dashboard error rate and the Service Map error count report different numbers for the same service whenever spans carry the legacy STATUS_CODE_ERROR value. Route both through errorPredicateSql (the tooltip's link needs the raw status expression, which it already has as source.statusCodeExpression).
| * filter keeps the tile errors-only rather than reintroducing the healthy | ||
| * spans. | ||
| */ | ||
| export function errorsConfig( |
There was a problem hiding this comment.
🔵 minor — errorsConfig rate mode duplicates the existing Request Error Rate chart builder
requestErrorRateConfig in packages/app/src/ServicesDashboardPage/HttpTab.tsx:210-243 already builds the identical MV-friendly shape (count aliased, count with aggCondition: expressions.isError, then a post-aggregation error_requests / total_requests column with ERROR_RATE_PERCENTAGE_NUMBER_FORMAT), and HttpTab.tsx:419 hides the helper columns with the same literal list that ERROR_RATE_HELPER_SERIES now re-declares. Export the builder from traceRedMetrics.ts (parameterised on the error predicate) and have HttpTab consume it, so the aliases and hidden-series list live in one place.
| verticalAnchor="start" | ||
| fontSize={11} | ||
| fontFamily="IBM Plex Mono, monospace" | ||
| fill="var(--color-text-muted)" |
There was a problem hiding this comment.
🔵 minor — Compact x-axis ticks get an explicit token fill while the y-axis ticks keep recharts' default
renderCompactXTick hardcodes fill="var(--color-text-muted)", but the YAxis tick object on line 1743 (and the non-compact XAxis tick on line 1736) passes only fontSize/fontFamily and inherits recharts' default tick colour. On the RED tiles the x-axis labels therefore render in a different colour from the y-axis labels of the same chart, and from every other chart's x-axis in the app. Either drop the fill here or apply the same token to both axis tick definitions.
| // auto-scaled y-axis). error_spans is a countIf over the same rows as | ||
| // total_spans = count(), so the ratio can't exceed 1; no clamp needed. | ||
| alias: 'Error rate', | ||
| valueExpression: `if(${TOTAL_SPANS_ALIAS} > 0, ${ERROR_SPANS_ALIAS} / ${TOTAL_SPANS_ALIAS}, 0)`, |
There was a problem hiding this comment.
🔵 minor — if(total_spans > 0, ...) guards a state the query cannot produce, and its comment is wrong
renderChartConfig emits no WITH FILL (see the removal note at packages/common-utils/src/core/renderChartConfig.ts:3070), so every returned bucket row corresponds to at least one source row and total_spans = count() is always >= 1 (sample-weighted counts use greatest(..., 1), renderChartConfig.ts:597). Empty buckets are synthesised client-side as 0 by formatResponseForTimeChart, never as 0/0. Either drop the if and divide directly (as HttpTab.tsx:236 does) or fix the comment so a future reader doesn't believe SQL returns empty buckets.
| timestampValueExpression: 'Timestamp', | ||
| select: [{ aggFn: 'count', aggCondition: '', valueExpression: '' }], | ||
| where: "ServiceName = 'api'", | ||
| whereLanguage: 'sql', |
There was a problem hiding this comment.
🔵 minor — Volume-mode test cannot catch dropping the caller's active filters
The base fixture sets filters: [], so expect(result?.filters).toEqual([{ type: 'sql', condition: ERROR_COND }]) passes identically whether errorsConfig spreads base.filters or replaces them — yet in production chartConfig.filters carries the user's active search filters, and dropping them would silently widen the Errors tile beyond the current search. Give the fixture a non-empty filters entry and assert it is preserved ahead of the error condition.
c75de5f to
203f197
Compare
| export function redBaseConfig( | ||
| histogramTimeChartConfig: BuilderChartConfigWithDateRange, | ||
| ): BuilderChartConfigWithDateRange { | ||
| return { ...histogramTimeChartConfig, groupBy: undefined }; |
There was a problem hiding this comment.
🟠 major — Default trace RED view drops the per-status drill-down the histogram provided
redBaseConfig strips the status groupBy from every tile, and TraceRedMetricsChart only wires onFocusSeries for the volume Errors mode (TraceRedMetricsChart.tsx:198), which is not the default (errorsMode starts at 'rate'). The old chart was histogramTimeChartConfig with groupBy: statusCodeExpression plus onFocusSeries={handleFocusSeries} (DBSearchPage.tsx:2666-2678), so clicking a bar pinned a tooltip with a per-status breakdown, per-status "view events" links, and a Focus action that called searchFilters.setOnlyFilters. On the new default view none of the three tiles is grouped and none receives onFocusSeries, so Focus silently degrades to chart-only visual isolation and the status breakdown is gone. Keep groupBy on the Throughput tile (it is the old histogram, stacked by status) and pass onFocusSeries to it; that also stops Throughput from issuing a second count-over-the-same-rows query alongside the grouped count query SearchTotalCountChart still runs for the results count.
| // Pin the Duration percentiles to stable, cool palette hues so latency never | ||
| // borrows error red (or warning orange / success green), and so each percentile | ||
| // keeps its color across sessions regardless of how the data sorts the lines. | ||
| const DURATION_SERIES_COLORS = { |
There was a problem hiding this comment.
🔵 minor — Duration colour pins are keyed by string literals that duplicate the select aliases
DURATION_SERIES_COLORS keys 'Avg' | 'p95' | 'p99' must match the alias values in durationConfig (traceRedMetrics.ts:158-178) exactly, because setLineColors looks the pin up by result-column name. Renaming an alias silently drops the pin and p99 falls back to the positional palette slot COLORS[2] — chart-red — which is the exact collision the pins exist to prevent, with no test or type failure. traceRedMetrics.ts already solves this for the error-rate helpers via TOTAL_SPANS_ALIAS/ERROR_SPANS_ALIAS; export the three duration aliases the same way and key the colour map off them.
| return undefined; | ||
| } | ||
| const errorCondition = errorPredicateSql(groupBy); | ||
| if (mode === 'rate') { |
There was a problem hiding this comment.
🔵 minor — Error-rate and duration chart builders re-implement existing inline configs
The rate branch (count + countIf + post-aggregation ratio, with the two helper aliases hidden via hiddenSeries) is the same construction already inline in ServiceDashboardEndpointSidePanel.tsx:114-147 ("Request Error Rate", hiddenSeries={['total_count','error_count']}) and HttpTab.tsx:214-243 (requestErrorRateConfig); durationConfig mirrors EndpointLatencyChart in HttpTab.tsx:96-119. This new pure module is the right home for them, but it leaves three copies of the same MV-friendly pattern that must now be kept in sync by hand. Have those two call sites build their configs from errorsConfig/durationConfig instead of restating them.
| // Heatmap tile config (duration distribution over time), matching the | ||
| // dashboard heatmap tile: DBHeatmapChart + toHeatmapChartConfig, no | ||
| // significant-fields comparison panel. | ||
| const { heatmapConfig, scaleType } = useMemo( |
There was a problem hiding this comment.
🔵 minor — Results-mode heatmap duplicates DBSearchHeatmapChart and ignores its persisted settings
This toHeatmapChartConfig({... select: [{valueExpression: getDurationMsExpression(source), countExpression: 'count()', heatmapScaleType: 'log'}], numberFormat: DURATION_HEATMAP_NUMBER_FORMAT}) is a copy of DBSearchHeatmapChart.tsx:145-158 with the URL state hardcoded. value, count and scaleType are page-level query params written by the delta view's settings drawer, so a user who sets ?scaleType=linear (or a non-duration value expression) sees it honoured in Delta mode and silently ignored in Results → Heatmap on the same URL. Read the same useQueryStates fields here, or extract the tile from DBSearchHeatmapChart and share it.
| // the `search executed` telemetry (both match on [QUERY_KEY_PREFIX]). Otherwise | ||
| // the slow avg/p95/p99 duration query would delay every live-tail refresh on | ||
| // trace sources and become what the reported search latency measures. | ||
| const RED_QUERY_KEY_PREFIX = `${QUERY_KEY_PREFIX}-red`; |
There was a problem hiding this comment.
🔵 minor — RED queries bypass the live-tail pause gate, so live ticks can stack them
useLiveUpdate pauses on isAnyQueryFetching, which is useIsFetching({queryKey: [QUERY_KEY_PREFIX]}) (line 1597) and therefore never matches search-red. With the default 10s refreshFrequency, every tick advances searchedTimeRange, changing the RED query keys and firing three fresh queries — including the avg/p95/p99 quantile scan the comment itself calls slow — without waiting for the previous ones, so on a large trace table those accumulate unbounded while live tail runs. If the goal is only to keep RED out of the reported search latency, split the two consumers: keep useSearchTelemetry on the search-only count and give useLiveUpdate a pause signal that also counts [RED_QUERY_KEY_PREFIX].
| @@ -152,6 +152,42 @@ describe('ChartUtils', () => { | |||
| ]); | |||
| }); | |||
|
|
|||
| it('pins a series to a palette token via seriesColors, overriding the positional palette', () => { | |||
There was a problem hiding this comment.
🔵 minor — seriesColors test never exercises the documented positional-slot behaviour
The new test uses a single-series response, so it only proves the pin wins over COLORS[0]. The subtle half of the change — "a pinned series does not consume a positional slot, so unpinned series keep their natural palette order" (ChartUtils.tsx:596-599) — is untested: moving colorIndex++ out of the else branch would still pass. Add a case with two series where only the first is pinned and assert the unpinned one gets COLORS[0], not COLORS[1].
203f197 to
b98f701
Compare
| durationExpression: string, | ||
| numberFormat: NumberFormat | undefined, | ||
| ): BuilderChartConfigWithDateRange { | ||
| return { |
There was a problem hiding this comment.
🟠 major — Duration chart plots Avg/p95/p99 as 0 in buckets with no spans
durationConfig inherits fillNulls from the search config, which is unset, so shouldFillNullsWithZero returns true (ChartUtils.tsx:1479) and formatResponseForTimeChart writes 0 into every generated empty bucket for every series (ChartUtils.tsx:971-1000). ClickHouse does no gap filling here — WITH FILL is commented out (renderChartConfig.ts:3070-3097) — so any bucket with no matching spans (common on a filtered trace search over a wide range) comes back missing and the latency lines drop to zero instead of showing a gap, reading as "0ms p99" rather than "no traffic". Set fillNulls: false in durationConfig's returned config (the <Area connectNulls> at HDXMultiSeriesTimeChart.tsx:1056 will bridge the gap); Throughput/Errors are counts and should keep the 0 fill.
| </IsolatedChartSyncProvider> | ||
| ) : ( | ||
| <ChartCard style={{ height: '100%', minHeight: 0 }}> | ||
| <DBHeatmapChart |
There was a problem hiding this comment.
🟠 major — Heatmap mode bypasses the live-tail backpressure the RED prefix was added for
DBHeatmapChart keys its two queries as ['heatmap', minMaxConfig] and ['heatmap_bucket', bucketConfig] (DBHeatmapChart.tsx:761 and :785) and accepts no queryKeyPrefix, so useIsFetching({queryKey: [RED_QUERY_KEY_PREFIX]}) in DBSearchPage.tsx:1608 resolves to 0 whenever traceChartMode === 'heatmap'. With live tail on and the Heatmap view selected, every refresh tick fires a fresh quantile(0.01)/max(Duration) bounds scan plus a bucket scan without waiting for the previous one — exactly the stacking the new prefix was introduced to prevent for the RED tiles. Either thread a queryKeyPrefix through HeatmapContainer into both useQueriedChartConfig calls and pass RED_QUERY_KEY_PREFIX here, or gate enabled on the same signal.
| * latter, so it's a no-op there. | ||
| */ | ||
| export function errorPredicateSql(statusOrSeverityExpression: string): string { | ||
| return `lower(${statusOrSeverityExpression}) IN ('error', 'status_code_error')`; |
There was a problem hiding this comment.
🔵 minor — Legacy STATUS_CODE_ERROR spans render as info-blue on the new Errors tile
The widened predicate now pulls STATUS_CODE_ERROR rows into the Errors volume chart, which is grouped by StatusCode, so firstGroupColumnIsLogLevel applies logLevelColor per status (ChartUtils.tsx:730). getLogLevelClass (utils.ts:332-355) only matches prefixes err/crit/fatal/alert/emerg, so 'status_code_error' falls through to the info branch and that series is drawn in chart-info blue on an errors-only tile, next to a red Error series. Add the legacy form to getLogLevelClass (or match includes('err')) so the color classifier and errorPredicateSql agree on what an error is.
| // Memoized: getTraceDurationNumberFormat returns a fresh object, so an | ||
| // unmemoized value would change identity every render and defeat the | ||
| // `duration` memo below (re-rendering DBTimeChart on every keystroke/tick). | ||
| const durationFormat = useMemo( |
There was a problem hiding this comment.
🔵 minor — durationFormat is recomputed here but DBTimeChart already derives the identical format
useChartNumberFormats calls getTraceDurationNumberFormat(source, series) for every select entry (source.ts:787) using config.source, which buildSearchChartConfig always populates (searchChartConfig.ts:161) and convertToTimeChartConfig preserves. durationConfig builds selects whose valueExpression is exactly source.durationExpression with avg/quantile, both in DURATION_PRESERVING_AGG_FNS (source.ts:504-520), so the inferred per-column and chart-wide formats are already {output:'duration', factor:10^-precision} — byte-identical to the memoized durationFormat. Drop the durationFormat memo and durationConfig's third parameter.
|
|
||
| function setLineColors( | ||
| sortedLineData: LineDataWithOptionalColor[], | ||
| seriesColors?: Record<string, ChartPaletteToken>, |
There was a problem hiding this comment.
🔵 minor — seriesColors adds a second per-series color mechanism alongside select[i].color
ChartConfigSchema already carries color: ChartPaletteTokenSchema.optional() on each select item (common-utils types.ts:416), consumed positionally against meta[i].name in DBTableChart.tsx:149-175, and types.ts:1502-1503 states line/bar/pie per-series colors are meant to ship through that same field. Keying a side map on displayName instead means the pin lives apart from the alias that defines it — the hazard traceRedMetrics.ts:146-151 documents rather than removes. Resolve pins from select[i].color in formatResponseForTimeChart (same positional meta mapping useChartNumberFormats already uses) and set color: 'chart-cyan' on the p95 select entry instead of adding the seriesColors prop.
| /** | ||
| * The single definition of an "error" span/log as a SQL predicate over a | ||
| * status-code or severity-text expression. Shared by the service dashboard, | ||
| * the LLM expressions, and trace search so the definition lives in one place. |
There was a problem hiding this comment.
🔵 minor — "Single definition" of an error span leaves the service map on the old predicate
Two other call sites still spell out the error condition themselves: countIf(ServerSpans.statusCode = 'Error') in hooks/useServiceMap.tsx:178 and ? = 'Error' in components/ServiceMap/ServiceMapTooltip.tsx:80. Both are case-sensitive exact matches, so after this change the same trace data yields a lower error count on the service map than on the services dashboard for any collector emitting STATUS_CODE_ERROR. Route those two through errorPredicateSql as well, or narrow the docblock's claim.
| verticalAnchor="start" | ||
| fontSize={11} | ||
| fontFamily="IBM Plex Mono, monospace" | ||
| fill="var(--color-text-muted)" |
There was a problem hiding this comment.
🔵 minor — Compact x-axis ticks use a different color from the y-axis ticks on the same chart
renderCompactXTick hardcodes fill="var(--color-text-muted)", but the YAxis right below (line 1743) keeps tick={{fontSize: 11, fontFamily}} with no fill, so it falls back to recharts' #666. On the RED tiles the x labels are visibly brighter than the y labels in dark mode (--color-text-muted is dark-2, #A6A7AB) and darker in light mode (dark-6). Apply the same fill to the YAxis tick object (or drop it from the compact tick) so both axes match.
| }); | ||
|
|
||
| describe('errorsConfig', () => { | ||
| it('rate: count + countIf aggregated separately, divided post-aggregation (MV-friendly)', () => { |
There was a problem hiding this comment.
🔵 minor — Error-rate test asserts the config object, never that the query renders
Every assertion in this file compares the builder's output to a literal restating the builder, so the one genuinely novel construct — a select entry with no aggFn whose valueExpression references two sibling select aliases (if(total_spans > 0, error_spans / total_spans, 0)) inside a GROUP BY __hdx_time_bucket query — is never exercised through renderChartConfig. A regression in how aliases are emitted (they render as AS "total_spans", renderChartConfig.ts:826-828) or in the aggFn-less branch at renderChartConfig.ts:781 would leave all these tests green and produce a broken ClickHouse query at runtime. Add one snapshot/render assertion for the rate config alongside the existing renderChartConfig tests.
For a trace source in results mode, replace the single count histogram above the results with a Throughput / Errors / Duration trio: - Throughput counts spans (bars). Duration shows Avg / p95 / p99 over the raw Duration column, with the unit applied at display from the source precision. - Errors toggles between rate and volume. Rate is countIf/count as a percent line, capped at 100%. Volume groups error spans by status and renders them as bars, so clicking a bar filters the results to that status; this keeps the per-status drill-down the count histogram had. - The three charts are DBTimeCharts under a shared sync scope (synced hover cursor) and use the dashboard tile card header. A RED/Heatmap switch in the stats row flips to the same heatmap tile the dashboard renders, and resets to RED when the source changes. - Aggregations are over raw columns (count, countIf, quantile/avg) so materialized views can satisfy them; ratio and unit conversion happen at the display layer. Aggregation builders live in a pure, unit-tested module. - Adds an opt-in compactXAxisLabels (edge-anchor first/last x labels) and yAxisMaxDomain (cap the y-axis while auto-scaling below it) to the shared time chart, both covered by focused unit tests. Logs and session sources keep the existing histogram. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b98f701 to
de63586
Compare
| </Box> | ||
| )} | ||
| {!hasQueryError && | ||
| (traceRedMetricsSource != null ? ( |
There was a problem hiding this comment.
🟠 major — Trace searches now run the status-grouped count histogram with nothing rendering it, plus a second ungrouped count for Throughput
SearchResultsCountGroup (DBSearchPage.tsx:2661) still derives "N Results" from histogramTimeChartConfig under QUERY_KEY_PREFIX, and SearchTotalCountChart deliberately mirrors DBTimeChart's config/queryKey so the two dedupe into one query (see the comment at components/SearchTotalCountChart.tsx:48 and the guard test __tests__/DBSearchPageQueryKey.test.tsx). For a trace source that chart is no longer rendered, so the grouped count() GROUP BY StatusCode, bucket scan now runs purely to produce the total, while throughputConfig runs a near-identical ungrouped count() over the same rows under search-red — two count scans per search and per ~10s live-tail tick where there used to be one. Pass the throughput config to SearchResultsCountGroup and render the Throughput tile under QUERY_KEY_PREFIX (only Errors/Duration need the separate prefix) so the count and the visible chart share a key again.
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 — heatmapChartConfig is a fresh object literal every render, so the heatmap memo in TraceRedMetricsChart never hits
heatmapChartConfig={{...chartConfig, dateRange: searchedTimeRange, with: aliasWith}} is rebuilt on every render of DBSearchPage, so useMemo(..., [heatmapChartConfig, durationMsExpression]) at TraceRedMetricsChart.tsx:121 re-runs toHeatmapChartConfig and hands DBHeatmapChart a new config object each time — the same identity trap the file already guards against for durationFormat (TraceRedMetricsChart.tsx:86-96). Memoize the literal in DBSearchPage the way filtersChartConfig (DBSearchPage.tsx:2035) already is, and pass the memoized value.
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
| from the initial config when uncontrolled, so a key swap | ||
| re-seeds bars (volume) vs line (rate). */} | ||
| <DBTimeChart | ||
| key={errorsMode} |
There was a problem hiding this comment.
🔵 minor — Rate/Vol toggle remounts the whole Errors chart, including the SegmentedControl that was just clicked
key={errorsMode} unmounts and remounts DBTimeChart on every toggle, which also destroys and recreates errorsModeControl inside its own toolbar — keyboard focus on the radio group is lost after each arrow-key/Enter change, and the chart re-seeds all its local state. DBTimeChart already supports a controlled display type: when a setDisplayType prop is supplied it follows config.displayType instead of its mount-time seed (components/DBTimeChart.tsx:657-666), so pass setDisplayType and drop the key.
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
| // (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 — Source-change reset clobbers traceChartMode on back/forward navigation
The effect compares only searchedSource?.id, so a history pop back to a URL that pins ?traceChartMode=heatmap (e.g. saved search A on source S1 → saved search B on source S2 → Back) is seen as a source change and rewritten to red, discarding the mode the restored URL carried. The existing source-change effect at DBSearchPage.tsx:1410-1476 already distinguishes a user-initiated switch from the form catching up to the config; gate the reset on the same signal rather than on a bare id comparison.
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
| return undefined; | ||
| } | ||
| const errorCondition = errorPredicateSql(groupBy); | ||
| if (mode === 'rate') { |
There was a problem hiding this comment.
🔵 minor — The error-rate select shape is a fourth copy of a pattern already in the services and LLM dashboards
errorsConfig(..., 'rate') rebuilds the exact count + countIf + post-aggregation-ratio + hidden-helper-series shape that ServicesDashboardPage/HttpTab.tsx:221-239 (total_requests/error_requests/error_rate, hiddenSeries at line 419), llm/dashboard/OverviewCharts.tsx:210 and components/ServiceDashboardEndpointSidePanel.tsx:114 already build inline, each with its own alias names. Since this PR is the first to extract it into a pure builder, export it in a form those call sites can adopt (alias names parameterised, helper-series list derived) rather than adding a fourth private variant.
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
| * so only it surfaces this. */ | ||
| onFocusSeries?: (filters: SeriesGroupFilter[]) => void; | ||
| }) { | ||
| const [errorsMode, setErrorsMode] = useState<ErrorsMode>('rate'); |
There was a problem hiding this comment.
🔵 minor — No test covers the component's mode branching, only the pure builders
traceRedMetrics.test.ts covers the config builders and ChartUtils.test.ts the color pin, but nothing exercises TraceRedMetricsChart itself: that rate hides ERROR_RATE_HELPER_SERIES and caps the axis at 1 while volume wires onFocusSeries and drops the cap, that the Errors tile disappears without a statusCodeExpression, or that mode === 'heatmap' swaps in the heatmap tile. A render test asserting the props handed to a mocked DBTimeChart per mode would catch a regression in any of those; the deferred e2e (#3176) does not cover this layer.
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
|
Closing this in favour of a stack of four smaller PRs. Same code, reviewed in pieces:
Each targets the previous branch, so each diff shows only its own changes. Reviewing from #3181 up means the two small ones can land immediately and the feature no longer arrives mixed in with unrelated predicate and formatting changes. Splitting it also resolved the review comments that were still open here. The "fix the other call sites" findings on Follow-ups stay open: #3176 for e2e coverage and #3177 for an "add to dashboard" action. |
Summary
Trace search shows a single count histogram above the results. For trace sources in results mode, this replaces it with RED metrics:
countIf(error) / count(), rendered as a percent line) and volume (countIf(error), bars).Every aggregation is over a raw column (
count,countIf,quantile/avgof the duration expression), so AggregatingMergeTree materialized views can satisfy the queries; ratio and unit conversion happen at the display layer or in a post-aggregation column.The three charts are
DBTimeCharts under a shared sync scope, so hovering one shows a synced cursor on all three at the same timestamp, and each renders as a dashboard-tile card. A RED/Heatmap switch in the search stats row flips the area to the same duration heatmap tile the dashboard renders.Each chart is built from the same base config the histogram uses (in a small pure module,
traceRedMetrics.ts), so they honor the active WHERE filter and selected time range. Logs and session sources keep the existing histogram.The Duration percentiles are pinned to cool palette hues (Avg blue, p95 cyan, p99 purple) so latency never borrows the red the Errors line uses, and each percentile keeps its color across sessions regardless of how the data sorts the lines.
These charts are auxiliary to the results query, so they run under their own query namespace: that keeps the slow quantile scan out of the reported search latency, while live tail still waits on them before advancing the window, so a tick can't stack a second set of chart queries on top of the first.
DBHeatmapCharttakes the samequeryKeyPrefixpropDBTimeChartalready had, so the heatmap view gets that backpressure too.This reuses existing building blocks (
DBTimeChart,SegmentedControl, the chart-sync context,DBHeatmapChart+toHeatmapChartConfig, and the tile card header); no new chart or control primitives.Screenshots
RED metrics on the trace search results view, light theme:
Dark theme:
Test plan
traceRedMetrics.test.ts) and for the chart color pipeline (ChartUtils.test.ts, including per-series color pinning and that a pinned series does not consume a positional palette slot).tsc --noEmit,eslintat the warning budget with no new warnings, and the productionbuild:clickhouse.Notes
minorchangeset.@sourcepointing at the delta-view tile it mirrors.Deferred, with follow-ups filed so they do not get lost: