feat: Emit low-cardinality http.client span names for fetch and XHR - #23682
Open
chargome wants to merge 6 commits into
Open
feat: Emit low-cardinality http.client span names for fetch and XHR#23682chargome wants to merge 6 commits into
chargome wants to merge 6 commits into
Conversation
With span streaming, `http.client` spans are named `{method} {url.domain}` instead of
`{method} {sanitized-url}`, falling back to the method alone when there is no domain.
Covers `instrumentFetchRequest` in `@sentry/core`, browser XHR, and `http.client.stream`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
size-limit report 📦
|
`parseUrl` returns the raw authority, so `user:pass@host:port` could reach the XHR span name and `server.address`. Strip it, and set `url.domain` on browser fetch and XHR spans so the value in the streamed name is filterable. `fetchStreamPerformance` now uses the client it receives in `setup` instead of `getClient()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bdcfbb5. Configure here.
chargome
marked this pull request as ready for review
August 27, 2026 16:13
chargome
requested review from
logaretm and
msonnb
and removed request for
a team
August 27, 2026 16:13
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
JPeer264
reviewed
Aug 27, 2026
| const streamedName = domain ? `${method} ${domain}` : method; | ||
| const streamSpan = startInactiveSpan({ | ||
| name: `${method} ${sanitizedUrl}`, | ||
| name: hasSpanStreamingEnabled(client) ? streamedName : `${method} ${sanitizedUrl}`, |
Member
There was a problem hiding this comment.
q: The sanitizedUrl also takes care of "data: URLs", should the streamedName solely be domains?
Edit: seems like the PR description covers that part
| // name or attribute. | ||
| const host = parsedUrl?.host?.replace(/^.*@/, ''); | ||
| // Unlike `server.address`, `url.domain` excludes the port. | ||
| const domain = host?.replace(/:\d+$/, ''); |
Member
There was a problem hiding this comment.
q: This works differently than in the fetchStreamPerformance integration, is that intended?
The other code I mean:
const domain = parsedUrl && !isURLObjectRelative(parsedUrl) ? parsedUrl.hostname : undefined;
JPeer264
approved these changes
Aug 27, 2026
Lms24
reviewed
Aug 31, 2026
Member
There was a problem hiding this comment.
One request: Can we add a few more tests for relative URLs?
For browser, we could add a streaming version of of /dev-packages/browser-integration-tests/suites/tracing/request/fetch-relative-url/test.ts.
For node, I think having one or two integration tests would also be a good thing. Maybe one relative, one absolute? sorry, this is covered in the other PR, I should have checked that first.
Besides test, my only other concern was loosing information on relative URLs (see comment)
Relative URLs previously left `http.client` and `http.client.stream` spans named after the method alone, even in browsers where the page origin is a perfectly good domain to resolve against. `instrumentFetchRequest` now takes a `urlBase` that the browser fills with the page origin, so a relative fetch is named `GET app.example.com` instead of `GET`. XHR and stream spans resolve the same way. Consolidate the three different domain derivations (two regexes over `parseUrl().host`, and an `isURLObjectRelative` check over a URL object) into one `getUrlDomain` helper. `URL.hostname` drops userinfo and the port structurally, so the credential stripping no longer needs a regex. Keep the GraphQL operation on outgoing request spans as `graphql.operation.name` and `graphql.operation.type`. It only ever lived in the span name, so the low-cardinality rename would otherwise have dropped it from the span entirely. Refs #23527 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XvHhCTmsaPtA8vMDuHXSwd
…82-feed # Conflicts: # MIGRATION.md # packages/browser/src/integrations/fetchStreamPerformance.ts # packages/browser/src/tracing/request.ts # packages/core/src/fetch.ts
`waitForStreamedSpans` resolves on the first envelope matching its predicate, and the predicate only waited for the three `http.client` spans. The pageload span often shares that envelope but does not have to, so asserting `parent_span_id` against it failed whenever it landed in a later one — the flakiness check caught this on repeat 10. Parenting is already covered by the `fetch-streamed` and `xhr-streamed` suites, so drop it here and assert only the span name and URL attributes these tests exist for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XvHhCTmsaPtA8vMDuHXSwd
Lms24
approved these changes
Aug 31, 2026
|
|
||
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| integrations: [Sentry.browserTracingIntegration(), Sentry.spanStreamingIntegration()], |
Member
There was a problem hiding this comment.
super-l: shouldn't be necessary anymore in v11
Suggested change
| integrations: [Sentry.browserTracingIntegration(), Sentry.spanStreamingIntegration()], | |
| integrations: [Sentry.browserTracingIntegration()], |
|
|
||
| Sentry.init({ | ||
| dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
| integrations: [Sentry.browserTracingIntegration(), Sentry.spanStreamingIntegration()], |
Member
There was a problem hiding this comment.
super-l: shouldn't be necessary anymore in v11
Suggested change
| integrations: [Sentry.browserTracingIntegration(), Sentry.spanStreamingIntegration()], | |
| integrations: [Sentry.browserTracingIntegration()] |
| */ | ||
| export function getUrlDomain(url: string, base?: string): string | undefined { | ||
| try { | ||
| return new URL(url, base).hostname || undefined; |
Member
There was a problem hiding this comment.
super-l: can we reuse our parseUrl function(s) for this? not sure if anything speaks against this
Member
There was a problem hiding this comment.
ah the base URL... this is probably fine to leave as-is although I could have sworn we have a helper somewhere that does this already. Anyway, feel free to ignore
Starting a browser span adds the integration on its own, so listing it in `Sentry.init` is no longer needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XvHhCTmsaPtA8vMDuHXSwd
chargome
enabled auto-merge (squash)
August 31, 2026 16:11
chargome
force-pushed
the
charlygomez/js-3412-http-client-fetch-xhr
branch
from
August 31, 2026 16:12
4886648 to
5385238
Compare
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
3 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.
With span streaming,
http.clientspans are named{method} {url.domain}(GET api.example.com)instead of
{method} {sanitized-url}, falling back to the method alone when there is no domain(relative and data URLs).
traceLifecycle: 'static'is unchanged.Covers
instrumentFetchRequestin@sentry/core(browser, bun, cloudflare, vercel-edge), browserXHR, and
http.client.stream.node:httpand undici follow separately.Spans also get a
url.domainattribute in both lifecycles, so the value in the name staysfilterable — same as the
resource.*port.Ref #23527