Repository navigation
fix: show OTel exceptions in the side panel and trace waterfall - #3412
Conversation
🦋 Changeset detectedLatest commit: 5c18405 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.
|
E2E Test Results✅ All tests passed • 445 passed • 1 skipped • 1833s
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:
Additional context: touches background tasks or the delivery pipeline lightly (4 lines, under the 30-line bar for Tier 4) Review process: Full human review — logic, architecture, edge cases. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
| @@ -1261,7 +1262,9 @@ export function DBTraceWaterfallChartContainer({ | |||
|
|
|||
| const barBackgroundColor = | |||
There was a problem hiding this comment.
🔵 minor — Log color rule is now kept in two places: the label swatch and barColor
This diff adds the same isError ? getChartColorError() : getChartColorSuccess() log rule to both barColor (L128) and the inline barBackgroundColor ternary that colors the label swatch. Any future change to one will leave the swatch and the bar out of sync. For log rows, compute the swatch with barColor({ isHighlighted: false, isError, type }), or pull the log branch into a small shared helper.
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
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 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. |
Deep Review✅ No critical issues found. No P0/P1 defects. The change is well-scoped and the risky areas (waterfall root-node logic, exception-attribute precedence, memo dependency arrays, the new config defaults) were traced and hold up. The prior Greptile P1 — fresh installs lose trace views because bare 🟡 P2 — recommended
🔵 P3 nitpicks (6)
Reviewers (8): correctness, testing, maintainability, project-standards, kieran-typescript, adversarial, previous-comments, julik-frontend-races. Testing gaps: crash-log dangling-parent root promotion (the stated intent) is unverified; no test asserts the tooltip renders absolute 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 |
[Medium impact] The changes affect exception and trace views plus local default source setup. Fresh installations can still lose trace details and the waterfall, so the previous finding needs a fix before merging.
|
| '[{"from":{"databaseName":"default","tableName":"otel_logs"},"kind":"log","timestampValueExpression":"Timestamp","name":"Logs","displayedTimestampValueExpression":"Timestamp","implicitColumnExpression":"Body","serviceNameExpression":"ServiceName","bodyExpression":"Body","eventAttributesExpression":"LogAttributes","resourceAttributesExpression":"ResourceAttributes","defaultTableSelectExpression":"Timestamp,ServiceName,SeverityText,Body","severityTextExpression":"SeverityText","traceIdExpression":"TraceId","spanIdExpression":"SpanId","metadataMaterializedViews":{"kvRollupTable":"otel_logs_kv_rollup_15m","granularity":"15 | ||
| minute"},"connection":"Local | ||
| ClickHouse","traceSourceId":"Traces","sessionSourceId":"Sessions","metricSourceId":"Metrics"},{"from":{"databaseName":"default","tableName":"otel_traces"},"kind":"trace","timestampValueExpression":"Timestamp","name":"Traces","displayedTimestampValueExpression":"Timestamp","implicitColumnExpression":"SpanName","serviceNameExpression":"ServiceName","eventAttributesExpression":"SpanAttributes","resourceAttributesExpression":"ResourceAttributes","defaultTableSelectExpression":"Timestamp,ServiceName,StatusCode,round(Duration/1e6),SpanName","traceIdExpression":"TraceId","spanIdExpression":"SpanId","durationExpression":"Duration","durationPrecision":9,"parentSpanIdExpression":"ParentSpanId","spanNameExpression":"SpanName","spanKindExpression":"SpanKind","statusCodeExpression":"StatusCode","statusMessageExpression":"StatusMessage","metadataMaterializedViews":{"keyRollupTable":"otel_traces_key_rollup_15m","kvRollupTable":"otel_traces_kv_rollup_15m","granularity":"15 | ||
| ClickHouse","traceSourceId":"Traces","metricSourceId":"Metrics"},{"from":{"databaseName":"default","tableName":"otel_traces"},"kind":"trace","timestampValueExpression":"Timestamp","name":"Traces","displayedTimestampValueExpression":"Timestamp","implicitColumnExpression":"SpanName","serviceNameExpression":"ServiceName","eventAttributesExpression":"SpanAttributes","resourceAttributesExpression":"ResourceAttributes","defaultTableSelectExpression":"Timestamp,ServiceName,StatusCode,round(Duration/1e6),SpanName","traceIdExpression":"TraceId","spanIdExpression":"SpanId","durationExpression":"Duration","durationPrecision":9,"parentSpanIdExpression":"ParentSpanId","spanNameExpression":"SpanName","spanKindExpression":"SpanKind","statusCodeExpression":"StatusCode","statusMessageExpression":"StatusMessage","spanEventsValueExpression":"Events","spanLinksValueExpression":"Links","metadataMaterializedViews":{"kvRollupTable":"otel_traces_kv_rollup_15m","granularity":"15 |
There was a problem hiding this comment.
Fresh installations lose trace views
The seeded otel_traces table declares Events.Timestamp, Events.Name, and Events.Attributes, but no Events column. The same applies to Links. These new defaults make useRowData select bare Events and Links, and the waterfall select bare Events. Those queries fail against the seeded schema, so newly provisioned sources cannot load trace details or the waterfall.
Build the expected arrays from the dotted columns, or add matching columns before enabling these defaults. The same defaults appear in docker/hyperdx/entry.local.base.sh and packages/api/.env.development.
| const formatTime = useFormatTime(); | ||
| // OTel records exceptions on spans as events named `exception`. | ||
| const isException = marker.name === 'exception'; | ||
| const markerColor = isException |
There was a problem hiding this comment.
🔵 minor — Exception marker red uses a UI token, not the chart error color the waterfall uses
The waterfall paints error bars and error-log swatches with getChartColorError() (--color-chart-error, set in packages/app/src/utils.ts:765). This marker uses var(--color-bg-danger), which is a different red in both brands (for example mantine red-4 or #ef4444). So an error span and its exception marker come out in two different reds. Use getChartColorError() here instead, as the chart-color contract in the repository conventions asks.
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
| @@ -1261,7 +1266,9 @@ export function DBTraceWaterfallChartContainer({ | |||
|
|
|||
| const barBackgroundColor = | |||
There was a problem hiding this comment.
🔵 minor — Label swatch color repeats barColor logic, and both had to be edited by hand
This PR changes the log/error color rule in two places: barColor() (line 128) and this inline barBackgroundColor ternary. The two already disagree for error spans: the bar is red but the swatch stays the service color. Build the swatch from barColor({ isHighlighted: false, isError, type, serviceColor }), or one shared helper, so the color rule has a single source.
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
jordan-simonovski
left a comment
There was a problem hiding this comment.
Looks good! Very exciting
f72c965 to
501b5ce
Compare
The merge-base changed after approval.
501b5ce to
d7c00a0
Compare
d7c00a0 to
5c18405
Compare
Closes HDX-5732.
Summary
Current OTel records exceptions as log records with
exception.*attributes, and span exceptions only load when the trace source setsspanEventsValueExpression, which our default sources didn't. This makes exceptions show up out of the box.Changes
exception.type/message/stacktraceattributes, not onlyexceptionspan events, and now sits above the HTTP panel. Drops the always-empty Breadcrumbs section.exceptionspan events are red.docker-compose.yml,entry.local.base.sh,.env.development): set span events/links, add the summary and exponential histogram metric tables, and drop stale rollup and schema-stripped fields, matching the Helm chart. Existing sources aren't migrated (also submitted this change for the Helm chart).How to test
exception.*attributes (e.g. from clickstack-browser): the Exception panel shows.Eventson the trace source, open a trace with an exception: red marker and red error log in the waterfall.Screenshots
Exceptions panel in sidebar:

Exceptions tooltip in waterfall:

Trace source populates

EventsandLinksby default: