-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Ensure normalizedRequest on sdkProcessingMetadata is merged
#14315
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
Conversation
size-limit report 📦
|
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
normalizedRequest on sdkProcessingMetadata is mergednormalizedRequest on sdkProcessingMetadata is merged
e8533fa to
cd85f65
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.
Sounds good to me!
a56fc65 to
4705e11
Compare
| client.init(); | ||
|
|
||
| // We want to avoid console errors in the tests | ||
| jest.spyOn(console, 'error').mockImplementation(() => {}); |
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.
Finally added this because it kept annoying me when running these tests locally to get this test log output all the time 😅
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.
Actually I'll move this out to #14358
fc7c7b5 to
003f261
Compare
We keep
normalizedRequestwith data like headers, urls etc. on thesdkProcessingMetadataon the scope.While this generally works, there are some potential pitfalls/footguns there:
One, if you put some (e.g. partial)
normalizedRequeston the current scope, it will just overwrite the fullnormalizedRequestfrom the isolation scope, where generally we'll try to put the request data automatically (e.g. in the http instrumentation). Think this example:the resulting event inside of this
withScopecallback would only have the headers, but would miss e.g. url etc. data set.This PR changes this so that the
normalizedRequestis merged between the types of scopes, so only set fields on e.g. the current scope will overwrite the same fields on the isolation scope, instead of just overwriting the whole normalizedRequest that results.Note that bundle size for browser is sadly anyhow affected (no matter if we go with a/b/c), as the code to merge it between scopes is always shared :(