Skip to content

Conversation

@pulpdrew
Copy link
Contributor

@pulpdrew pulpdrew commented Oct 17, 2025

Closes HDX-2593

Summary

This PR adds a source's displayedTimestampValueExpression (if one exists) to the default order by on the search page.

Motivation

In schemas like our default otel_logs table, there are two timestamp columns: TimestampTime DateTime (1-second precision) and a Timestamp DateTime64(9) (nanosecond precision). TimestampTime is preferred for filtering because it is more granular and in the primary key. However, ordering by TimestampTime alone results in an arbitrary order of events within each second:

Screenshot 2025-10-17 at 2 28 50 PM

Details

The HyperDX source configuration form already supports configuring a 'Displayed Timestamp Column" for a log source. This PR adds the same option for Trace sources. This field is inferred from the otel logs and traces schemas as Timestamp.

Screenshot 2025-10-17 at 2 30 13 PM

If the source has a displayed timestamp column configured, and if this column is different than the source's timestamp value expression, then this field will be added to the default order by which is generated for the search page. This results in a more precise ordering of the events in the logs table within each second:

Screenshot 2025-10-17 at 2 33 16 PM

@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2025

🦋 Changeset detected

Latest commit: b7489fa

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

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api 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
Copy link

vercel bot commented Oct 17, 2025

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

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Oct 27, 2025 1:45pm

placeholder="SpanName"
/>
</FormRow>
<FormRow
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field was already being inferred for the default OTEL trace table, now we allow the user to modify it if needed.

@claude
Copy link

claude bot commented Oct 17, 2025

PR Review - feat: Include displayed timestamp in default order by

Overall Assessment: Good implementation with comprehensive tests. Minor issue found.

Issues Found:

  • ⚠️ Empty ORDER BY expression (DBSearchPage.tsx:540-554): When timestampExpr is empty/null, getFirstTimestampValueExpression returns empty string, resulting in ORDER BY clause DESC (space + DESC only). While existing tests expect this behavior, it creates invalid SQL. → Add guard to handle empty firstTimestampValueExpression before constructing ORDER BY clause, or ensure timestampValueExpression is always non-empty at the source validation level.

Security Note:

✅ SQL injection risk is mitigated - displayedTimestampValueExpression is validated via Zod schema and requires authentication/team access (consistent with existing timestampValueExpression handling).

Positive Observations:

  • ✅ Comprehensive test coverage with 20+ test cases
  • ✅ Proper handling of edge cases (trimming, empty strings, duplicates)
  • ✅ Consistent with existing patterns in codebase
  • ✅ Added field to trace sources (feature parity with log sources)
  • ✅ Clear PR description with motivation and screenshots

@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2025

E2E Test Results

All tests passed • 26 passed • 3 skipped • 232s

Status Count
✅ Passed 26
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 3

View full report →

@pulpdrew pulpdrew marked this pull request as draft October 17, 2025 18:41
@pulpdrew pulpdrew force-pushed the drew/order-by-displayed-timestamp branch from 3e718c9 to 383704b Compare October 17, 2025 18:55
@pulpdrew pulpdrew force-pushed the drew/order-by-displayed-timestamp branch from 383704b to 7445bbf Compare October 17, 2025 18:59
@pulpdrew pulpdrew marked this pull request as ready for review October 17, 2025 19:15
@pulpdrew pulpdrew requested a review from knudtty October 17, 2025 19:15
@brandon-pereira
Copy link
Member

Maybe its a bug maybe not,but the timestamp column gets changed when sorting

Screen.Recording.2025-10-17.at.3.39.37.PM.mov

@pulpdrew
Copy link
Contributor Author

pulpdrew commented Oct 21, 2025

Maybe its a bug maybe not, but the timestamp column gets changed when sorting

That's a good point. Though it seems that the default order by is restored if the user clicks on the table header again to unsort by the column. And this is existing behavior whenever the default order by is not the Timestamp column (which is almost always for log sources, since the timestamp column is typically TimestampTime and optimized schema will often have a default order by something like toStartOfMinute(TimestampTime), TimestampTime.

I think the solution would be to determine if the user is clicking on a timestamp-type column which is included in the default order by, and in that case override the selected sort column with the default optimized order by. That could be a good enhancement.

Copy link
Contributor

@knudtty knudtty left a comment

Choose a reason for hiding this comment

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

Great!

@kodiakhq kodiakhq bot merged commit 21614b9 into main Oct 27, 2025
8 of 9 checks passed
@kodiakhq kodiakhq bot deleted the drew/order-by-displayed-timestamp branch October 27, 2025 13:44
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.

4 participants