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.

Sign up for GitHub

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

Jump to bottom

fix(core): Run client eventProcessors before global ones #9032

Merged
merged 1 commit into from Sep 20, 2023
Merged

fix(core): Run client eventProcessors before global ones #9032

merged 1 commit into from Sep 20, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Sep 15, 2023 •
edited

I noticed that while from an API POV the current behavior makes kind-of sense IMHO:

  1. Run global event processors
  2. Run scope event processors
  3. Run client event processors

It is potentially breaking, as if we rewrite integrations to use the new client processors, their processing will run after any user global event processors, leading to potentially unexpected outcomes.

So this PR changes this to instead run them in this order:

  1. Run client event processors
  2. Run global event processors
  3. Run scope event processors

Which should be more stable for now.

In v8, we should update this to run a more sensible order:

  1. Global
  2. Client
  3. Scope

@mydea mydea requested review from lforst and Lms24 September 15, 2023 08:49
@mydea mydea self-assigned this Sep 15, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2023 •
edited

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 75.61 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.51 KB (+0.06% 🔺)
@sentry/browser - Webpack (gzipped) 22.11 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 70.28 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.61 KB (+0.07% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.67 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 222.19 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 86.68 KB (+0.04% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 61.53 KB (+0.06% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.48 KB (+0.03% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 75.63 KB (+0.04% 🔺)
@sentry/react - Webpack (gzipped) 22.14 KB (+0.08% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 93.51 KB (+0.03% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 51.09 KB (+0.06% 🔺)

Lms24
Lms24 approved these changes Sep 15, 2023
View reviewed changes
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.

Makes sense for now and yes, let's definitely change this in v8

@mydea mydea force-pushed the fn/adjust-processEventOrder branch 2 times, most recently from 7a361b3 to 6e5beae Compare September 15, 2023 10:17
mydea
mydea commented Sep 15, 2023
View reviewed changes
packages/core/src/utils/prepareEvent.ts
result = finalScope.applyToEvent(prepared, hint);
result = finalScope.applyToEvent(prepared, hint, clientEventProcessors);
} else {
// Apply client & global event processors even if there is no scope
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that in some tests we actually ran into this branch, so IMHO it makes sense to ensure also global processors run if no scope is supplied (which is also kind of weird right now). This should only really be a problem when not going through getCurrentHub().xxx, as there we always put the scope, but e.g. if you call captureException() on the client directly or similar, this is not guaranteed to be there.

mydea
mydea commented Sep 15, 2023
View reviewed changes
packages/node/test/manual/release-health/runner.js Outdated
@@ -15,7 +15,7 @@ for (const dir of scenariosDirs) {

const processes = scenarios.map(([filename, filepath]) => {
return new Promise(resolve => {
const scenarioProcess = spawn('node', [filepath]);
const scenarioProcess = spawn('node', [filepath], { timeout: 10000 });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed this was never timing out, so adding a reasonable timeout here to prevent super long test run times when something goes wrong.

mydea
mydea commented Sep 15, 2023
View reviewed changes
...ages/node/test/manual/release-health/session-aggregates/aggregates-disable-single-session.js Outdated
@@ -12,6 +12,9 @@ function cleanUpAndExitSuccessfully() {
}

function assertSessionAggregates(session, expected) {
if (!session.aggregates) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually also happening before, only now it is raising an error (=sentry exception) correctly which leads to this failing 😅

@mydea mydea mentioned this pull request Sep 20, 2023
Merged
@mydea
Copy link
Member Author

mydea commented Sep 20, 2023

I actually extracted part of this out here: #9064 so we can reason about this properly.

@mydea
Copy link
Member Author

mydea commented Sep 20, 2023

Rebased this on develop, this now only contains the change to run client > global > scope processors in that order!

@mydea mydea merged commit 42ddf41 into develop Sep 20, 2023
82 checks passed
@mydea mydea deleted the fn/adjust-processEventOrder branch September 20, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lms24 Lms24 Lms24 approved these changes

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

@lforst lforst Awaiting requested review from lforst

Assignees

@mydea mydea

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mydea @Lms24 @AbhiPrasad