feat(core): Ensure url, http.query and http.fragment are directly added#22547
feat(core): Ensure url, http.query and http.fragment are directly added#22547mydea wants to merge 3 commits into
url, http.query and http.fragment are directly added#22547Conversation
mydea
changed the title
url, http.query and http.fragment are directly added
![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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d6bc9b0. Configure here.
| } | ||
| if (parsedUrl.hash) { | ||
| attributes['http.fragment'] = parsedUrl.hash; | ||
| attributes[HTTP_FRAGMENT] = parsedUrl.hash.slice(1) || undefined; |
There was a problem hiding this comment.
Split HTTP query fragment formats
Medium Severity
This commit normalizes http.query and http.fragment by stripping leading ? and # in core fetch, HTTP breadcrumbs, URL span helpers, and Node server spans, but browser XHR and Node undici fetch breadcrumbs still emit the raw URL.search / URL.hash values with those prefixes. The same app can therefore report different query and fragment shapes on http.client spans and HTTP breadcrumbs depending on the API used.
Reviewed by Cursor Bugbot for commit d6bc9b0. Configure here.
There was a problem hiding this comment.
Confirmed. Need to update the following as well to match:
- packages/browser/src/tracing/request.ts 402-403
- packages/node/src/utils/outgoingFetchRequest.ts 261-264
Also found these:
- packages/astro/src/server/middleware.ts 236 and 240
- packages/bun/src/integrations/bunserver.ts 277 and 280
Also, there are TODO(v11) comments about this in packages/opentelemetry/src/utils/parseSpanDescription.ts line 202 and packages/opentelemetry/src/spanExporter.ts line 456 that seem related, and I think can be dropped, since the todo will be to-done after this PR.
Might want to verify that the stripping happening in the otel lib isn't going to double-strip these fields, since the field set in packages/core/src/integrations/requestdata.ts is stripped already. I think it's actually behaving properly, stripping it only where it needs to and not anywhere else (apart from the handful of locations noted above), and it'd likely fail a bunch of tests if it was double-stripping. In which case the TODO(v11) comments are definitely a bit of a hazard.
size-limit report 📦
|
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Some suggestions for cleanup and I think the clanker comment is likely valid.
It might be good to add a helper utility that can strip fragment/search strings, since we have url.search?.strip(1) || undefined in a bunch of places now? Something like:
function stripUrlPart(part: string || undefined || null, prefix: '?' : '#') {
return (part && part.charAt(0) === prefix) ? part.strip(1) : undefined;
}| ...(parsedUrl.search ? { 'http.query': parsedUrl.search.slice(1) || undefined } : {}), | ||
| ...(parsedUrl.hash ? { 'http.fragment': parsedUrl.hash.slice(1) || undefined } : {}), |
There was a problem hiding this comment.
These get serialized with JSON.stringify, so undefined values are safe. Not a blocker, but worth cleaning up where we encounter it, imo. Also, these can probably pull in the costs to cut down on bundle size ever so slightly:
| ...(parsedUrl.search ? { 'http.query': parsedUrl.search.slice(1) || undefined } : {}), | |
| ...(parsedUrl.hash ? { 'http.fragment': parsedUrl.hash.slice(1) || undefined } : {}), | |
| [HTTP_FRAGMENT]: parsedUrl.hash?.slice(1) || undefined, | |
| [HTTP_QUERY]: parsedUrl?.search?.slice(1) || undefined, |
| const query = urlObj?.search ? urlObj.search.slice(1) || undefined : undefined; | ||
| const fragment = urlObj?.hash ? urlObj.hash.slice(1) || undefined : undefined; |
There was a problem hiding this comment.
Same here
| const query = urlObj?.search ? urlObj.search.slice(1) || undefined : undefined; | |
| const fragment = urlObj?.hash ? urlObj.hash.slice(1) || undefined : undefined; | |
| const query = urlObj?.search?.slice(1) || undefined; | |
| const fragment = urlObj?.hash?.slice(1) || undefined; |
| } | ||
| if (parsedUrl.hash) { | ||
| attributes['http.fragment'] = parsedUrl.hash; | ||
| attributes[HTTP_FRAGMENT] = parsedUrl.hash.slice(1) || undefined; |
There was a problem hiding this comment.
Confirmed. Need to update the following as well to match:
- packages/browser/src/tracing/request.ts 402-403
- packages/node/src/utils/outgoingFetchRequest.ts 261-264
Also found these:
- packages/astro/src/server/middleware.ts 236 and 240
- packages/bun/src/integrations/bunserver.ts 277 and 280
Also, there are TODO(v11) comments about this in packages/opentelemetry/src/utils/parseSpanDescription.ts line 202 and packages/opentelemetry/src/spanExporter.ts line 456 that seem related, and I think can be dropped, since the todo will be to-done after this PR.
Might want to verify that the stripping happening in the otel lib isn't going to double-strip these fields, since the field set in packages/core/src/integrations/requestdata.ts is stripped already. I think it's actually behaving properly, stripping it only where it needs to and not anywhere else (apart from the handful of locations noted above), and it'd likely fail a bunch of tests if it was double-stripping. In which case the TODO(v11) comments are definitely a bit of a hazard.
| [ATTR_HTTP_REQUEST_METHOD_ORIGINAL]: request.method, | ||
| [URL_FULL]: requestUrl.toString(), | ||
| [URL_PATH]: requestUrl.pathname, | ||
| [URL_QUERY]: requestUrl.search, |
There was a problem hiding this comment.
Should this be requestUrl.search?.strip(1) || undefined?
2 participants
We rely on this in many tests, but only set this in span inferral often, so it depends on OTEL stuff.