fix(core): Run client eventProcessors before global ones#9032
Conversation
b4a7886 to
b5b39ba
Compare
size-limit report 📦
|
Lms24
left a comment
There was a problem hiding this comment.
Makes sense for now and yes, let's definitely change this in v8
7a361b3 to
6e5beae
Compare
| result = finalScope.applyToEvent(prepared, hint); | ||
| result = finalScope.applyToEvent(prepared, hint, clientEventProcessors); | ||
| } else { | ||
| // Apply client & global event processors even if there is no scope |
There was a problem hiding this comment.
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.
6e5beae to
f478c79
Compare
| const processes = scenarios.map(([filename, filepath]) => { | ||
| return new Promise(resolve => { | ||
| const scenarioProcess = spawn('node', [filepath]); | ||
| const scenarioProcess = spawn('node', [filepath], { timeout: 10000 }); |
There was a problem hiding this comment.
Noticed this was never timing out, so adding a reasonable timeout here to prevent super long test run times when something goes wrong.
| } | ||
|
|
||
| function assertSessionAggregates(session, expected) { | ||
| if (!session.aggregates) { |
There was a problem hiding this comment.
This was actually also happening before, only now it is raising an error (=sentry exception) correctly which leads to this failing 😅
f478c79 to
ab75422
Compare
|
I actually extracted part of this out here: #9064 so we can reason about this properly. |
ab75422 to
9ea5500
Compare
|
Rebased this on develop, this now only contains the change to run client > global > scope processors in that order! |
I noticed that while from an API POV the current behavior makes kind-of sense IMHO:
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:
Which should be more stable for now.
In v8, we should update this to run a more sensible order: