Skip to content
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

feat(core): Ensure normalizedRequest on sdkProcessingMetadata is merged #14315

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 15, 2024

We keep normalizedRequest with data like headers, urls etc. on the sdkProcessingMetadata on the scope.

While this generally works, there are some potential pitfalls/footguns there:

One, if you put some (e.g. partial) normalizedRequest on the current scope, it will just overwrite the full normalizedRequest from the isolation scope, where generally we'll try to put the request data automatically (e.g. in the http instrumentation). Think this example:

Sentry.withScope(scope => {
  scope.setSdkProcessingMetadata({ normalizedRequest: { headers: moreSpecificHeaders } });
});

the resulting event inside of this withScope callback would only have the headers, but would miss e.g. url etc. data set.

This PR changes this so that the normalizedRequest is 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.

Related to this, this PR also exposes a new setRequestEventData(normalizedRequest) API, which is a type-safe way to set request data on the scope (defaults to isolation scope), and which will also merge the request data of the scope itself.

(naming based on #14317)

So instead of:

getIsolationScope().setSdkProcessingMetadata({ normalizedRequest: thisIsNotTypedAndCouldBeAnything });

You would do:

setRequestEventData(thisHasToBeTyped);

Alternatively, we could also leave setRequestEventData and a) Adjust the type of setSdkProcessingMetadata to "fix" normalizedRequest, which is kind of breaking (although very unlikely to actually affect any user, but still...), or b) Leave it untyped and generally merge this in setSdkProcessingMetadata(), or c) do not merge it at all in these cases.

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 :(

@mydea mydea self-assigned this Nov 15, 2024
Copy link
Contributor

github-actions bot commented Nov 15, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 22.8 KB +0.15% +33 B 🔺
@sentry/browser - with treeshaking flags 21.57 KB +0.18% +38 B 🔺
@sentry/browser (incl. Tracing) 35.31 KB +0.13% +44 B 🔺
@sentry/browser (incl. Tracing, Replay) 72.04 KB +0.08% +53 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.41 KB +0.07% +40 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 76.35 KB +0.06% +45 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 89.18 KB +0.03% +26 B 🔺
@sentry/browser (incl. Feedback) 39.96 KB +0.08% +32 B 🔺
@sentry/browser (incl. sendFeedback) 27.45 KB +0.13% +34 B 🔺
@sentry/browser (incl. FeedbackAsync) 32.27 KB +0.15% +47 B 🔺
@sentry/react 25.57 KB +0.19% +49 B 🔺
@sentry/react (incl. Tracing) 38.26 KB +0.08% +30 B 🔺
@sentry/vue 26.95 KB +0.12% +33 B 🔺
@sentry/vue (incl. Tracing) 37.14 KB +0.11% +40 B 🔺
@sentry/svelte 22.94 KB +0.15% +35 B 🔺
CDN Bundle 24.16 KB +0.13% +30 B 🔺
⛔️ CDN Bundle (incl. Tracing) (max: 37.11 KB) 37.11 KB +0.18% +65 B 🔺
CDN Bundle (incl. Tracing, Replay) 71.77 KB +0.08% +55 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 77.11 KB +0.07% +49 B 🔺
CDN Bundle - uncompressed 70.85 KB +0.19% +131 B 🔺
CDN Bundle (incl. Tracing) - uncompressed 110.06 KB +0.12% +131 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 222.58 KB +0.06% +131 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 235.8 KB +0.06% +131 B 🔺
@sentry/nextjs (client) 38.37 KB +0.05% +16 B 🔺
@sentry/sveltekit (client) 35.88 KB +0.08% +28 B 🔺
@sentry/node 134.41 KB +0.05% +68 B 🔺
@sentry/node - without tracing 96.29 KB +0.06% +51 B 🔺
@sentry/aws-serverless 106.53 KB +0.05% +49 B 🔺

View base workflow run

Copy link

codecov bot commented Nov 15, 2024

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
654 2 652 24
View the top 2 failed tests by shortest run time
transactions.test.ts Captures request metadata
Stack Traces | 0.035s run time
transactions.test.ts:139:5 Captures request metadata
errors.test.ts Sends graphql exception to Sentry
Stack Traces | 0.054s run time
errors.test.ts:73:5 Sends graphql exception to Sentry

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

@mydea mydea changed the title fix(core): Ensure normalizedRequest on sdkProcessingMetadata is merged feat(core): Ensure normalizedRequest on sdkProcessingMetadata is merged Nov 15, 2024
@mydea mydea marked this pull request as ready for review November 15, 2024 09:37
Copy link
Member

@Lms24 Lms24 left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants