Hubs/Scopes Merge 38 - Use `ScopeType.COMBINED` for cross platform (`InternalSentrySdk`) #3375

adinauer · 2024-04-19T11:50:11Z

#skip-changelog

📜 Description

To send values from all scope types (global, isolation, current) to cross platform SDKs we use ScopeType.COMBINED for .configureScope to retrieve all tags, breadcrumbs etc.

💡 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

…ainScopes to rootScopes

github-actions · 2024-04-19T11:50:41Z

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

Generated by 🚫 dangerJS against 130f966

adinauer · 2024-04-19T11:50:44Z

sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java

@@ -44,9 +45,9 @@ public final class InternalSentrySdk {
  @Nullable
  public static IScope getCurrentScope() {
    final @NotNull AtomicReference<IScope> scopeRef = new AtomicReference<>();
-    // TODO [HSM] should this retrieve combined scope?


Yes, at least until cross-platform SDKs support separate scope types themselves.

adinauer · 2024-04-19T11:51:29Z

sentry/src/main/java/io/sentry/CombinedScopeView.java

@@ -431,8 +433,7 @@ public void setPropagationContext(@NotNull PropagationContext propagationContext

  @Override
  public @NotNull IScope clone() {
-    // TODO [HSM] just return a new CombinedScopeView with forked scope?
-    return getDefaultWriteScope().clone();
+    return new CombinedScopeView(globalScope, isolationScope.clone(), scope.clone());


The idea here is to have a clone that doesn't change the original so we're safer by cloning both isolation and current scope.

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

sentry/src/main/java/io/sentry/CombinedScopeView.java

@@ -454,7 +455,6 @@ public void bindClient(@NotNull ISentryClient client) {

  @Override
  public @NotNull ISentryClient getClient() {
-    // TODO [HSM] checking for noop here doesn't allow disabling via client, is that ok?


Discussed internally with backend team, if a customer needs this, they can provide their own noop ISentryClient that isn't actually NoOpSentryClient and thus not detected here.

adinauer · 2024-04-19T11:53:32Z

sentry/src/main/java/io/sentry/ScopeType.java

@@ -4,7 +4,5 @@ public enum ScopeType {
  CURRENT,
  ISOLATION,
  GLOBAL,
-
-  // TODO [HSM] do we need a combined as well so configureScope


Yes, at least for cross platform SDKs until they support different scope types separately themselves.

adinauer · 2024-04-19T11:54:05Z

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

@@ -646,9 +646,7 @@ public void withScope(final @NotNull ScopeCallback callback) {

    } else {
      final @NotNull IScopes forkedScopes = forkedCurrentScope("withScope");
-      // TODO [HSM] should forkedScopes be made current inside callback?


Yes, I'd expect Sentry.setTag to operate on the forked scopes inside withScope

github-actions · 2024-04-19T12:00:38Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	379.04 ms	431.88 ms	52.83 ms
Size	1.70 MiB	2.28 MiB	595.24 KiB

Previous results on branch: feat/hsm-38-combined-scope-for-cross-platform

Startup times

Revision	Plain	With Sentry	Diff
`d429cf3`	405.68 ms	497.78 ms	92.09 ms

App size

Revision	Plain	With Sentry	Diff
`d429cf3`	1.70 MiB	2.28 MiB	595.24 KiB

lbloder · 2024-04-23T13:52:52Z

sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt

@@ -138,14 +139,17 @@ class InternalSentrySdkTest {
        )
        // TODO [HSM] add breadcrumbs to all scopes and assert they are there


Is this comment still relevant?

No, already removed.

adinauer and others added 30 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

Add pushIsolationScope; add fork methods to ISCope

2d26033

Use separate scopes for current, isolation and global scope; rename m…

bbb6700

…ainScopes to rootScopes

Allow controlling which scope configureScope uses

c714b21

Combine scopes

a474402

Use new API for CRONS integrations

ae93e33

Add lifecycle helper

b01298b

Change spring integrations to use new API

b64e688

Use new API in servlet integrations

d06fc50

adinauer added 11 commits April 15, 2024 13:25

Fix breadcrumb ordering

bf4a7bf

Mark TODOS with [HSM]

62cb91a

Add getGlobalScope and forkedRootScopes to IScopes

b1630ea

Fix EventProcessor ordering on scopes

136b9ce

Reuse code in Scopes

94d54ef

No longer replace global scope

017599d

Replace hub occurrences in comments, var names etc.

f4c2b3c

Implement ScopesTest

61c9d4a

Implement CombinedScopeViewTest

04f3892

Fix combined contexts

840c194

Use combined scopes for cross platform

ab1c3a6

adinauer requested review from romtsn, stefanosiano and markushi as code owners April 19, 2024 11:50

adinauer commented Apr 19, 2024

View reviewed changes

This was referenced Apr 19, 2024

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

Merged

Hubs/Scopes Merge 22 - Combine global, isolation and current scope #3346

Merged

lbloder reviewed Apr 23, 2024

View reviewed changes

lbloder approved these changes Apr 23, 2024

View reviewed changes

markushi approved these changes Apr 30, 2024

View reviewed changes

Base automatically changed from feat/hsm-37-combined-contexts to 8.x.x May 2, 2024 12:25

Merge branch '8.x.x' into feat/hsm-38-combined-scope-for-cross-platform

130f966

adinauer merged commit 9349303 into 8.x.x May 2, 2024
10 of 17 checks passed

adinauer deleted the feat/hsm-38-combined-scope-for-cross-platform branch May 2, 2024 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hubs/Scopes Merge 38 - Use `ScopeType.COMBINED` for cross platform (`InternalSentrySdk`) #3375

Hubs/Scopes Merge 38 - Use `ScopeType.COMBINED` for cross platform (`InternalSentrySdk`) #3375

adinauer commented Apr 19, 2024

github-actions bot commented Apr 19, 2024 •

edited

adinauer Apr 19, 2024 •

edited

adinauer Apr 19, 2024

adinauer Apr 19, 2024

adinauer Apr 19, 2024

adinauer Apr 19, 2024

github-actions bot commented Apr 19, 2024 •

edited

Previous results on branch: feat/hsm-38-combined-scope-for-cross-platform

Startup times

App size

lbloder Apr 23, 2024

adinauer Apr 23, 2024

		@@ -138,14 +139,17 @@ class InternalSentrySdkTest {
		)
		// TODO [HSM] add breadcrumbs to all scopes and assert they are there

Hubs/Scopes Merge 38 - Use ScopeType.COMBINED for cross platform (InternalSentrySdk) #3375

Hubs/Scopes Merge 38 - Use ScopeType.COMBINED for cross platform (InternalSentrySdk) #3375

Conversation

adinauer commented Apr 19, 2024

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Apr 19, 2024 • edited

adinauer Apr 19, 2024 • edited

Choose a reason for hiding this comment

adinauer Apr 19, 2024

Choose a reason for hiding this comment

adinauer Apr 19, 2024

Choose a reason for hiding this comment

adinauer Apr 19, 2024

Choose a reason for hiding this comment

adinauer Apr 19, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 19, 2024 • edited

Performance metrics 🚀

Previous results on branch: feat/hsm-38-combined-scope-for-cross-platform

Startup times

App size

lbloder Apr 23, 2024

Choose a reason for hiding this comment

adinauer Apr 23, 2024

Choose a reason for hiding this comment

Hubs/Scopes Merge 38 - Use `ScopeType.COMBINED` for cross platform (`InternalSentrySdk`) #3375

Hubs/Scopes Merge 38 - Use `ScopeType.COMBINED` for cross platform (`InternalSentrySdk`) #3375

github-actions bot commented Apr 19, 2024 •

edited

adinauer Apr 19, 2024 •

edited

github-actions bot commented Apr 19, 2024 •

edited