Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 98884b0. Configure here.
Contributor
size-limit report 📦
|
Lms24
changed the title
ui span names
Lms24
force-pushed
the
feat/ui-span-name-conventions
branch
from
September 22, 2026 14:49
215d521 to
1defdc9
Compare
Lms24
commented
Sep 22, 2026
| const pageloadSpanId = inpSpan.parent_span_id; | ||
|
|
||
| expect(inpSpan).toEqual({ | ||
| name: 'body > NormalButton', |
Member
Author
There was a problem hiding this comment.
this is fine IMHO because for this test (plus the ones below), trace lifecycle is static but we nevertheless emit inp as a v2 span. So the naming change doesn't have to apply here (logaf l though but we can fix later if it comes up at all)
| [SENTRY_SEGMENT_NAME]: { | ||
| type: 'string', | ||
| value: 'Pageload', | ||
| value: 'Click', |
Member
Author
There was a problem hiding this comment.
this fixes a bug previously, since we took the "Pageload" low card. name from the active pageload span previously. "Click" is better than that IMHO
Lms24
marked this pull request as ready for review
September 22, 2026 15:34
Lms24
requested review from
logaretm,
msonnb,
nicohrubec and
s1gr1d
and removed request for
a team
September 22, 2026 15:34
Member
Author
|
yo bugbot review |
logaretm
reviewed
Sep 22, 2026
logaretm
left a comment
logaretm
left a comment
Member
There was a problem hiding this comment.
I think I prefer to align with the API's own naming rather than consolidate them.
I explain a bit in this PR:
logaretm
reviewed
Sep 22, 2026
Member
There was a problem hiding this comment.
A bit of a nit, handling unknown the case of the INP target is interesting.
in #24573 which overlaps a bit with this (it's fine it also adds a new attribute so no worries on the inp attr), but in that PR i chose to omit.
Question for the review: should the "unknown" case be reported as or as unknown without the brackets, or omit it entirely? I chose to omit it for now.
Thoughts on this? I don't have a strong opinion here, I listed all 3 options in that PR.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
logaretm
reviewed
Sep 22, 2026
| [SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME]: entry?.duration ?? inpValue, | ||
| }; | ||
| if (selector) { | ||
| attributes['browser.web_vital.inp.target'] = selector; |
Member
There was a problem hiding this comment.
m: We got some risk here, name value changed to be the component name so it would mess up what develop does after #24573 was merged.
So to preserve the values correctly we need to:
- Drop the
if (selector)check block to preserveunknownhandling. - Replace
namedown there withselectorto make sure INP targets still has the selector.
entry && selector !== UNKNOWN_ELEMENT_NAME && (attributes['browser.web_vital.inp.target'] = selector);Otherwise LGTM!
When span streaming is enabled, UI component, interaction, action, and web-vital spans use low-cardinality names instead of DOM paths or angle-bracketed component names. Co-Authored-By: Cursor <cursoragent@cursor.com>
The E2E suites still expected the pre-convention span shape. Streamed INP spans on unannotated elements are named `Click` now, with the DOM path on `ui.element.target`, and component spans carry `ui.component_name` in both trace lifecycles, which broke the static apps asserting an exact `data` map. Also assert the spotlight filter against `ui.element.target` directly instead of falling back to the span name, so the test pins one shape rather than passing either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The suites checked the new low-cardinality span names but not the attributes that now carry what the names gave up, so a regression that dropped `ui.component_name` or `sentry.description` would have gone unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ebase Rebasing onto develop replayed this branch over "Report the INP target and interaction type as attributes" (#24573). Both sides had added an attributes block to `_sendInpSpan`, and the auto-merge kept both, leaving a duplicate `const attributes` declaration and a duplicate `browser.web_vital.inp.target` assignment. Git raised no conflict for either. Keep that PR's attribute line and drop ours, with one change: it read the selector off the span name, which no longer holds one here. Under span streaming the name is the component name or the op's fallback, so the attribute now reads `selector` directly. Omitting it for a missing entry or an unresolved element is unchanged. The same merge duplicated the attribute's key inside single object literals across six test files, which is dead code in a JS object. Drop the second of each pair, and drop our negative assertion for the entry-less case, which that PR's own test covers more precisely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lms24
force-pushed
the
feat/ui-span-name-conventions
branch
from
September 22, 2026 19:55
4f50163 to
c1a59f1
Compare
`browser.web_vital.inp.target` is new on `ui.interaction.click` spans, since the selector no longer fits in the low-cardinality span name. It was set from `htmlTreeAsString(entry.target)` unconditionally, which yields `<unknown>` for a null target and an empty string for a node without a tag name. `PerformanceEventTiming.target` is null whenever the node is disconnected or in a shadow tree, so a handler that removes its own element, or any click inside a web component, would have tagged the span with a value nobody can filter on. Guard the attribute the way `_sendInpSpan` already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
logaretm
approved these changes
Sep 22, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Emits low cardinality span names for
uispans when span streaming is enabled. This includes:mostly, we just override description inference here because we can't really model the descriptions with just attributes/templates in a general enough way. I think this is acceptable for these spans.
conventions are still pending: getsentry/sentry-conventions#643, so I'll only merge this once conventions are approved