ref(core)!: Replace deprecated http.* and net.* span attributes on HTTPS spans - #23423
Draft
msonnb wants to merge 1 commit into
Draft
ref(core)!: Replace deprecated http.* and net.* span attributes on HTTPS spans#23423msonnb wants to merge 1 commit into
http.* and net.* span attributes on HTTPS spans#23423msonnb wants to merge 1 commit into
Conversation
…n HTTP spans Part of the v11 migration away from attributes `@sentry/conventions` marks deprecated. Scoped to HTTP spans; the `net.peer.*` attributes on database and messaging spans are migrated separately. Straight renames: `http.method` -> `http.request.method`, `http.status_code` -> `http.response.status_code`, `http.scheme` -> `url.scheme`, `http.user_agent` -> `user_agent.original`, `http.flavor` -> `network.protocol.version`, `http.client_ip` -> `client.address`, `http.response_content_length` -> `http.response.body.size`, `http.response_transfer_size` -> `http.response.size`, `net.peer.ip` -> `network.peer.address`, `net.host.ip` -> `network.local.address`, `net.transport` -> `network.transport`, and `url.same_origin` -> `http.request.same_origin`. `http.request_content_length*`, `http.response_content_length_uncompressed` and `http.status_text` are left alone — they are not in `@sentry/conventions` at all, so they have no replacement to move to. Three cases needed more than a rename: - `http.host`, `net.host.name` and `net.peer.name` all map to `server.address`, but do not hold the same thing. On a server span `server.*` describes the server and the socket peer is the *client*, so the remote address/port go to `network.peer.*` and the local socket to `network.local.*`. `server.address` comes from the `Host` header with the port split out into `server.port` by a new `splitHostHeader` helper, shared by the core and node span builders so the two stay identical. The raw header is still on `http.request.header.host`. - `http.target` carried pathname *and* query, while `url.path` is the pathname only. The core server span set neither `url.query` nor `url.fragment`, so dropping `http.target` would have lost the query — it now sets both, which the node server span already did. - Consumers that matched on `http.target` were repointed at `url.path`: the react-router low-quality-transaction filter and the TanStack Start tunnel-route filter, both `ignoreSpans` rules against our own spans that would otherwise have silently stopped matching. The Next.js readers keep `http.target` as a fallback behind a `url.path` primary, since they also see spans from a user's own OpenTelemetry instrumentation. All other read-side fallbacks are untouched for the same reason. `SanitizedRequestData`, the shape backing `http` breadcrumb data, now keys the method as `http.request.method`. Span attributes in the touched files are now imported from `@sentry/conventions/attributes` rather than written as string literals. That is what surfaced `url.same_origin` as deprecated; as a literal it was invisible. `no-unfiltered-url-attributes` no longer guards `http.target`: nothing sets it, and its replacement `url.path` is a bare pathname with no query to filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
bugbot run |
Contributor
size-limit report 📦
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 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 701694c. Configure here.
| return { hostname: match[1], port: port <= 65535 ? port : undefined }; | ||
| } | ||
| return { hostname: host || 'localhost', port: undefined }; | ||
| } |
There was a problem hiding this comment.
Host header parser mishandles IPv6
Medium Severity
splitHostHeader splits on the last : plus digits, so an IPv6 Host value such as [::1]:8080 keeps the brackets in server.address, and a missing header becomes localhost. Both values are wrong for server.address on every incoming server span.
Reviewed by Cursor Bugbot for commit 701694c. Configure here.
| // `Host` header land on `server.address`; the header wins when both are set. | ||
| // `url.path`, `url.query` and `http.request.method` come from `attributes` below, which is why | ||
| // the old `http.target` (path plus query) has no separate replacement here. | ||
| [SERVER_ADDRESS]: request.getHeader('host') ?? request.host, |
There was a problem hiding this comment.
Client spans embed port in address
Medium Severity
Outgoing HTTP spans copy the Host header or URL.host into server.address, so the port stays in the address and server.port is never set. Server spans already split those with splitHostHeader. Client traces therefore disagree with the server spans and with the server.* spec.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 701694c. Configure here.
| 'network.local.port': expect.any(Number), | ||
| 'network.peer.address': expect.any(String), | ||
| 'server.port': expect.any(Number), | ||
| 'http.response.status_code': 200, |
There was a problem hiding this comment.
E2E tests remap peer port wrongly
High Severity
These toEqual payloads list server.address twice and map net.peer.port to server.port. The second key wins, so the Host-header address is never asserted, and the extra network.peer.port the SDK still emits makes the strict equality fail. I flagged this because the testing conventions in the review rules require tests to assert the new attributes thoroughly.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 701694c. Configure here.
| return ( | ||
| transactionEvent.contexts?.trace?.data?.['http.target'] === `/generation-functions?metadataTitle=${testTitle}` | ||
| ); | ||
| return transactionEvent.contexts?.trace?.data?.['url.path'] === `/generation-functions?metadataTitle=${testTitle}`; |
There was a problem hiding this comment.
Tests match query on url.path
High Severity
waitForTransaction and the Next.js 15 tracesSampler now compare url.path to a string that still includes the query. url.path is pathname-only; http.target used to carry path plus query. Those waiters never match, so the tests time out. I flagged this because the testing conventions in the review rules require tests to cover the new attributes correctly.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 701694c. Configure here.
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
1 participant
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.
Straight renames
http.method->http.request.methodhttp.status_code->http.response.status_codehttp.scheme->url.schemehttp.user_agent->user_agent.originalhttp.flavor->network.protocol.versionhttp.client_ip->client.addresshttp.response_content_length->http.response.body.sizehttp.response_transfer_size->http.response.sizenet.peer.ip->network.peer.addressnet.host.ip->network.local.addressnet.transport->network.transporturl.same_origin->http.request.same_originChanges that are not renames
server.addressandserver.porthttp.host,net.host.nameandnet.peer.nameall map toserver.address. They do not hold the same value.On an HTTP server span,
server.*describes the server. The socket peer is the client. The remote address and port therefore go tonetwork.peer.*. The local socket goes tonetwork.local.*.server.addressnow comes from theHostheader. A newsplitHostHeaderhelper takes the port out of that header and puts it onserver.port. The span builders in@sentry/coreand@sentry/nodeboth call this helper, so both produce the same attributes.The unparsed
Hostheader stays available onhttp.request.header.host.url.queryandurl.fragmenton core server spanshttp.targetheld the pathname and the query.url.pathholds only the pathname.The server span in
@sentry/coreset neitherurl.querynorurl.fragment. Droppinghttp.targetwould therefore have lost the query. That span now sets both. The server span in@sentry/nodealready set both.Consumers that matched on
http.targetTwo
ignoreSpansrules match spans that the SDK itself emits. Both now match onurl.path:@sentry/react-router@sentry/tanstackstart-reactWithout this change, both rules would match no spans and report no error.
The readers in
@sentry/nextjsstill readhttp.target, but only after they readurl.path. These readers also receive spans from an OpenTelemetry instrumentation that the user set up, and that instrumentation still emits the old attributes. Every other read-side fallback stays for the same reason.SanitizedRequestDataThis type is the shape of
httpbreadcrumb data. It now useshttp.request.methodas the key for the request method.no-unfiltered-url-attributesThis lint rule no longer guards
http.target. The SDK no longer setshttp.target. Its replacementurl.pathholds a pathname without a query, so there is nothing to filter.part of #18895