feat(browser): Add support for streamed spans in httpContextIntegration#20464
feat(browser): Add support for streamed spans in httpContextIntegration#20464nicohrubec wants to merge 2 commits intodevelopfrom
httpContextIntegration#20464Conversation
|
|
||
| const reqData = getHttpRequestData(); | ||
|
|
||
| span.attributes = { |
There was a problem hiding this comment.
l: instead of spreading this here, can we just set the attributes we want to set directly? IMHO we can just have a utility we keep using like setIfNotExists(span.attributes, 'url.full', reqData.url), or similar? Not 100% sure but I'd suspect this is faster at scale than creating a new object all the time when processing spans like this?
There was a problem hiding this comment.
We have a utility @sentry/core that does exactly what we would want I think (ref). We could export that from core and then use it in any integrations we need to port. I copied the approach here from how Lukas did the cultureContext integration (ref), not sure if he had a specific reason in mind not to use this. Wdyt?
There was a problem hiding this comment.
sounds good to me (exporting this and using it). FWIW the approach from lukas and in this PR are not bad I just think it is slightly better (probably) to avoid creating new objects for these things all the time - and this will likely happen quite a lot if there are multiple event processors all just adding one or two attributes!
size-limit report 📦
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![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 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7c5c438. Configure here.
| const reqData = getHttpRequestData(); | ||
|
|
||
| span.attributes = { | ||
| 'url.full': reqData.url, |
There was a problem hiding this comment.
Bug: The code unconditionally sets span.attributes['url.full'] even when the URL is an empty string, violating the Sentry span protocol which requires omitting empty attributes.
Severity: MEDIUM
Suggested Fix
Conditionally set the 'url.full' attribute only when reqData.url is a non-empty string. Add a check, such as if (reqData.url) { span.attributes['url.full'] = reqData.url; }, to ensure empty attributes are not sent, aligning with the protocol and the handling of other optional attributes.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/browser/src/integrations/httpcontext.ts#L38
Potential issue: In environments where `window.document.location.href` is inaccessible,
such as sandboxed iframes, the `getLocationHref()` function returns an empty string.
Consequently, `getHttpRequestData()` provides an empty string for the URL. The new code
at `processSegmentSpan` unconditionally assigns this empty string to
`span.attributes['url.full']`. This violates the Sentry span protocol, which mandates
that empty attributes must be omitted. This behavior is inconsistent with how other
optional attributes like `User-Agent` are handled, which are only added if they have a
value, and could lead to data validation issues.
There was a problem hiding this comment.
^^ this also seems legitimate, we likely need to make sure that passing in undefined as a value does not result in this being set on the attributes object.
2 participants
This PR adds span processing support for the
httpContextIntegration:processSegmentSpancallback onhttpContextIntegrationthat adds the attributes. All attributes are already registered in sentry conventions: https://getsentry.github.io/sentry-conventions/attributes/url/#url-full and https://getsentry.github.io/sentry-conventions/attributes/http/#http-request-header-keyCloses #20378