Hubs / Scopes Merge 18 - Implement `pushScope` ,`popScope` and `withScope` for `Scopes` by adinauer · Pull Request #3321 · getsentry/sentry-java

adinauer · 2024-04-04T14:39:43Z

#skip-changelog

📜 Description

Without having a stack as we used to in Hub the only option I see is to make pushScope etc. manipulate scopes storage.

There is one major problem here, where we use a Scopes (used to be Hub) reference that isn't ScopesAdapter (used to be HubAdapter). I think the most relevant here are reactive frameworks (reactor / webflux). Here's problematic code:

IScopes scopes = Sentry.cloneMainHub()
scopes.pushScope()
scopes.setRequest(...)
...
// during handling of the request:
Sentry.captureMessage("...")

Because pushScope now creates a fork of Scopes and sets it as current, doing anything with the scopes reference, will lead to unexpected results. In this case request isn't set on the message being captured. The problem is, this used to work just fine with Hub because of its internal stack.

The main reason, we have so much code, using scopes (previously hub) references is testability. Maybe we should consider a different approach for testing, e.g. using a special scopes storage. However I don't want to fiddle with tests and production code too much at the same time, so I suggest leaving tests alone as much as possible and once production changes are (mostly) done, we can revisit our testing approach.

This PR also introduces makeCurrent on Scopes which I copied from OpenTelemetry. It stores a Scopes in the scopes storage, making it the current one, that is used by static API.

I'm not yet entirely certain how to handle pushScope and popScope. Maybe we should deprecate them in favor of a single new method, that hands back a lifecycle token for closing (popping) and restoring previous state.

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

…pes-merge-2-add-scopes

github-actions · 2024-04-04T14:40:25Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 698a693

adinauer · 2024-04-04T14:43:50Z

sentry/src/main/java/io/sentry/Scopes.java

