Skip to content

Show elapsed time and Generated SQL for search timeline view#2384

Open
vinzee wants to merge 2 commits into
hyperdxio:mainfrom
vinzee:va/stats_for_nerds
Open

Show elapsed time and Generated SQL for search timeline view#2384
vinzee wants to merge 2 commits into
hyperdxio:mainfrom
vinzee:va/stats_for_nerds

Conversation

@vinzee
Copy link
Copy Markdown
Contributor

@vinzee vinzee commented May 30, 2026

Summary

Shows scanned row count and elapsed query time inline on the search page. Adds a code icon button that opens a modal with the formatted generated SQL (similar to the modal in results table).

Screenshots or video

Elapsed time with the Show Generated SQL button:
image

Modal showing SQL for the timeline view:
image

How to test on Vercel preview

Preview routes:

Steps:

References

  • Linear Issue: n/a
  • Related PRs: n/a

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 30, 2026

⚠️ No Changeset found

Latest commit: 802c937

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented May 30, 2026

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

A member of the Team first needs to authorize it.

@vinzee vinzee changed the title Show elapsed time and Generated SQL modal for search timeline view Show elapsed time and Generated SQL for search timeline view May 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 30, 2026

Deep Review

🔴 P0/P1 -- must fix

  • packages/app/src/DBSearchPage.tsx:884 -- The emit useEffect depends on [searchElapsedMs, sourceId] and only short-circuits on searchElapsedMs == null, so when the user picks a new chartConfig.source after a completed search the effect refires and emits HyperDX.addAction('search executed', { latency_ms: <prior>, source_id: <new> }), attributing the previous run's latency to a source that never ran the query.
    • Fix: Snapshot sourceId alongside the elapsed value inside the first effect (single state object, or pair of refs cleared on emit) and drop sourceId from the emission effect's dependency array.
    • correctness, maintainability, adversarial, julik-frontend-races, previous-comments
  • packages/app/src/DBSearchPage.tsx:866 -- isAnyQueryFetching = useIsFetching({ queryKey: [QUERY_KEY_PREFIX] }) > 0 aggregates every query under the search prefix, including useLiveUpdate polls (live tail defaults to true at parseAsBoolean.withDefault(true)), window-focus refetches, and retries, so the hook mints a 'search executed' telemetry event on every poll cycle and floods the metric with phantom events that are nothing to do with user-initiated searches.
    • Fix: Gate emission on an explicit user-initiated signal (set a userInitiatedRef in the submit handler that drives setSearchedConfig, clear after emit) or skip emission while isLive is true.
    • correctness, adversarial, julik-frontend-races, previous-comments

