Hubs/Scopes Merge 23 - Use new API for CRONS integrations #3347
Conversation
…pes-merge-2-add-scopes
…ainScopes to rootScopes
|
| @@ -49,13 +52,14 @@ public void jobToBeExecuted(final @NotNull JobExecutionContext context) { | |||
| if (maybeSlug == null) { | |||
| return; | |||
| } | |||
| scopes.pushScope(); | |||
| final @NotNull ISentryLifecycleToken lifecycleToken = scopes.pushIsolationScope(); | |||
There was a problem hiding this comment.
For this it likely makes sense to have it isolated.
| @@ -86,7 +87,7 @@ public Object invoke(final @NotNull MethodInvocation invocation) throws Throwabl | |||
| return invocation.proceed(); | |||
| } | |||
|
|
|||
| scopes.pushScope(); | |||
| final @NotNull ISentryLifecycleToken lifecycleToken = scopes.pushIsolationScope(); | |||
There was a problem hiding this comment.
Not 100% sure here, maybe we should make it configurable whether this isolates or not (pushScope vs. pushIsolationScope). The @SentryCheckIn annotation may be used on jobs but can also be used on any method where it may not make sense to isolate but rather keep e.g. the request scope.
There was a problem hiding this comment.
Default should probably be isolated.
There was a problem hiding this comment.
Yeah makes sense!
If any user code within invocation.proceed(); performs changes via the static API, e.g. Sentry.setTag(), those will be written to the isolation scope which is created here, right?
There was a problem hiding this comment.
Yeah, static API and configureScope default to isolation scope for non Android.
There was a problem hiding this comment.
The more I think about API, the more I think we should just find a sane default and only deliver that as a starting point. We can always add more API if users actually ask for it.
| @@ -30,12 +31,11 @@ public static <U> U withCheckIn( | |||
| final @Nullable MonitorConfig monitorConfig, | |||
| final @NotNull Callable<U> callable) | |||
| throws Exception { | |||
| final @NotNull ISentryLifecycleToken lifecycleToken = Sentry.pushIsolationScope(); | |||
There was a problem hiding this comment.
Maybe this should also be configurable to use pushScope or pushIsolationScope.
There was a problem hiding this comment.
Default should probably also be isolated. Add overload with isolated: true/false, defaulting to true
Performance metrics 🚀
|
#skip-changelog
📜 Description
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps