Skip to content

Conversation

@pulpdrew
Copy link
Contributor

@pulpdrew pulpdrew commented Nov 12, 2025

Ref HDX-2752

Summary

This PR adds a new Source setting which specifies additional Span-Level attributes which should be displayed above the Trace Waterfall. The attributes may be specified for both Log and Trace kind sources.

These are displayed on the trace panel rather than at the top of the side panel because they are trace level (from any span in the trace), whereas the top of the side panel shows information specific to the span. In a followup PR, we'll add row-level attributes, which will appear at the top of the side panel.

Notes:

  1. The attributes are pulled from the log and trace source (if configured for each) despite the search page only being set to query one of those sources. If an attribute value comes from the log source while the trace source is currently configured on the search page, the attribute tag will not offer the option to show the "Add to search" button and instead will just show a "search this value" option, which navigates the search page to search the value in the correct source.

Demo

First, source is configured with highlighted attributes:
Screenshot 2025-11-14 at 3 02 25 PM

Values for those attributes within the trace show up on the trace panel above the waterfall:
Screenshot 2025-11-14 at 3 02 59 PM

Values are searchable when clicked, and if a lucene version of the property is provided, the lucene version will be used in the search box
Screenshot 2025-11-14 at 3 03 10 PM

@changeset-bot
Copy link

changeset-bot bot commented Nov 12, 2025

🦋 Changeset detected

Latest commit: 31cb5e9

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

This PR includes changesets to release 3 packages
Name Type
@hyperdx/common-utils Patch
@hyperdx/api Patch
@hyperdx/app 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 Nov 12, 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 Nov 18, 2025 7:34pm

@claude
Copy link

claude bot commented Nov 12, 2025

PR Review - feat: Add custom trace-level attributes above trace waterfall

Critical Issues

None found

Code Quality Issues

  • SQL Injection Risk in EventTag.tsx:48 - Using SqlString.format with SqlString.raw(name) allows raw SQL. Validate name parameter or use prepared statements
  • Missing error boundary in DBHighlightedAttributesList - Wrap component in try-catch or error boundary to prevent entire panel crash if attribute rendering fails
  • Deprecated props removed without migration path in NetworkPropertyPanel.tsx:28,29 - Removed onPropertyAddClick and generateSearchUrl props. Verify no breaking changes to existing usages
  • useMemo missing dependency in DBTraceWaterfallChart.tsx:297 - dateRangeStartInclusive marked as added dependency but not verified in implementation

Observations

  • Comprehensive test coverage (480 lines of tests)
  • Proper TypeScript typing throughout
  • Good separation of concerns (utility functions in separate file)
  • Zod schema validation for new fields
  • Handles edge cases (null, undefined, empty strings)

Recommendations

  1. Add input validation for SQL expressions before using SqlString.raw()
  2. Consider adding error boundaries around new components
  3. Verify NetworkPropertyPanel change does not break existing features

@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2025

E2E Test Results

All tests passed • 39 passed • 3 skipped • 395s

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

View full report →


// Currently we can't distinguish null or 'null'
if (value === 'null') {
if (value == null || value === 'null') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSON column values were being returned as actual null values here, which then broke the value.length check below.

Copy link
Member

Choose a reason for hiding this comment

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

nit: it would be helpful to add examples in the comment (for null case)

@pulpdrew pulpdrew marked this pull request as ready for review November 13, 2025 13:24
@pulpdrew pulpdrew requested review from a team and wrn14897 and removed request for a team November 13, 2025 13:24
React.useEffect(() => {
if (
!areFiltersEqual(filters, parsedQuery.filters) &&
Object.values(parsedQuery.filters).length > 0
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 check was preventing us from clearing the filter state if all filters were removed from the URL params (which happens when changing sources due to clicking on an attribute from a different source)

@pulpdrew pulpdrew marked this pull request as ready for review November 14, 2025 21:08
statusCodeExpression: String,
statusMessageExpression: String,
spanEventsValueExpression: String,
highlightedTraceAttributeExpressions: {
Copy link
Member

Choose a reason for hiding this comment

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

does the ordering matter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ordering of elements in this array? No I don't think it matters - the Event Tags are sorted before they're displayed client-side. Why?

Copy link
Member

Choose a reason for hiding this comment

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

since it's an array type, it makes sense to sort them before deploying, or we can sort them during query time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way we end up sorting them on the UI can depend on the values that are queried for each attribute and for each trace. There also should not be very many attributes (dozens at most, I would guess) so I don't think it should be any issue with sorting them on the client side when used.

Copy link
Member

@wrn14897 wrn14897 left a comment

Choose a reason for hiding this comment

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

lgtm

@kodiakhq kodiakhq bot merged commit c4915d4 into main Nov 18, 2025
10 of 11 checks passed
@kodiakhq kodiakhq bot deleted the drew/trace-attribute-tags branch November 18, 2025 19:34
kodiakhq bot pushed a commit that referenced this pull request Nov 19, 2025
Closes HDX-2776
Closes HDX-2752

# Summary

This PR extends the custom attributes feature from #1356 to support custom attributes on individual rows (rather than entire traces). Further, attributes which are URLs are now rendered as links, supporting use-cases which require building custom external links from queried data.

## Demo

Source Configuration:

<img width="972" height="649" alt="Screenshot 2025-11-18 at 6 01 55 PM" src="https://github.com/user-attachments/assets/42e9cc95-d9e3-4155-8e25-58e4dcf0b787" />

Side panel:
<img width="1587" height="511" alt="Screenshot 2025-11-18 at 6 02 12 PM" src="https://github.com/user-attachments/assets/3a1d7683-eb79-4925-99fe-4e2d7cdad78f" />

Inferred link:
<img width="727" height="173" alt="Screenshot 2025-11-18 at 6 02 32 PM" src="https://github.com/user-attachments/assets/68f42f95-598c-4b30-b616-9556e32945bf" />


<details>
<summary>logview attribute definition</summary>

```sql
if(
  NOT empty(TraceId) AND NOT empty(SpanId) AND NOT empty(ServiceName),
  concat('https://logview.com?q=',
    'trace_id=', TraceId,
    '&span_id=', SpanId,
    '&service=', ServiceName,
    '&start_time=', formatDateTime(Timestamp - INTERVAL 1 SECOND, '%FT%T.%fZ'),
    '&end_time=', formatDateTime(Timestamp + Duration/1e9 + INTERVAL 1 SECOND, '%FT%T.%fZ')
  ),
  ''
)
```
</details>
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