🟡 P2 -- recommended

  • packages/app/src/DBSearchPage.tsx:874 -- Several QUERY_KEY_PREFIX queries (heatmap, histogram, total-count, paginated rows, explain) enable on staggered ticks, so isAnyQueryFetching can dip to 0 mid-search; the truthy branch then re-runs searchStartTimeRef.current = performance.now() unconditionally, splits one logical search into two emitted events, and reports two undercount latencies instead of one wall-clock measurement.
    • Fix: Guard the start assignment with if (searchStartTimeRef.current == null) so the anchor survives intermediate query bursts within a single user-initiated search.
    • adversarial, julik-frontend-races, correctness
  • packages/app/src/__tests__/useSearchTelemetry.test.tsx -- The new SearchNumRows UI surface (modal open via IconCode, isSearching vs searchElapsedMs != null branches, hasData gating, sqlConfig ?? config fallback, and the !numRows -> numRows == null semantic change that now renders 'Scanned Rows: 0' for an empty-string rows) has no render coverage, and the hook tests assert latency_ms only as expect.any(Number) plus >= 0, which would accept a NaN from an uninitialized-ref bug.
    • Fix: Add render tests for SearchNumRows covering the load/error/no-data branches, the modal open/close path, and the sqlConfig fallback; stub performance.now so the hook tests assert an exact integer latency_ms; add cases for (fetching:false, source:'a') -> (fetching:false, source:'b'), multi-cycle true->false->true->false, and mount-with-already-fetching.
    • testing, julik-frontend-races, adversarial
  • packages/app/src/DBSearchPage.tsx:2241 -- Both render sites pass sqlConfig={histogramTimeChartConfig}, so the "Generated SQL" modal renders the histogram aggregate query while sitting next to a Scanned Rows: N counter that comes from useExplainQuery(config) on the rows query — a user who clicks the code icon reads SQL that is not the statement producing the displayed count.
    • Fix: Either pass the rows config as sqlConfig, rename the modal title to make it clear it is the timeline-chart SQL (e.g. Generated Histogram SQL), or render both queries in the modal with labeled sections.
    • correctness, adversarial
  • packages/app/src/DBSearchPage.tsx:377 -- The display now uses Number(numRows).toLocaleString() instead of the prior Number.parseInt(numRows)?.toLocaleString(), and the hasData gate widened from !numRows to numRows != null, so an empty-string rows value from EXPLAIN ESTIMATE now renders Scanned Rows: 0 (was suppressed) and any non-numeric string renders Scanned Rows: NaN.
    • Fix: Compute const n = Number(numRows); and gate on Number.isFinite(n), rendering the empty branch (and hiding the divider/icon) when it fails.
    • adversarial, testing, correctness
🔵 P3 nitpicks (11)
  • packages/app/src/DBSearchPage.tsx:859 -- useSearchTelemetry is exported from a 2518-line page module, exceeding the codified Max 300 lines rule (agent_docs/code_style.md, CLAUDE.md), and the test must jest.mock('@/layout', ...) purely because importing the hook drags in the entire page's transitive graph.
    • Fix: Move the hook to packages/app/src/hooks/useSearchTelemetry.ts alongside useExplainQuery.tsx and drop the @/layout mock from the test.
    • maintainability, project-standards, kieran-typescript, testing
  • packages/app/src/DBSearchPage.tsx:387 -- formatDurationMs(searchElapsedMs!) survives only because of the sibling (isSearching || searchElapsedMs != null) guard; any future restructure of the ternary silently dereferences null.
    • Fix: Narrow with searchElapsedMs == null ? 'Elapsed Time: ...' : Elapsed Time: ${formatDurationMs(searchElapsedMs)} `` or capture into a local const after an early `if`.
    • maintainability, kieran-typescript, correctness, julik-frontend-races
  • packages/app/src/DBSearchPage.tsx:2241 -- sqlConfig={histogramTimeChartConfig ?? undefined} is a no-op because the prop is already optional and histogramTimeChartConfig is already typed T | undefined.
    • Fix: Pass sqlConfig={histogramTimeChartConfig} directly at both call sites.
    • maintainability, kieran-typescript
  • packages/app/src/DBSearchPage.tsx:2236 -- SearchNumRows is rendered twice with identical prop shapes (the pattern and results analysis modes), so any future prop change requires two synchronized edits and a one-side miss will silently desynchronize the two modes.
    • Fix: Lift the shared props into one object (or hoist the element into a single variable) and spread it at both render sites.
    • maintainability, adversarial
  • packages/app/src/DBSearchPage.tsx:379 -- searchElapsedMs is only cleared on the next isAnyQueryFetching:true transition, so the "Elapsed Time" and "Scanned Rows" text linger after the user mutates filters but before pressing Search again, mixing stale latency with a result set that no longer matches the displayed inputs.
    • Fix: Reset searchElapsedMs to null when searchedConfig identity changes, or key the displayed elapsed value to the same config that gates hasData.
    • adversarial
  • packages/app/src/__tests__/useSearchTelemetry.test.tsx:12 -- The mock addAction: (...args: unknown[]) => mockAddAction(...args) erases HyperDX.addAction's real signature, so a callsite that renames a key (latencyMs for latency_ms) or swaps arity passes the type check.
    • Fix: Assign mockAddAction = jest.fn<ReturnType<typeof HyperDX.addAction>, Parameters<typeof HyperDX.addAction>>() as the addAction value directly, with no unknown[] wrapper.
    • kieran-typescript, testing
  • packages/app/src/DBSearchPage.tsx:1387 -- chartConfig?.source ?? null coerces string | undefined to string | null only to be re-coerced to '' via sourceId ?? '' inside the hook, mixing both nullish sentinels for one concept.
    • Fix: Type the hook parameter as string | undefined and pass chartConfig?.source directly.
    • kieran-typescript
  • packages/app/src/DBSearchPage.tsx:862 -- useSearchTelemetry lacks an explicit return-type annotation, so the public hook shape is inference-only and a future edit could silently widen { searchElapsedMs: number | null }.
    • Fix: Annotate ): { searchElapsedMs: number | null } on the exported hook.
    • kieran-typescript
  • packages/app/src/DBSearchPage.tsx:874 -- On a deep-linked search URL where queries kick off during the first render, searchStartTimeRef.current = performance.now() is set in the mount-effect after the queries already started, so the reported latency under-counts the cold-load duration by a small consistent delta.
    • Fix: Capture the start anchor at the search-submit / setSearchedConfig callsite instead of inferring from useIsFetching.
    • julik-frontend-races
  • packages/app/src/__tests__/useSearchTelemetry.test.tsx:9 -- The jest.mock('@/layout', ...) shim only exists because useSearchTelemetry is imported from DBSearchPage, coupling a focused unit test to a 2518-line page module.
    • Fix: Drop the mock after extracting the hook to its own file under packages/app/src/hooks/.
    • testing
  • packages/api/src/mcp/tools/query/helpers.ts -- The UI now exposes the generated SQL and the wall-clock latency next to search results, but formatQueryResult returns only { result, note?, hint? } to MCP, so an agent driving hyperdx_search cannot observe the SQL or the latency users now see — an agent-native parity gap.
    • Fix: Surface the rendered SQL (e.g. from renderChartConfig in clickhouseClient.queryChartConfig) and the measured latency_ms / result.statistics.elapsed in the tool response, gated by an opt-in flag if payload size is a concern.
    • agent-native

Reviewers (10): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, adversarial, kieran-typescript, julik-frontend-races, previous-comments.

Testing gaps:

  • No test covers the (fetching:false, source:'a') -> (fetching:false, source:'b') two-render path that triggers the stale-source double-emit (Issue A).
  • No test simulates live-tail polling cycles to characterize phantom-event volume under default isLive: true (Issue B).
  • No test exercises true -> false -> true -> false within a single search to characterize emission count under aggregated useIsFetching (Issue C).
  • No test stubs performance.now, so latency_ms is asserted as expect.any(Number) and would pass for NaN.
  • No render tests for the new SearchNumRows branches, the modal open/close flow, the sqlConfig fallback, or the numRows === '0' semantic change.

Shows scanned row count and elapsed query time inline on the search page.
Adds a code icon button that opens a modal with the formatted generated SQL.
@vinzee vinzee force-pushed the va/stats_for_nerds branch from e1c22e2 to 37c6108 Compare May 30, 2026 01:20
@karl-power karl-power self-requested a review June 1, 2026 14:27
@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 1, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jun 1, 2026 4:22pm
hyperdx-storybook Ready Ready Preview, Comment Jun 1, 2026 4:22pm

Request Review

Copy link
Copy Markdown
Contributor

@karl-power karl-power left a comment

Choose a reason for hiding this comment

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

Thanks @vinzee - nice features. Can you check the P0/P1 issues from the deep review and update if needed?

I also noticed this flicker on the generated SQL after each poll

Untitled.mov

Please also add a changeset when ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants