-
Notifications
You must be signed in to change notification settings - Fork 329
feat: Add filter for root spans #1341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: e8dd6dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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.
|
464fb67 to
5adca3e
Compare
PR Review - Root Spans Filter✅ No critical issues found. The implementation is clean and follows project patterns well:
Minor enhancement opportunity (optional): |
E2E Test Results✅ All tests passed • 40 passed • 3 skipped • 303s
|
5adca3e to
3204693
Compare
| </Text> | ||
| </Tooltip> | ||
| } | ||
| onChange={() => setRootSpansOnly(!rootSpansOnly)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on directly setting the filters object as opposed to a specific property? Or does it not work there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did think about doing it that way, but decided against it because
- Our filter states are based on parsing SQL, so we'd need to ensure our filter is of the form
parentSpanId IN (''). This is probably fine, though less straightforward to me thanempty(parentSpanId) - Our UI filter states are based on the values parsed from the filters parameter, so without specifically handling the
parentSpanIdfilter when parsing, we'd end up withparentSpanIdin the filter sidebar. We could specifically handle that, but that didn't seem any better to me than using a separate param.
Curious if you see a major benefit to having this in the filters object instead though - we could do it if there is one!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm a bit split on this, that makes a lot of sense why it doesn't work well today. I can imagine in the future we have other filter settings that are just "preset filter combos" kind of thing (though no concrete use case yet). Might just be a more scalable solution long term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We chatted offline and decided to go with using filters instead. We will show the related filter as applied in the sidebar (like the image above) but with an (empty) label.
3204693 to
e8dd6dd
Compare

Closes HDX-2772
This PR adds a filter that allows for quickly viewing just root spans from a Trace source.
Notes: