feat(browser): Emit low cardinality navigation span names - #23550
Conversation
6eeaeb1 to
97991d2
Compare
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| span.updateName(pathname); | ||
| // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. | ||
| const client = getClient(); | ||
| span.updateName(client && hasSpanStreamingEnabled(client) ? NAVIGATION_SPAN_NAME_FALLBACK : pathname); |
There was a problem hiding this comment.
Avoidable getClient in utils
Low Severity
This is more an “is this necessary” check than a hard violation: updateNavigationSpanUrlFromLocation newly calls getClient() to gate the streaming name. Callers already have or can receive a client, so threading it through would avoid creating/relying on a current client in multi-client setups.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 97991d2. Configure here.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Nice, thank you! Clanker comments are probably false positives but worth double checking.
There was a problem hiding this comment.
no action required, just comment: Angular is a pretty bad offender in terms of getting early route parameterization :( Maybe something to revisit some day, though their closed approach to the build system (by default) probably makes a manifest injection like in other frameworks less practical.
Not pretty but I think this is the best we can do here.
| span.updateName(pathname); | ||
| // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. | ||
| const client = getClient(); | ||
| span.updateName(client && hasSpanStreamingEnabled(client) ? NAVIGATION_SPAN_NAME_FALLBACK : pathname); |
Names navigation spans `Navigation` when span streaming is enabled and the SDK has no parameterized route for them. Names in static mode are unchanged.
97991d2 to
ec31f77
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ec31f77. Configure here.
| name: | ||
| source === 'route' || !hasSpanStreamingEnabled(client) | ||
| ? placeholderEntry.routeName | ||
| : NAVIGATION_SPAN_NAME_FALLBACK, |
There was a problem hiding this comment.
Scope transaction name drops router path
Medium Severity
Several React/Remix navigation starts now pass the Navigation fallback but omit the url option. startBrowserTracingNavigationSpan then sets scope.transactionName from WINDOW.location.pathname, which is the previous page during some transitions and the document path (/) for hash/memory routers, so errors group away from the actual route.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ec31f77. Configure here.
… route chain `getParameterizedRoute` only looked at the leaf match and fell back to the raw `location.pathname` when it had no `route.path`. Index and layout routes have none, so a library-mode navigation to `/users/123` under an index child was named `/users/123` with `sentry.segment.name.source: 'route'`, putting a raw URL back into a streamed span name. Nested children were also mis-named after their own relative segment (`/edit` instead of `/users/:id/edit`). Joining the matched chain resolves both. Framework-mode route configs are unaffected: `prefix()` flattens the full path onto each child, so the joined template equals the leaf path it produced before. When nothing matches at all, streamed spans now keep their `Navigation`/`Pageload` name instead of taking the URL; static-lifecycle names are unchanged.
2 participants
Names navigation spans
Navigationwhen span streaming is enabled and the SDK has no parameterized route for them. Names in static mode are unchanged.The scope's transaction name keeps the URL, since that's what error events group by.
navigation.redirectspans share the code path so they get the fallback too, which imo is right.closes #23522