Hubs/Scopes Merge 33 - No longer replace global scope #3362
Conversation
…pes-merge-2-add-scopes
…ainScopes to rootScopes
adinauer
requested review from
romtsn,
stefanosiano and
markushi
as code owners
|
| @@ -44,6 +44,7 @@ public final class InternalSentrySdk { | |||
| @Nullable | |||
| public static IScope getCurrentScope() { | |||
| final @NotNull AtomicReference<IScope> scopeRef = new AtomicReference<>(); | |||
| // TODO [HSM] should this retrieve combined scope? | |||
There was a problem hiding this comment.
This way, if a user adds, e.g. tags to global scope, cross platform SDKs will also receive the tag.
| @ApiStatus.Internal | ||
| @Override | ||
| public void replaceOptions(final @NotNull SentryOptions options) { | ||
| // TODO [HSM] check if already enabled and noop in that case? |
There was a problem hiding this comment.
This shouldn't be used after Sentry.init since only some options are actually updated.
| this.tracesSampler = new TracesSampler(options); | ||
| this.transactionPerformanceCollector = options.getTransactionPerformanceCollector(); | ||
|
|
||
| // TODO [HSM] Checking isEnabled may not be what we want with global scope anymore |
There was a problem hiding this comment.
Allowing users to use global scope before Sentry.init may imply that we can't noop if not yet enabled.
There was a problem hiding this comment.
What do we do then, if a capture method is called before Sentry.init?
Right now, this would call capture on NoOpScope with no information for the user, right?
There was a problem hiding this comment.
I'd say if the SDK hasn't been enabled yet we shouldn't send anything. We shouldn't store things to send once enabled as the SDK might never be enabled.
RFC also suggests to not do anything in that case: https://github.com/getsentry/rfcs/pull/122/files#diff-c9abcee1a2612ddd7dd66e226e832f520e6b4cbc67bdcbb5371514af7d25ae38R94
Performance metrics 🚀
|
| @@ -24,6 +24,10 @@ | |||
| /** Sentry initialization class */ | |||
| public final class SentryAndroid { | |||
|
|
|||
| static { | |||
| Sentry.getGlobalScope().replaceOptions(new SentryAndroidOptions()); | |||
There was a problem hiding this comment.
Maybe @romtsn can recall exactly, but I think creating options can be expensive. There's also SentryOptions.empty(), maybe that's better?
There was a problem hiding this comment.
Ah, might be a good idea.
There was a problem hiding this comment.
even the empty() one will instantiate some of the options fields (like EnvelopeReader or JsonSerializer), so ideally we don't do that, but I guess there's no way around it. We should just probably fix the root cause here eventually and lazily initialize those fields #2541
There was a problem hiding this comment.
IIRC we discussed that this is not required, as any integrations / processors will access the SentryAndroidOptions only after Sentry.init() which replaces the options already.
| @@ -24,6 +24,10 @@ | |||
| /** Sentry initialization class */ | |||
| public final class SentryAndroid { | |||
|
|
|||
| static { | |||
| Sentry.getGlobalScope().replaceOptions(new SentryAndroidOptions()); | |||
There was a problem hiding this comment.
IIRC we discussed that this is not required, as any integrations / processors will access the SentryAndroidOptions only after Sentry.init() which replaces the options already.
#skip-changelog
📜 Description
We're now instantiating
globalScopewith defaultSentryOptions. These are replaced in astatic {}block forSentryAndroid(temporarily) and then replaced onSentry.initto actually use options customized by the user.Also remove options from
Scopesand instead use globla scope options (viaCombinedScopeView).💡 Motivation and Context
Before we were replacing global scope on
Sentry.init.💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps