-
-
Notifications
You must be signed in to change notification settings - Fork 461
Hubs / Scopes Merge 18 - Implement pushScope ,popScope and withScope for Scopes
#3321
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
Changes from all commits
305baf5
95f5e1b
27f2398
ce3c14f
ce615f4
22ddc00
305c217
da927bc
8279276
9bfc086
b998e50
739827a
69f2d63
792d482
9bcbce6
3f25a4b
d6fb40a
7752bcc
1e329c5
b0d89ae
cdd414a
98da9ff
698a693
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -595,20 +595,21 @@ private void updateLastEventId(final @NotNull SentryId lastEventId) { | |||||||
| // TODO needs to be deprecated because there's no more stack | ||||||||
| // TODO needs to return a lifecycle token | ||||||||
| @Override | ||||||||
| public void pushScope() { | ||||||||
| public ISentryLifecycleToken pushScope() { | ||||||||
| if (!isEnabled()) { | ||||||||
| options | ||||||||
| .getLogger() | ||||||||
| .log(SentryLevel.WARNING, "Instance is disabled and this 'pushScope' call is a no-op."); | ||||||||
| return NoOpScopesStorage.NoOpScopesLifecycleToken.getInstance(); | ||||||||
| } else { | ||||||||
| // Scopes scopes = this.forkedScopes("pushScope"); | ||||||||
| // return scopes.makeCurrent(); | ||||||||
| Scopes scopes = this.forkedCurrentScope("pushScope"); | ||||||||
| return scopes.makeCurrent(); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // public SentryLifecycleToken makeCurrent() { | ||||||||
| // // TODO store.set(this); | ||||||||
| // } | ||||||||
| public ISentryLifecycleToken makeCurrent() { | ||||||||
| return Sentry.setCurrentScopes(this); | ||||||||
| } | ||||||||
|
|
||||||||
| // TODO needs to be deprecated because there's no more stack | ||||||||
| @Override | ||||||||
|
|
@@ -618,8 +619,11 @@ public void popScope() { | |||||||
| .getLogger() | ||||||||
| .log(SentryLevel.WARNING, "Instance is disabled and this 'popScope' call is a no-op."); | ||||||||
| } else { | ||||||||
| // TODO how to remove fork? | ||||||||
| // TODO getParentScopes().makeCurrent()? | ||||||||
| final @Nullable Scopes parent = getParent(); | ||||||||
| if (parent != null) { | ||||||||
| // TODO this is never closed | ||||||||
| parent.makeCurrent(); | ||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're handed back a lifecycle token here which we never call close on. Popping scopes this way is kinda hacky. I haven't come up with a better approach yet.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I read it right
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous Take a piece of nested code: When leaving the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied this pattern from OpenTelemetry, as we'll have to call their
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think of this chain of lifecycle tokens as a replacement for the stack. We're keeping scopes from being garbage collected by holding a reference to each parent scopes instance. |
||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -634,7 +638,7 @@ public void withScope(final @NotNull ScopeCallback callback) { | |||||||
| } | ||||||||
|
|
||||||||
| } else { | ||||||||
| Scopes forkedScopes = forkedScopes("withScope"); | ||||||||
| Scopes forkedScopes = forkedCurrentScope("withScope"); | ||||||||
| // TODO should forkedScopes be made current inside callback? | ||||||||
| // TODO forkedScopes.makeCurrent()? | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Depending on the behaviour we want to achieve, it might make sense to make them the currentScopes. IIRC We could just add the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed in #3375 |
||||||||
| try { | ||||||||
|
|
@@ -705,7 +709,8 @@ public void flush(long timeoutMillis) { | |||||||
| if (!isEnabled()) { | ||||||||
| options.getLogger().log(SentryLevel.WARNING, "Disabled Hub cloned."); | ||||||||
| } | ||||||||
| return new HubScopesWrapper(forkedScopes("scopes clone")); | ||||||||
| // TODO should this fork isolation scope as well? | ||||||||
| return new HubScopesWrapper(forkedCurrentScope("scopes clone")); | ||||||||
| } | ||||||||
|
|
||||||||
| @ApiStatus.Internal | ||||||||
|
|
||||||||
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.
pushScopeby itself here wouldn't be problematic, however combining this with e.g. settingrequeston therequestHubreference, leads to unexpected results where e.g.requestisn't set on captured messages.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.
Instead of
pushScope, maybe we could simply callrequestHub.forkedCurrentScopes()and then call.makeCurrent()on that. Then from there on only use the reference, returned by the fork call.Uh oh!
There was an error while loading. Please reload this page.
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.
I'm thinking tho we don't even need to call
pushScopehere since we're forkingScopesanyways.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.
We could look into
Mono.using()to do the cleanup for us in a follow up PRThere 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.
Let's test first and then follow up with a separate PR if it actually works / helps