Hubs/Scopes Merge 19 - Add pushIsolationScope and fork methods#3343
Hubs/Scopes Merge 19 - Add pushIsolationScope and fork methods#3343
pushIsolationScope and fork methods#3343Conversation
…pes-merge-2-add-scopes
adinauer
requested review from
markushi,
romtsn and
stefanosiano
as code owners
|
Performance metrics 🚀
|
| } | ||
|
|
||
| @Override | ||
| public @NotNull IScopes forkedCurrentScope(@NotNull String creator) { |
There was a problem hiding this comment.
l: I'm wondering if we should have method overloads here instead. E.g. .forkedScope(ScopeType type), Same could be done for the getters.
There was a problem hiding this comment.
Hmm this could be confusing as I think we should always fork current scope whenever we fork isolation scope and having .forkedScope(ISOLATION) fork both seems odd.
| } | ||
|
|
||
| @Override | ||
| public @NotNull IScope getScope() { |
There was a problem hiding this comment.
What's the reason for adding those getters?
There was a problem hiding this comment.
This was part of the RFC, not sure it's actually needed. It somewhat contradicts what we discussed in one of our meetings where someone said the idea of .configureScope was to make it harder to hold a reference to the scope.
We can remove them again and see if we actually need them, then re-add.
|
|
||
| @Override | ||
| public @NotNull ISentryLifecycleToken pushIsolationScope() { | ||
| return NoOpScopesStorage.NoOpScopesLifecycleToken.getInstance(); |
There was a problem hiding this comment.
Shouldn't this also be Sentry.pushIsolationScope()? Same as in HubAdapter?
There was a problem hiding this comment.
Hub should be removed before releasing the major IMO.
| return NoOpScopesStorage.NoOpScopesLifecycleToken.getInstance(); | ||
| } else { | ||
| Scopes scopes = this.forkedCurrentScope("pushScope"); | ||
| final @NotNull IScopes scopes = this.forkedCurrentScope("pushScope"); |
There was a problem hiding this comment.
Comment on method regarding the return of a lifecycle token can be removed
There was a problem hiding this comment.
Already removed in a follow up PR
| * | ||
| * @return scope | ||
| */ | ||
| public @NotNull IScope getScope(); |
There was a problem hiding this comment.
public keyword can be removed
| * | ||
| * @return isolation scope | ||
| */ | ||
| public @NotNull IScope getIsolationScope(); |
There was a problem hiding this comment.
public keyword can be removed
3 participants
#skip-changelog
📜 Description
💡 Motivation and Context
pushIsolationScopeforks both current and isolation scope and makes them the current scopes.💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps