Skip to content

fix: count legacy STATUS_CODE_ERROR spans as errors - #3527

Merged
kodiakhq[bot] merged 3 commits into
ClickHouse:mainfrom
Tyagiquamar:agent/legacy-span-error-status
Nov 3, 2026
Merged

kodiakhq[bot] merged 3 commits into
ClickHouse:mainfrom
Tyagiquamar:agent/legacy-span-error-status

Conversation

@Tyagiquamar

Copy link
Copy Markdown
Contributor

Summary

Service map error counts, the error drill-down, and the trace waterfall now treat legacy STATUS_CODE_ERROR span statuses the same way the services dashboard already does. Collectors that still write that value no longer show a service or trace as error-free.

Root cause

errorStatusPredicateSql already matches both Error and STATUS_CODE_ERROR. The service map still counted only ServerSpans.statusCode = 'Error', and its error drill-down used the same exact match. The waterfall's getRowSeverity returned an error only when StatusCode === 'Error'. On a deployment whose collector writes STATUS_CODE_ERROR, those surfaces reported zero errors while the services dashboard did not.

Fixes #3203.

Fix

The SQL predicate and a new isErrorStatusCode helper share one list: error and status_code_error, compared case-insensitively. The service map count and the error drill-down use the SQL predicate. The waterfall uses the helper for the error count and the row highlight. Log severity is unchanged.

Test plan

  • Before the helper accepted legacy values, jest --testPathPatterns spanPredicates failed 5 cases (ERROR, error, STATUS_CODE_ERROR, status_code_error, Status_Code_Error expected true, received false) when isErrorStatusCode was an exact === 'Error' check.
  • After the fix, from packages/common-utils: node ..\..\.yarn\releases\yarn-4.13.0.cjs exec jest --coverage=false --testPathPatterns spanPredicates passed, 17 tests.
  • From the repo root: node .yarn/releases/yarn-4.13.0.cjs workspace @hyperdx/app exec jest --coverage=false --testPathPatterns "useServiceMap.test|ServiceMap/__tests__/utils.test" passed, 86 tests.
  • node .yarn/releases/yarn-4.13.0.cjs workspace @hyperdx/common-utils build succeeded.
  • eslint --flag v10_config_lookup_from_file --quiet on the touched files exited 0. Prettier was run on those files.

How to reproduce

Starting point: a trace source whose status column contains STATUS_CODE_ERROR (legacy collector) as well as Error. The public demo dataset does not store that legacy value, so the unit tests above are the reliable reproduction.

On main:

  1. Open the service map for that source. Nodes whose spans use only STATUS_CODE_ERROR show 0.00% errors.
  2. Open a trace that contains a STATUS_CODE_ERROR span. The waterfall does not mark that span as an error, and the header error count skips it.

On this branch:

  1. The same service map counts those spans, and the error drill-down filter is lower(StatusCode) IN ('error', 'status_code_error').
  2. The waterfall highlights those spans and includes them in the error count.

Screenshots or video

No screenshots. The change is the status match used for counts and highlighting, not layout. Reproducing the visible difference needs rows stored as STATUS_CODE_ERROR, which play.hyperdx.io does not provide.

How to test on Vercel preview

N/A. This is not a layout change. The corrected counts show up only when the selected trace source stores STATUS_CODE_ERROR.

References

Service map counts and the trace waterfall only matched the short status Error, so collectors that still write STATUS_CODE_ERROR showed no errors.
@Tyagiquamar
Tyagiquamar requested a review from a team as a code owner November 3, 2026 11:38
@Tyagiquamar
Tyagiquamar requested a review from knudtty November 3, 2026 11:38
@vercel

vercel Bot commented Nov 3, 2026

Copy link
Copy Markdown

@Tyagiquamar is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot

changeset-bot Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 483540a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Patch
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

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

});
}
return row.StatusCode === 'Error' ? 'error' : undefined;
return isErrorStatusCode(row.StatusCode) ? 'error' : undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — Waterfall legacy-status fix has no component test

The waterfall's error count and row highlight now depend on isErrorStatusCode, but DBTraceWaterfallChart.test.tsx has no fixture with StatusCode: 'STATUS_CODE_ERROR'. A regression back to === 'Error' in getRowSeverity would still pass. Add a waterfall test with a legacy-status span and assert that the error count includes it and the row is marked as an error.

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

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 minor

1 posted as inline comment(s) on the changed lines. 2 listed below.

Findings outside the changed lines

  • 🟠 packages/app/src/components/ServiceMap/utils.ts:137 — Error drill-down still matches only short span kinds, so legacy-collector services return zero rows → Collectors that write STATUS_CODE_ERROR also write SPAN_KIND_SERVER / SPAN_KIND_CONSUMER. The service map counts those spans because its query (packages/app/src/hooks/useServiceMap.tsx:126) filters on IN ('Server','Consumer','SPAN_KIND_SERVER','SPAN_KIND_CONSUMER'). The drill-down WHERE still uses IN ('Server', 'Consumer'). On exactly the deployments this PR targets, the map now shows errors but clicking through opens an empty search. Fix: add the legacy kinds to the drill-down filter, ideally through one shared span-kind predicate in spanPredicates.ts (next to serverSpanKindPredicateSql) used by both the query and the drill-down. Update the test expectation to match.
1 minor
  • 🔵 packages/app/src/components/DBRowSidePanel.tsx:1012 — Span side panel still colours only exact 'Error' status red → After this PR, a STATUS_CODE_ERROR span is highlighted as an error in the waterfall. Clicking it opens the side panel, where the status text stays uncoloured because the check is still statusCode === 'Error'. STATUS_CODE_OK similarly misses the green. Use isErrorStatusCode(statusCode) here so the two views agree.

Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: base:e2754d20 → PR head · 9 files (6 source/test, 1 changeset) · ~packages/common-utils span predicates + packages/app service map & trace waterfall.
Intent: Treat the legacy STATUS_CODE_ERROR span status as an error (alongside Error) across the service-map error count, the error drill-down filter, and the trace-waterfall row severity, backed by one shared ERROR_STATUS_CODES list.

✅ No critical issues found. The change is strictly additive — the new SQL predicate matches a superset of the old = 'Error' check, chSql inlines the serviceMapErrorCountSql() string as raw SQL identically to the prior inline fragment (verified at packages/common-utils/src/clickhouse/index.ts:266), the interpolated literals are static (no injection surface), and the buildServiceMapErrorWhere placeholder-to-argument mapping is correct and asserted by a full-string test. The JS helper and the SQL predicate share one source of truth, and the happy paths are well covered.

🟡 P2 — recommended

  • packages/app/src/components/DBRowSidePanel.tsx:1012 — the span detail side panel still colors status red only on an exact statusCode === 'Error', so a STATUS_CODE_ERROR span that this PR now highlights red in the waterfall opens a side panel whose status text is not red, leaving the same user flow internally inconsistent. Pre-existing; file untouched by this diff, so sever it as a follow-up rather than a merge blocker.
    • Fix: route this status check through the shared isErrorStatusCode helper in a follow-up so every span surface agrees on what "errored" means.
🔵 P3 nitpicks (3)
  • packages/app/src/components/DBTraceWaterfallChart.tsx:158 — getRowSeverity's new span branch (isErrorStatusCode(row.StatusCode)) has no direct test; coverage is indirect via the well-tested isErrorStatusCode unit tests.
    • Fix: add a getRowSeverity case asserting a STATUS_CODE_ERROR span row returns 'error' while a log row still routes through log severity.
  • packages/common-utils/src/core/spanPredicates.ts:24 — isErrorStatusCode matches only string forms, not the numeric OTLP code '2' that the CLI waterfall also treats as error; the old app check had the same gap, so this is a parity note, not a regression.
    • Fix: decide whether the shared helper should also recognize '2' to unify span-error semantics across app and CLI surfaces.
  • packages/app/src/components/aiSummarize/logic.ts:83 — a sibling surface uses a case-sensitive exact === 'STATUS_CODE_ERROR' check that diverges from the new shared predicate; pre-existing and out of scope.
    • Fix: fold this into the shared helper in a follow-up so the single-definition goal holds across all consumers.

Reviewers (5): correctness, testing, maintainability, kieran-typescript, performance.

Testing gaps: getRowSeverity span/log branching is only indirectly covered; no end-to-end assertion that a STATUS_CODE_ERROR span increments the waterfall header error count.

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

@brandon-pereira

Copy link
Copy Markdown
Member

LGTM Thanks!

@kodiakhq
kodiakhq Bot merged commit d0dd06f into ClickHouse:main Nov 3, 2026
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Route the remaining error-status checks through the shared predicate

2 participants