Skip to content

fix: Bound the side panel's row lookup after 'View Trace' to a time window - #2816

Open
pulpdrew wants to merge 5 commits into
mainfrom
drew/timestamp-filter-trace-viewer
Open

fix: Bound the side panel's row lookup after 'View Trace' to a time window#2816
pulpdrew wants to merge 5 commits into
mainfrom
drew/timestamp-filter-trace-viewer

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

When initially opening the trace viewer when navigating from a log row, the current row's span was looked up in the traces table by SpanId and TraceId only. This does not make use of the partition/primary keys on time stamp which are present in the OTEL schemas, making the experience slow for large volume deployments.

This change adds date range bound to the row lookup when a date range can be reasonable inferred from the source panel/page. To account for cases where the target of the lookup may be outside of the date range (eg. looking up a very long span based on a log row which was emitted near the end of the very long span), there is a fallback to the previous no-bounds query.

When navigating from a Log Row Details view to the trace waterfall, the date range is determined by using the first DateTime-type column in the log row's timestamp value expression. This handles the case where the timestamp value expression has a leading Date type column, which would then result in the lookup date range being centered around midnight instead of the actual time of the log message. Again, if there are no matching results based on that lookup, then the previous no-bound lookup behavior serves as the fallback.

Screenshots or video

Before:

Screen.Recording.2026-08-05.at.8.41.42.AM.mov

After:

Screen.Recording.2026-08-05.at.8.41.04.AM.mov

How to test on Vercel preview

Open the trace side panel and observe the generated queries. Try it when navigating from both traces and from correlated logs. Ensure that the timestamp value expression is used rather than the displayed timestamp value expression as a filter (the latter is often not in the PK).

References

@changeset-bot

changeset-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6e016a1

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

This PR includes changesets to release 4 packages
Name Type
@hyperdx/app Patch
@hyperdx/common-utils 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

@vercel

vercel Bot commented Aug 5, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 6, 2026 11:07pm
hyperdx-storybook Ready Ready Preview Aug 6, 2026 11:07pm

Request Review

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 484 production lines changed (Tier 2 max: < 250)
  • Cross-layer change: touches frontend (packages/app) + shared utils (packages/common-utils)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 6
  • Production lines changed: 484 (+ 1007 in test files, excluded from tier calculation)
  • Branch: drew/timestamp-filter-trace-viewer
  • Author: pulpdrew

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@greptile-apps

greptile-apps Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR bounds the initial cross-source row lookup used by View Trace while preserving an unbounded fallback when the inferred window misses.

  • Projects every component of a source timestamp expression and selects the highest-precision timestamp value as the navigation anchor.
  • Passes an asymmetric time window through side-panel frames and row-detail tabs.
  • Retries without bounds after a complete, empty bounded query.
  • Adds focused coverage for composite timestamps, fallback behavior, and side-panel navigation.

Confidence Score: 5/5

The PR appears safe to merge.

The previously reported composite timestamp failure is fixed: every timestamp component is projected, day-precision columns are skipped, and the highest-precision DateTime value anchors the trace lookup window; no blocking failure remains.

Important Files Changed

Filename Overview
packages/app/src/components/DBRowDataPanel.tsx Adds timestamp projections, bounded row querying, and a guarded unbounded fallback without leaving the previously reported composite-timestamp failure.
packages/app/src/components/DBRowSidePanel.tsx Resolves the source row’s event-time anchor, carries it through View Trace navigation, and applies the resulting window only to the landed cross-source frame.
packages/app/src/utils/rowTimestamps.ts Adds bracket-aware timestamp projections, metadata-based precision selection, and validated asymmetric lookup-window construction.
packages/common-utils/src/core/utils.ts Adds reusable ClickHouse timestamp-type classification supporting Date, DateTime, DateTime64, timezone arguments, and nullable wrappers.
packages/app/src/components/tests/DBRowSidePanel.viewTraceTimeFilter.test.tsx Covers the full View Trace anchor propagation and verifies that composite Date plus DateTime expressions use the event-time component.

Sequence Diagram

sequenceDiagram
  participant Log as Log row panel
  participant Stack as Side-panel stack
  participant Trace as Trace row lookup
  participant CH as ClickHouse
  Log->>Log: Resolve highest-precision timestamp
  Log->>Stack: Push trace frame with focusTimestamp
  Stack->>Trace: Derive bounded date range
  Trace->>CH: Query TraceId + SpanId within window
  alt Matching span found
    CH-->>Trace: Return span
  else Complete bounded result is empty
    Trace->>CH: Retry TraceId + SpanId without bounds
    CH-->>Trace: Return fallback result
  end
Loading

Reviews (6): Last reviewed commit: "Merge branch 'main' into drew/timestamp-..." | Re-trigger Greptile

Comment thread packages/app/src/components/DBRowDataPanel.tsx Outdated
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 275 passed • 1 skipped • 1097s

Status Count
✅ Passed 275
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew
pulpdrew force-pushed the drew/timestamp-filter-trace-viewer branch from 1a82627 to 183ca71 Compare August 5, 2026 13:30
@github-actions github-actions Bot added review/tier-3 Standard — full human review required and removed review/tier-2 Low risk — AI review + quick human skim labels Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

<!-- deep-review -->

Deep Review

Scope caveat: git was unavailable in this environment (every shell call failed at sandbox setup), so the changed-file set was reconstructed from the working tree at PR head rather than from a diff. New-vs-pre-existing attribution is therefore less certain than usual, and each finding below states which side it falls on.

🔴 P0/P1 — must fix

  • packages/app/src/components/DBRowSidePanel.tsx:288 — The ±1h window is derived from the origin log's instant but filtered against the destination span's Timestamp, which in the OTel traces schema is the span start, so any span longer than roughly an hour that emits a log near its end is now excluded and the lookup returns zero rows with no fallback.
    • Fix: Make the window asymmetric around the anchor (reach much further backward than forward, since a span always starts at or before the log it contains) and retry once without dateRange when a bounded lookup returns zero rows.
    • correctness, adversarial, reliability

The consequence chain was independently traced by two reviewers: the bounded query succeeds with data: [], normalizedRow becomes undefined (line 403), traceId becomes undefined (line 502), and DBTracePanel then renders its !traceId fallback — a "Trace ID Expression" source-configuration editor — so a user who clicked "View Trace" lands on what looks like a broken-source form. Because the frame is persisted into sidePanelSourceStack, a reload or a shared link reproduces it.

🟡 P2 — recommended

  • packages/app/src/components/DBRowSidePanel.tsx:783 — The !isRowSuccess guard distinguishes only loading and error, so a successful-but-empty response falls through to the full render and timestampDate becomes an Invalid Date that propagates into oneHourRange/fourHourRange and on into child panels' dateRange.
    • Fix: Add an explicit isRowSuccess && rowData?.data?.length === 0 branch rendering a "row not found" state, and guard the derived ranges behind a valid-date check so NaN never reaches a child query key.
    • adversarial, reliability, testing
  • packages/app/src/utils/rowTimestamps.ts:87 — The tie-break keeps the earlier token on equal precision, so a toStartOfHour(Timestamp), Timestamp expression — the exact shape optimizeTimestampValueExpression documents and generates — anchors on the truncated bucket boundary and shrinks the window's forward reach to near zero.
    • Fix: On a precision tie prefer the later instant rather than the earlier token, since a truncated column is always at or before the true event time.
    • correctness, adversarial
  • packages/app/src/components/DBTracePanel.tsx:109 — Sibling row lookups that have a suitable window already in scope still call useRowData without one, so SpanDetailPanel and the session-event push at DBRowSidePanel.tsx:566 keep the unbounded scan this change exists to remove.
    • Fix: Thread the already-computed window into these useRowData call sites, or document why each is deliberately left unbounded as the span-link path already is.
    • performance, correctness
  • packages/app/src/components/DBRowSidePanel.tsx:288 — The frameDateRange memo is the actual behavior change and has no test coverage for any of its four gates: the ±60-minute bounds, the leafNav != null suppression, the absent-anchor case, and the unparseable-anchor case.
    • Fix: Add packages/app/src/components/__tests__/DBRowSidePanel.test.tsx covering each gate, or extract the window derivation into a pure helper and unit-test it there.
    • testing, maintainability, kieran-typescript, learnings, reliability
  • packages/app/src/components/__tests__/DBRowDataPanel.test.ts:92 — The time filtering block mocks useChartConfig and asserts only on the config object's shape, so nothing pins that the emitted SQL bounds the timestampValueExpression column rather than the displayed timestamp column.
    • Fix: Add a test that exercises the real renderer and asserts the generated predicate targets the source's timestampValueExpression, not the displayed expression.
    • testing
🔵 P3 nitpicks (10)
  • packages/app/src/utils/rowTimestamps.ts:65classifyTimestampType strips only a single Nullable(...) wrapper, so a LowCardinality(DateTime) column yields no anchor and silently keeps the full scan, disagreeing with renderChartConfig.ts:977 which already uses stripTypeWrappers for the same purpose.
    • Fix: Use the existing stripTypeWrappers helper inside classifyTimestampType and add a LowCardinality case to rowTimestamps.test.ts.
  • packages/app/src/components/DBRowSidePanel.tsx:299 — The isNaN guard checks the anchor but not the arithmetic result, so a JS-max-date value overflows on add(focus, { minutes: 60 }) and binds {Int64: NaN} into the query, surfacing a raw ClickHouse error.
    • Fix: Validate both computed bounds before returning the tuple and fall back to undefined when either is invalid.
  • packages/app/src/utils/rowTimestamps.ts:38 — Every path that silently drops the window — day-precision-only sources, an alias/expression mismatch, an absent timestampValueExpression — collapses into the same unsignalled undefined, so a fleet-wide regression to full scans is indistinguishable from correct behavior.
    • Fix: Warn when a source declares a non-empty timestampValueExpression but no anchor can be resolved, so the optimization being inactive is observable.
  • packages/app/src/utils/rowTimestamps.ts:77 — This is the third copy of the unix-seconds-vs-string Date coercion, and the useFormatTime.tsx:10 variant already omits the * 1000 branch, so the copies have already drifted.
    • Fix: Export one clickhouseValueToDate helper and call it from resolveRowTimestampAnchor, DBRowSidePanel.tsx:406, and useFormatTime.tsx.
  • packages/app/src/utils/rowTimestamps.ts:61 — The selection rule duplicates pickBucketTimestampColumn in common-utils/src/core/utils.ts:174, kept in sync only by a comment asserting that it matches.
    • Fix: Extract the shared skip-and-rank rule into one function that both call sites consume.
  • packages/app/src/components/DBRowSidePanel.types.ts:40focusTimestamp means "anchor for bounding the destination lookup" while focusDate at line 499 in the same file means "instant the waterfall scrolls to", inviting a future contributor to conflate them.
    • Fix: Rename the schema field to name its role, such as lookupAnchorTimestamp, while the URL contract is still unshipped.
  • packages/app/src/components/DBRowSidePanel.tsx:288 — The ±60-minute window is spelled out independently of the identical oneHourRange at line 484 and the fourHourRange at line 492, so retuning the pruning window means finding three call sites.
    • Fix: Introduce a named window constant and a small windowAround(date, span) helper shared by all three.
  • packages/app/src/utils/rowTimestamps.ts:54row: Record<string, any> leaves the row[alias] access and the typeof rawValue === 'number' narrowing unchecked, against the documented "avoid any" rule in agent_docs/code_style.md.
    • Fix: Type the parameter as Record<string, unknown> and narrow explicitly to string | number before constructing the Date.
    • kieran-typescript, project-standards
  • packages/app/src/components/DBRowDataPanel.tsx:56 — The "callers must memoize the tuple" obligation is documented but unenforceable by types or runtime, and two reviewers confirmed TanStack Query hashes keys structurally so the stated risk is overstated anyway.
    • Fix: Derive the query key's range component from epoch milliseconds so caller identity stops mattering, or accept a primitive anchor and build the tuple inside the hook.
    • maintainability, api-contract
  • packages/app/src/components/DBRowOverviewPanel.tsx:328 — A second Accordion.Item value="spanEvents" duplicates the block at line 275 with only ps differing, so the Span Events section renders twice with a duplicate Accordion value; verified directly and pre-existing, but it sits in a file this change touches.
    • Fix: Delete the later duplicate block and keep the contentPx variant.

Reviewers (12): correctness, adversarial, performance, testing, maintainability, project-standards, kieran-typescript, reliability, api-contract, julik-frontend-races, agent-native, learnings.

Testing gaps:

  • No test round-trips a frame carrying focusTimestamp through pushSourcesidePanelSourceStackparseSourceStack; the FRAME fixture in useSidePanelStack.test.tsx never sets the field, so the back-compat guarantee (zod strips unknown keys, verified safe today) is unpinned against a future .strict().
  • No test covers what the panel renders when a bounded lookup succeeds with zero rows — the specific new failure mode.
  • No LowCardinality-wrapped or truncating-token (toStartOfHour(ts), ts) case in rowTimestamps.test.ts.
  • Presence of Playwright/e2e coverage for the "View Trace" flow could not be confirmed, since directory enumeration was unavailable in this environment.

Refuted during review (recorded so they are not re-raised): the fresh-[Date, Date]-identity refetch loop, keepPreviousData stamping a stale anchor, a bounded-then-unbounded double query, and cross-version URL parse failure were each investigated and found not to hold.

@pulpdrew
pulpdrew marked this pull request as draft August 5, 2026 17:36
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

The bounded/fallback state machine in useRowData is well covered by packages/app/src/components/__tests__/DBRowDataPanel.test.ts (bounded hit, bounded empty → fallback, fallback serves the row, bounded error, no-window, loading handoff, incomplete chunk), and the pure helpers in rowTimestamps.ts have thorough unit tests. Three reviewers independently flagged "bounded-query errors never reach the unbounded fallback" as a P1 defect; that behavior is deliberate and explicitly pinned by a test at DBRowDataPanel.test.ts:266. What survives is the narrower point below.

🟡 P2 -- recommended

  • packages/app/src/components/DBRowDataPanel.tsx:239 -- isFallbackActive treats every bounded-query error as evidence the row genuinely failed, but the bounded config is the only one that reaches timeFilterExpr, whose await metadata.getColumn(...) at renderChartConfig.ts:936 is unguarded while the adjacent getTableMetadata call is wrapped, so a DESCRIBE failure on the destination table now hard-errors a lookup that previously had no metadata dependency at all.
    • Fix: Let failures that only the bounded query can produce fall through to the unbounded retry, or wrap the getColumn call in timeFilterExpr so it degrades to columnMeta = null like its neighbour.
    • correctness, adversarial, julik-frontend-races
  • packages/app/src/utils/rowTimestamps.ts:109 -- The 4h/1h window misses systematically for whole deployment classes (spans outliving the lookback, sources anchored on ingestion rather than emit time, cross-emitter skew beyond the lead), and because the fallback is gated on the bounded result settling, each miss costs the bounded round trip plus the full unbounded scan serially, with nothing recording how often that happens.
    • Fix: Instrument the isBoundedEmpty rate so the constants can be tuned against real miss frequency.
    • adversarial, performance
  • packages/app/src/components/DBRowSidePanel.tsx:539 -- The wiring that feeds the tested hook is itself untested: rowFocusTimestamp resolution, the focusTimestamp stamped on the "View Trace" push at line 918, its deliberate absence on the span-link push at line 596, and the rule that frameDateRange must return undefined while leafNav is set are all asserted only in comments.
    • Fix: Add a DBRowSidePanel test following the DBTracePanel.test.tsx mocking pattern that pins the anchor round trip and the nav-entry suppression.
    • testing, maintainability, kieran-typescript, project-standards
🔵 P3 nitpicks (5)
  • packages/app/src/utils/rowTimestamps.ts:126 -- getRowLookupWindow validates the anchor but not the two bounds derived from it, so a URL-supplied anchor within the window width of the JS Date range limit returns a tuple containing an Invalid Date, which becomes fromUnixTimestamp64Milli({Int64: NaN}).
    • Fix: Return undefined unless both computed bounds have a finite getTime().
  • packages/app/src/utils/rowTimestamps.ts:78 -- The bare new Date(rawValue) is correct only because date_time_output_format: 'iso' is pinned in the ClickHouse client defaults; every test fixture is Z-suffixed, so flipping that setting to simple would silently reparse every anchor as browser-local time with a green suite.
    • Fix: Add a case asserting the offset-less "YYYY-MM-DD HH:MM:SS" form is either handled or rejected, so the cross-package dependency fails loudly.
  • packages/app/src/components/DBRowSidePanel.tsx:539 -- rowFocusTimestamp sits ~40 lines from the unrelated pre-existing focusDate, and swapping one for the other would type-check after a trivial .toISOString() / new Date() wrapper.
    • Fix: Rename the anchor to something that cannot be confused with the waterfall highlight hint.
  • packages/app/src/components/DBRowDataPanel.tsx:59 -- The dateRange param documents a "callers must memoize this" contract enforced only by a comment, even though getRowLookupWindow already takes the ISO string and returns the tuple.
    • Fix: Accept the anchor string and derive the tuple inside the hook so the contract is structural rather than advisory.
  • packages/app/src/components/DBRowSidePanel.tsx:918 -- CLAUDE.md requires a changeset for user-facing behavior changes to @hyperdx/app, and this qualifies; the reviewer could not enumerate .changeset/ in this environment.
    • Fix: Confirm a changeset accompanies this branch.

Reviewers (8): correctness, adversarial, testing, performance, julik-frontend-races, kieran-typescript, maintainability, project-standards.

Testing gaps:

  • No end-to-end assertion that a window miss still lands on the correct row, i.e. that the feature is a pure optimization.
  • No test that a composite destination timestampValueExpression actually emits the toDate(...)-bounded predicate on the partition column that the whole optimization depends on.
  • No backward-compatibility case for a sidePanelSourceStack link serialized before focusTimestamp existed, or one carrying a stale-but-valid anchor.

Environment note: Bash, Grep, Glob, and WebFetch were all unavailable in this run (bwrap init failure; no search tooling), so scope was reconstructed by reading files directly rather than from git diff. Attribution of new-vs-pre-existing lines is inferred, and the agent-native and past-learnings reviewers were skipped because they require repo-wide search.

@pulpdrew
pulpdrew requested a review from karl-power August 5, 2026 19:12
karl-power
karl-power previously approved these changes Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trace drilldown from log side panel runs an unbounded TraceId/SpanId lookup (no partition pruning) — regression since 2.30.0

2 participants