+      final @Nullable Scopes parent = getParent();
+      if (parent != null) {
+        // TODO this is never closed
+        parent.makeCurrent();


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.

If I read it right makeCurrent() returns the token for the previous scope. Since this is legacy push/pop API, wouldn't it make sense to automatically close the previous scope?

Suggested change

parent.makeCurrent();

final @NotNull ISentryLifecycleToken previousScopes = parent.makeCurrent();

previousScopes.close();

The previous Scopes should be restored, when the current one is closed, so we can't close it when creating a new (child) scope.

Take a piece of nested code:

// outermost scopes (scopes1) Sentry.withScope(scope -> { // inner scopes (scopes2) try { ... = scope.forkedCurrentScopes("...").makeCurrent()) { // innermost scopes (scopes3) } }

When leaving the try block, we're closing scopes3 and restoring scopes2. When leaving the withScope lambda, we're closing scopes2 and restoring scopes1.

I copied this pattern from OpenTelemetry, as we'll have to call their io.opentelemetry.context.Scope.close() inside our OTEL flavor lifecycle token. See https://github.com/getsentry/sentry-java/pull/3285/files#diff-ce5e1c852e10adfae80110c82615eaa9571a7eeb5ed8bb79512c08dc388c4bc0R43

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.

adinauer · 2024-04-04T14:45:13Z

sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java

              serverWebExchange.getAttributes().put(SENTRY_SCOPES_KEY, requestHub);
              Sentry.setCurrentScopes(requestHub);
-              requestHub.pushScope();
+              requestHub.pushScope(); // TODO don't


pushScope by itself here wouldn't be problematic, however combining this with e.g. setting request on the requestHub reference, leads to unexpected results where e.g. request isn't set on captured messages.

Instead of pushScope, maybe we could simply call requestHub.forkedCurrentScopes() and then call .makeCurrent() on that. Then from there on only use the reference, returned by the fork call.

I'm thinking tho we don't even need to call pushScope here since we're forking Scopes anyways.

We could look into Mono.using() to do the cleanup for us in a follow up PR

Let's test first and then follow up with a separate PR if it actually works / helps

github-actions · 2024-04-04T14:49:32Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	373.45 ms	437.56 ms	64.11 ms
Size	1.70 MiB	2.28 MiB	591.81 KiB

Previous results on branch: feat/hsm-18-push-scope-etc

Startup times

Revision	Plain	With Sentry	Diff
`1bbfe96`	380.17 ms	428.18 ms	48.02 ms

App size

Revision	Plain	With Sentry	Diff
`1bbfe96`	1.70 MiB	2.28 MiB	591.81 KiB

markushi · 2024-04-18T08:03:39Z

There is one major problem here, where we use a Scopes (used to be Hub) reference that isn't ScopesAdapter (used to be HubAdapter). I think the most relevant here are reactive frameworks (reactor / webflux).

Is this really an issue? IMHO push/pop should only be used in combination with other old APIs like withScope(@NotNull ScopeCallback callback) or configureScope(@NotNull ScopeCallback callback);, as those were designed in a way to avoid leaking references.

adinauer · 2024-04-18T11:33:19Z

Is this really an issue?

We'll see as bug reports come in, I guess. We can fix all our integrations but in theory it's possible users will experience some weird behaviour because of this.

lbloder · 2024-04-19T09:58:00Z

sentry/src/main/java/io/sentry/Scopes.java

-      Scopes forkedScopes = forkedScopes("withScope");
+      Scopes forkedScopes = forkedCurrentScope("withScope");
      // TODO should forkedScopes be made current inside callback?
      // TODO forkedScopes.makeCurrent()?


Depending on the behaviour we want to achieve, it might make sense to make them the currentScopes. IIRC Hub does this by pushing/popping scope around the callback.

We could just add the makeCurrent call to the the try/catch block here.

Changed in #3375

adinauer and others added 22 commits March 27, 2024 16:30

replace hub with scopes

305baf5

Add Scopes

95f5e1b

Introduce IScopes interface.

27f2398

Replace IHub with IScopes in core

ce3c14f

Replace IHub with IScopes in android core

ce615f4

Replace IHub with IScopes in android integrations

22ddc00

Replace IHub with IScopes in apollo integrations

305c217

Replace IHub with IScopes in okhttp integration

da927bc

Replace IHub with IScopes in graphql integration

8279276

Replace IHub with IScopes in logging integrations

9bfc086

Replace IHub with IScopes in more integrations

b998e50

Replace IHub with IScopes in OTel integration

739827a

Replace IHub with IScopes in Spring 5 / Spring Boot 2 integrations

69f2d63

Replace IHub with IScopes in Spring 6 / Spring Boot 3 integrations

792d482

Replace IHub with IScopes in samples

9bcbce6

Merge branch 'feat/hsm-13-replacements-in-samples' into feat/hubs-sco…

3f25a4b

…pes-merge-2-add-scopes

gitscopes -> github

d6fb40a

Replace ThreadLocal with ScopesStorage

7752bcc

Move client and throwable to span map to scope

1e329c5

Add global scope

b0d89ae

use global scope in Scopes

cdd414a

Implement pushScope popScope and withScope for Scopes

98da9ff

adinauer requested review from markushi, romtsn and stefanosiano as code owners April 4, 2024 14:39

adinauer changed the title ~~Implement pushScope ,popScope and withScope for Scopes~~ Hubs / Scopes Merge 18 - Implement pushScope ,popScope and withScope for Scopes Apr 4, 2024

adinauer commented Apr 4, 2024

View reviewed changes

lbloder reviewed Apr 19, 2024

View reviewed changes

Base automatically changed from feat/hsm-17-global-scope to 8.x.x April 19, 2024 12:06

lbloder approved these changes Apr 19, 2024

View reviewed changes

Merge branch '8.x.x' into feat/hsm-18-push-scope-etc

698a693

adinauer merged commit 6ee5169 into 8.x.x Apr 19, 2024

adinauer deleted the feat/hsm-18-push-scope-etc branch April 19, 2024 12:15

	parent.makeCurrent();
	final @NotNull ISentryLifecycleToken previousScopes = parent.makeCurrent();
	previousScopes.close();

Uh oh!

Comments

Conversation

adinauer commented Apr 4, 2024

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions bot commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adinauer Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinauer Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: feat/hsm-18-push-scope-etc

Startup times

App size

Uh oh!

markushi commented Apr 18, 2024

Uh oh!

adinauer commented Apr 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 4, 2024 •

edited

Loading

adinauer Apr 4, 2024 •

edited

Loading

adinauer Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading