feat: Streamline isolation scope handling & reset in isolation scopes - #22890
Conversation
size-limit report 📦
|
2cab26a to
1eda574
Compare
1eda574 to
ccf3b05
Compare
ccf3b05 to
8979856
Compare
|
|
||
| if (!hasSpansEnabled()) { | ||
| const scopePropagationContext = { ...isolationScope.getPropagationContext(), ...scope.getPropagationContext() }; | ||
| const scopePropagationContext = scope.getPropagationContext(); |
There was a problem hiding this comment.
it seems like this removes a footgun where we potentially incorrectly merged a propagation context before here. I like it!
8979856 to
8196869
Compare
a2496b8 to
7e8cef9
Compare
7e8cef9 to
dea8d51
Compare
dea8d51 to
d1f9cac
Compare
d1f9cac to
2efb184
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 65c8a4c. Configure here.
There was still a missed case where the isolation scop's propagation context was (now incorrectly) being used. Test verifies the resulting mismatched behavior.
65c8a4c to
d01ded9
Compare
isaacs
left a comment
There was a problem hiding this comment.
This is a good change, but there are a few gaps, which I think would result in bugs in trace propagation.
I rebased it onto develop, which picked up the fix for one missing case, which was fixed on 46f7431 as part of #22889.
For the others, I added commits with tests and fixes here: https://gist.github.com/isaacs/790e2d23abeebddf5b9079f53e4fbf45 or on the isaacs/fix-fn/streamline-acs-isolation-scope branch.
I figured this is probably more useful than just comments, since I'm going to be out next 2 weeks, so whoever picks this up can inspect and land with those additions, or go a different direction if you decide. 👍
There was a problem hiding this comment.
Should this also be updated to just scope.getPropagationContext()?
| } = scope.getPropagationContext(); |
or:
| } = { | |
| ...scope.getPropagationContext(), | |
| }; |

This PR does two things:
startNewTrace. But this does not align with core/browser handling, so going forward this will be removed. instead, we want this logic to be applied in an isolation scope, so the following code will result in separate traces:without this change, both of these spans would be part of the same trace, which is not what we want in server.
note: In browser, we do not actually fork the isolation scope, there is just a single one, so this does not apply.