feat: Emit low-cardinality http.server span names in framework SDKs - #23597
Conversation
size-limit report 📦
|
80e4f09 to
ef8061b
Compare
|
bugbot run |
![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 3 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 ef8061b. Configure here.
| name: | ||
| attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE] === 'route' || !client || !hasSpanStreamingEnabled(client) | ||
| ? `${request.method} ${routeName}` | ||
| : request.method?.toUpperCase() || HTTP_SPAN_NAME_FALLBACK, |
There was a problem hiding this comment.
Route source check uses removed attribute
High Severity
The streaming name gate checks SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, which is no longer set. Route metadata is written to SENTRY_SEGMENT_NAME_SOURCE instead, so the route branch never matches. With span streaming on, parameterized Bun and Nitro http.server spans collapse to just the method, and Cloudflare/Deno drop the low-cardinality GET / name.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ef8061b. Configure here.
| // to `${method} ${route}` once Next.js reports a route. | ||
| if (isRootSpan && spanAttributes?.[ATTR_NEXT_SPAN_TYPE] === 'BaseServer.handleRequest') { | ||
| const client = getClient(); | ||
| if (client && hasSpanStreamingEnabled(client)) { |
There was a problem hiding this comment.
Avoidable getClient in Next.js hooks
Low Severity
This is more an “is this necessary” check than a hard violation: the new spanStart rename calls getClient() even though a client is already in scope. On the edge path vercelEdgeInit returns that client, and on the server path handleOnSpanStart is registered on the same instance. A current-client lookup can pick the wrong client in a multi-client setup and skip or mis-apply the streaming rename.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit ef8061b. Configure here.
| expect.any(Function), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Feat PR lacks integration or E2E tests
Low Severity
This is a feat PR, but the new coverage is only unit tests (Astro, Cloudflare, React Router, SvelteKit). Framework span naming depends on real request/route lifecycle, so at least one integration or E2E test would lock in that routed http.server spans stay parameterized and unrouted ones fall back when streaming is on. Several touched packages (Bun, Deno, Elysia, Next.js, Nitro, Remix) have no new tests at all.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit ef8061b. Configure here.
865560a to
9b5e334
Compare
9b5e334 to
330935b
Compare
330935b to
2d61e3b
Compare
Applies the same span-streaming gate to the runtime and framework SDKs so no integration keeps a raw URL in an http.server span name. Requests that resolve to a route are unchanged. Three sites differ from the rest and are worth a closer look: remix reads its own span name back, sveltekit also renames SvelteKit's native root span, and nextjs renames in a `spanStart` hook because Next.js — not the SDK — creates that span. Refs #23527 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2d61e3b to
b37ecb0
Compare
1 participant
Applies the same span-streaming gate across the runtime and framework SDKs so no integration keeps a raw URL in an
http.serverspan name; routed requests are unchanged. Stacked on #23596.Worth a closer look: remix reads its own span name back, sveltekit also renames SvelteKit's native root span, and nextjs renames in a
spanStarthook because Next.js creates that span. Refs #23527