Hubs/Scopes Merge 22 - Combine global, isolation and current scope by adinauer · Pull Request #3346 · getsentry/sentry-java

adinauer · 2024-04-12T09:40:33Z

#skip-changelog

📜 Description

When creating envelopes/events/transactions/... we now have to use all three scopes (global, isolation and current).

TODO: Breadcrumbs currently do not maintain insertion order across scopes. This will be adressed in a future PR.

💡 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-12T09:41:06Z

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

Generated by 🚫 dangerJS against 1a804ca

adinauer · 2024-04-12T09:45:43Z

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

+
+  @Override
+  public @NotNull Contexts getContexts() {
+    return new CombinedContextsView(


CombinedContextsView isn't totally necessary to have but it avoids constantly creating copies of Contexts.

We should use CombinedContextsView as ScopeType.COMBINED for cross platform SDKs should get all values and calls setDevice and setOperatingSystem so handing back a copy that isn't mutable (or rather loses updates) isn't ideal. See #3375

adinauer · 2024-04-12T09:46:41Z

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

+
+  @Override
+  public @NotNull IScope clone() {
+    // TODO just return a new CombinedScopeView with forked scope?


Not quite sure here.

We're now returning a new CombinedScopeView with forked isolation and current scope. See #3375

github-actions · 2024-04-12T09:51:47Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	411.22 ms	497.16 ms	85.94 ms
Size	1.70 MiB	2.28 MiB	595.91 KiB

Previous results on branch: feat/hsm-22-combine-scopes

Startup times

Revision	Plain	With Sentry	Diff
`be47198`	360.88 ms	426.53 ms	65.66 ms

App size

Revision	Plain	With Sentry	Diff
`be47198`	1.70 MiB	2.28 MiB	595.91 KiB

sentry/src/main/java/io/sentry/CombinedContextsView.java

markushi · 2024-04-18T13:53:01Z

sentry/src/main/java/io/sentry/CombinedContextsView.java

+  }
+
+  @Override
+  public @Nullable Device getDevice() {


I think we need some propagation or copy-on-write mechanism for all Scope related fields.

E.g.

val user = Sentry.getUser() // returns global scope user, as it's unset Sentry.setUser(user) // now default scope is set to the user user.email = "[email protected]" // both global and default scope is written to

Hmm this should only happen if someone calls .setUser on global scope so an actual User instance is returned from CombinedScopeView and then pass that into .setUser that goes to current or isolation scope.

Can we ignore this edge case for now and address it, when it actually happens for a customer?

One solution would be to have something like CombinedContextsView for user and other fields.

Here's an example that should avoid the problem:

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

Lines 87 to 98 in ab1c3a6

@Nullable User user = scope.getUser();

if (user == null) {

user = new User();

scope.setUser(user);

}

if (user.getId() == null) {

try {

user.setId(Installation.id(context));

} catch (RuntimeException e) {

logger.log(SentryLevel.ERROR, "Could not retrieve installation ID", e);

}

}

lbloder

The two comments within Scopes.getGlobalScope() could be removed:

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

Lines 584 to 588 in a474402

    
           public @NotNull IScope getGlobalScope() { 
        
             // TODO should be: 
        
             return Sentry.getGlobalScope(); 
        
             //    return scope; 
        
           }

LGTM otherwise

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

adinauer requested review from markushi, romtsn and stefanosiano as code owners April 12, 2024 09:40

adinauer commented Apr 12, 2024

View reviewed changes

adinauer mentioned this pull request Apr 16, 2024

Hubs/Scopes Merge 14 - Add Scopes to replace Hub #3311

Merged

7 tasks

markushi reviewed Apr 18, 2024

View reviewed changes

lbloder approved these changes Apr 22, 2024

View reviewed changes

Base automatically changed from feat/hsm-21-configure-scope-configurable to 8.x.x April 22, 2024 12:43

Merge branch '8.x.x' into feat/hsm-22-combine-scopes

1a804ca

adinauer merged commit 9546564 into 8.x.x Apr 22, 2024

adinauer deleted the feat/hsm-22-combine-scopes branch April 22, 2024 12:47

	@Nullable User user = scope.getUser();
	if (user == null) {
	user = new User();
	scope.setUser(user);
	}
	if (user.getId() == null) {
	try {
	user.setId(Installation.id(context));
	} catch (RuntimeException e) {
	logger.log(SentryLevel.ERROR, "Could not retrieve installation ID", e);
	}
	}

	public @NotNull IScope getGlobalScope() {
	// TODO should be:
	return Sentry.getGlobalScope();
	// return scope;
	}

Uh oh!

Comments

Conversation

adinauer commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions bot commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adinauer Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

adinauer Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinauer Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

adinauer Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: feat/hsm-22-combine-scopes

Startup times

App size

Uh oh!

Uh oh!

markushi Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

adinauer Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

adinauer Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

lbloder left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adinauer commented Apr 12, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading

adinauer Apr 19, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading