-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add onRequestSpanEnd
hook to browser tracing integration
#17884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
ae30de5
to
c7282c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full transparency: I haven't yet looked at this in detail but I wanna share some initial thoughts:
- I think in general, it's fine to add this hook and this is somewhat in line with existing OTel (Node) instrumentation for Http. Not particularly happy with the bundle size impact but at the end of the day, this is scoped to the integration, so it is fine IMHO.
- My main "concern" is that if we expose this hook now, user will likely use it to set response headers, in whatever form they want to. Which is nice for us because PII is less of a concern BUT it also means users likely won't follow our conventions around attribute naming and structure for response headers.
=> Let's make sure we follow up with automatically setting response header attributes soon. I still need to go back and think about why we initially only scoped this only to replay. IIRC there was a concern around adding this to browserTracing by default but I can't recall what specifically that was 😅
@Lms24 Thanks, I don't really have any context around this so all the information is helpful and much appreciated!
I agree, but doesn't the same risk already exist with From the issue:
I don't have opinions on this, so happy to discuss it further. The main issue seems to be auto instrumentation of response headers rather than providing a hook for it, so this is just offering users an escape hatch at the moment.
Oh yea, I was thinking whether to tackle this but decided not to until someone takes a look at this and gives me more context, given that there are PII concerns and Replay relationship. |
This PR aims to address #9643 partially by introducing a
onRequestSpanEnd
hook to the browser integration. These changes make it easier for users to enrich tracing spans with response header data.Example
Tracing Integration and API Improvements
onRequestSpanEnd
callback toBrowserTracingOptions
andRequestInstrumentationOptions
, allowing users to access response headers when a request span ends. This enables custom span annotation based on response data.onRequestSpanEnd
for both Fetch and XHR requests, passing parsed response headers to the callback.Utility and Refactoring
parseXhrResponseHeaders
,getFetchResponseHeaders
,filterAllowedHeaders
) innetworkUtils.ts
, and exported them for reuse across packages.utils.ts
file to avoid failing the file size limit lint rule.I was hesitant to hoist up those replay utils initially but a few of them were needed to expose them on the hook callback.
Type and API Consistency
RequestHookInfo
andResponseHookInfo
to standardize the information passed to request span hooks, and exported them from the core package for use in integrations.I also added the necessary tests to test out the new hook.