Repository navigation
feat(app): show RED metrics on the trace search results view - #3183
alex-fedotyev wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: ec9cb49 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 |
 |
Deep ReviewScope: ✅ No critical issues found. No P0/P1: security confirmed the SQL expressions are trusted source config used identically across the codebase (no new injection surface), all 11 prior-review fixes landed, and there is no happy-path crash or data loss. P2 advice below. 🟡 P2 — recommended
🔵 P3 nitpicks (9)
Reviewers (12): correctness, security, adversarial, testing, maintainability, project-standards, performance, kieran-typescript, julik-frontend-races, previous-comments, agent-native, learnings-researcher. Testing gaps:
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (mode === 'rate') { | ||
| return { | ||
| ...base, | ||
| select: [ |
There was a problem hiding this comment.
🔵 minor — Error-rate count + countIf + post-aggregation ratio + hidden helper series is a fourth copy of an existing pattern
The repo's REQUIRED DRY rule asks for a grep on the operation before adding a helper. This exact trio already exists at ServicesDashboardPage/HttpTab.tsx:221-239 (requestErrorRateConfig: total_requests / error_requests / error_rate, ERROR_RATE_PERCENTAGE_NUMBER_FORMAT) with its matching hiddenSeries={['total_requests','error_requests']} at HttpTab.tsx:419, again inside the CTE at HttpTab.tsx:282-298, and again at llm/dashboard/OverviewCharts.tsx:152-166. Extract one builder — e.g. errorRateSelect(errorCondition, { totalAlias, errorAlias, rateAlias }) returning the select trio plus its hidden-series list — into a shared module and have all four call sites use it, rather than adding a fourth independently-aliased copy whose helper names must be kept in sync with a separate ERROR_RATE_HELPER_SERIES constant by hand.
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
There was a problem hiding this comment.
The shape is shared, agreed, but extracting a builder would touch HttpTab, OverviewCharts and the service-dashboard side panels, which widens this PR beyond its own files. Tracking it as a follow-up rather than folding it in here.
PR Review
3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 minor 2 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
6bdd45c to
887421b
Compare
7c3a45a to
108e06a
Compare
🟡 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
|
E2E Test Results✅ All tests passed • 466 passed • 1 skipped • 1021s
Tests ran across 4 shards in parallel. |
108e06a to
3c58c91
Compare
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.
3c58c91 to
2c78a82
Compare
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
- Fall back to the single histogram on a compact (mobile) viewport instead of three unreadable side-by-side tiles. - Compute a numeric capped y-axis domain for the rate tile so it dedupes tick labels and no longer collapses to [0, 0] when the sole series is selected; the cap now applies on the selection and fit paths too. - Drop the unreachable 0/0 guard on the error-rate column; a returned GROUP BY bucket always has total_spans >= 1, so divide directly the way HttpTab does. - Rename the Duration percentile series p95/p99 to P95/P99 to match the app's acronym casing. - Strengthen the volume-mode test so it catches dropped sidebar filters, and add computeYAxisBounds coverage for the capped axis.
- Throughput now renders the status-grouped count histogram this view replaces, under the results-count query-key prefix, so react-query runs a single shared scan instead of a second ungrouped count() on every search and live-tail tick. This also restores the per-status drill-down the histogram offered. - Duration series pin their colors through each select item's `color` rather than a name-keyed seriesColors prop, so there is one source of truth for per-series color. Drops the DBTimeChart seriesColors prop, the DURATION_SERIES_COLORS map, and the alias/color lockstep test; setLineColors reads the color off the select item by result-column name.
…ant duration format - When a trace source has a custom group-by selected, fall back to the single histogram (which honors arbitrary group-by) instead of the RED tiles, whose grouping is fixed (Throughput and Errors by status, Duration ungrouped). The group-by selector now has a coherent effect rather than silently regrouping only part of the view. - Drop the hand-computed duration numberFormat: DBTimeChart already infers it via useChartNumberFormats -> getTraceDurationNumberFormat, because each duration series aggregates the source's duration expression. Removes the getTraceDurationNumberFormat memo and durationConfig's numberFormat parameter.
- Compact x-axis ticks use the responsive axis tick font size instead of a hardcoded 11px, matching the non-compact ticks and honoring the user's content font scale. - Correct the yAxisMaxDomain prop doc: the cap applies on every path (selection and fit included), not only the default path.
…ries
Group-by none resolves to no grouping, which collapses Throughput to a
single count() series; with the legend on, that surfaced the raw aggregate
expression ("count()") as the series label. Show the Throughput legend only
when the view is grouped (the status-code default), where it names each
status, matching how the histogram gated its own legend. The ungrouped case
keeps just the "Throughput" title.
| return undefined; | ||
| } | ||
| const entries: [string, ChartPaletteToken][] = []; | ||
| for (const selectItem of config.select) { |
There was a problem hiding this comment.
🟠 major — select[].color is documented as table-only, but every time chart now applies it, so existing dashboard line/bar tiles change color
Don't read config.select[].color in every DBTimeChart. Add an explicit seriesColors prop (or a separate per-series field) that only TraceRedMetricsChart passes. The schema comment at packages/common-utils/src/types.ts:459-465 says the field is used only on table tiles and other display types ignore it. The editor only shows the color control when the display type is Table (ChartEditorControls.tsx:317), and switching to Line doesn't clear it. Here's the result: a saved tile with a column color that was later switched to Line or Bar now renders in that color, and the user has no control to clear it. Because colors are keyed by valueColumnName, a grouped tile (e.g. count() grouped by ServiceName) colors every group line the same and they can't be told apart.
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
| } | ||
| const entries: [string, ChartPaletteToken][] = []; | ||
| for (const selectItem of config.select) { | ||
| if (selectItem.alias && selectItem.color) { |
There was a problem hiding this comment.
🔵 minor — An unrecognized color token makes the whole time chart fail to render
Before adding a select item's color to seriesColors, run it through isChartPaletteToken / resolveChartPaletteToken. HDXMultiSeriesTableChart.tsx:253-259 and DBNumberChart.tsx:322 already do this. getColorFromCSSToken throws on an unknown token (utils.ts:584-585). Unknown values (stale hex, hand-edited, or forward-rolled tokens) are deliberately left as-is by migrateOrPreserveColor (dashboard.ts:78). Inside setLineColors, that throw is caught as resultFormattingError, so the whole tile shows an error instead of falling back to the default palette.
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
Summary
Trace search shows a single count histogram above the results. For trace sources that expose a duration column, this replaces it with RED metrics:
countIf(error) / count(), rendered as a percent line) and volume (countIf(error), bars, grouped by status).durationPrecision.Every aggregation is over a raw column (
count,countIf,quantile/avgof the duration expression), so AggregatingMergeTree materialized views can satisfy the queries; the ratio and the 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. Clicking a status series on the Throughput or Errors-volume chart filters the results, which is the drill-down the histogram offered. Logs and other sources keep the existing histogram, as do trace sources on a compact (narrow/mobile) viewport, where three side-by-side tiles would not fit.The per-chart aggregations for Errors and Duration live in a small pure module,
traceRedMetrics.ts, built from the same base config the histogram uses, so they honor the active WHERE filter and time range and can be unit tested without rendering.Two supporting props on shared chart components, each with its only consumer in this PR:
yAxisMaxDomain, so the Errors rate tile caps at 100% instead of auto-scaling a near-zero series to a nonsense range. The cap is a numeric domain, so the axis keeps clean, de-duplicated tick labels and never collapses to a single point.compactXAxisLabels, so the edge time labels do not clip on tiles this narrow.The Duration percentiles are pinned to cool 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. The colors are set on each series' select-item
color(the schema's existing per-series color field), whichsetLineColorsnow reads by result-column name, so there is a single source of truth and no separate color map to keep in sync.The Errors and Duration tiles 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 cannot stack a second set of chart queries on the first. Throughput shares the results-count query instead, so it adds no extra scan.
Screenshots
Light theme:
Dark theme:
Test plan
traceRedMetrics.test.ts) and the chart color pipeline (ChartUtils.test.ts), including that a select-item color overrides the positional palette and does not consume a positional slot.chart-redslot. Because the color now lives on the select item, it cannot drift from the series it colors.computeYAxisBoundscoverage for the capped rate axis: a flat/zero series shows[0, cap]with clean ticks, the cap holds under selection (no[0, 0]collapse) and over reference lines, and is skipped for stacked charts.tsc --noEmit,eslintwith no new warnings, and knip. CI covers the full suite and the productionbuild:clickhousebundle.Notes
minorchangeset.mainnow that fix(app): count legacy STATUS_CODE_ERROR spans as errors #3181 (the shared error-span predicate) has merged, so the diff is just the RED-metrics files. The Errors tile useserrorStatusPredicateSqlfrom@hyperdx/common-utils, which fix(app): count legacy STATUS_CODE_ERROR spans as errors #3181 landed.