Hubs / Scopes Merge 18 - Implement pushScope ,popScope and withScope for Scopes

[adinauer]

#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

[github-actions]

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

Generated by 🚫 dangerJS against 698a693

[adinauer]

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.

[markushi]

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();

[adinauer]

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.

[adinauer]

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

[adinauer]

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]

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.

[adinauer]

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.

[adinauer]

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

[lbloder]

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

[adinauer]

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

[github-actions]

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]

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]

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]

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.

[adinauer]

Changed in #3375