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

adinauer · 2024-04-02T13:46:28Z

#skip-changelog

📜 Description

Introducing Scopes class which shall replace Hub class.

Scopes forwards API to one/multiple of current scope, isolation scope or global scope (will be fleshed out more in follow up PRs).

Scopes is somewhat of a 1:1 copy of Hub but uses different IScope depending on API. Some things previously stored on Hub will have to be moved.

💡 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

github-actions · 2024-04-02T13:48:38Z

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

Generated by 🚫 dangerJS against d260706

github-actions · 2024-04-02T14:02:55Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	397.27 ms	441.39 ms	44.12 ms
Size	1.70 MiB	2.24 MiB	555.31 KiB

adinauer · 2024-04-04T08:17:56Z

sentry/src/main/java/io/sentry/Sentry.java

@@ -800,6 +801,7 @@ public static void removeExtra(final @NotNull String key) {
  public static void pushScope() {
    // pushScope is no-op in global hub mode
    if (!globalHubMode) {
+      // TODO this might have to behave differently from Scopes.pushScope


I'll address pushScope, popScope and withScope in a separate PR.

We just forward to current Scopes and let it deal with it. If pushScope is called on a non current Scopes, the Scopes forked by pushScope is made current (i.e. saved in ThreadLocal).

adinauer · 2024-04-04T08:23:22Z

sentry/src/main/java/io/sentry/Sentry.java

@@ -254,8 +254,9 @@ private static synchronized void init(
    Sentry.globalHubMode = globalHubMode;

    final IScopes hub = getCurrentScopes();
-    // TODO new Scopes()
-    mainScopes = new Hub(options);
+    final IScope rootScope = new Scope(options);


This bit in init will have to be replaced in a follow up PR.

This code region has changed multiple times. #3362 has the most recent state (for now :D)

adinauer · 2024-04-04T11:26:01Z

sentry/api/sentry.api

@@ -661,10 +661,12 @@ public abstract interface class io/sentry/IScope {
 	public abstract fun endSession ()Lio/sentry/Session;
 	public abstract fun getAttachments ()Ljava/util/List;
 	public abstract fun getBreadcrumbs ()Ljava/util/Queue;
+	public abstract fun getClient ()Lio/sentry/ISentryClient;


Client now lives on IScope and we'll set the default one on global scope in the future (follow up PR).

Client is now bound to global scope, as of #3344 but the code region is changed in later PRs a couple times.

Sounds good! For metrics it would be important to have only one shared SentryClient instance by default, as metrics aggregation/sending is bound to that right now. If that's not the case anymore just let me know, I can take a look.

Should still be true. The SentryClient is set on global scope on Sentry.init. Before that there's a NoOpSentryClient on global scope. By default there should also only ever be NoOpSentryClient on isolation and current scope. A user can however bind any client on any of the scopes.

adinauer · 2024-04-04T11:27:02Z

sentry/api/sentry.api

 	public abstract fun getContexts ()Lio/sentry/protocol/Contexts;
 	public abstract fun getEventProcessors ()Ljava/util/List;
 	public abstract fun getExtras ()Ljava/util/Map;
 	public abstract fun getFingerprint ()Ljava/util/List;
+	public abstract fun getLastEventId ()Lio/sentry/protocol/SentryId;


I was thinking that we could track last event ID per scope. By default it should probably just return the value from global scope in the future.

Changed in #3346

lbloder · 2024-04-09T08:25:30Z

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

+          }
+        }
+
+        // TODO which scopes do we call this on? isolation and current scope?


I would think so, yes

We need to update this in a follow up PR

lbloder · 2024-04-09T08:35:43Z

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

+    }
+  }
+
+  // TODO this seems unused


Yes, at first glance this is only called in a single Test. Most likely safe to remove

Already removed in a follow up PR.

lbloder · 2024-04-09T08:37:59Z

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

+    @NotNull
+    PropagationContext propagationContext =
+        PropagationContext.fromHeaders(getOptions().getLogger(), sentryTrace, baggageHeaders);
+    // TODO should this go on isolation scope?


To me it makes sense to put this on isolation scope

For backend this goes to isolation scope by default. For Android it goes to current scope but that should never be forked on Android.

markushi

Looking good, left a few comments. I'm approving this, as I assume the mentioned TODOs are addressed in the follow-up PRs.

markushi · 2024-04-17T07:55:26Z

sentry/src/main/java/io/sentry/Scope.java

nit: a few final keywords are missing, e.g. in the setters.

Will address in a follow up PR.

markushi · 2024-04-17T07:58:14Z

sentry/api/sentry.api

@@ -661,10 +661,12 @@ public abstract interface class io/sentry/IScope {
 	public abstract fun endSession ()Lio/sentry/Session;
 	public abstract fun getAttachments ()Ljava/util/List;
 	public abstract fun getBreadcrumbs ()Ljava/util/Queue;
+	public abstract fun getClient ()Lio/sentry/ISentryClient;


Sounds good! For metrics it would be important to have only one shared SentryClient instance by default, as metrics aggregation/sending is bound to that right now. If that's not the case anymore just let me know, I can take a look.

markushi · 2024-04-17T08:25:58Z

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

+    this(scope, isolationScope, null, options, creator);
+  }
+
+  private Scopes(


Seeing IScope and Scopes side by side here could be confusing to our users, since both are public APIs. I'm wondering if we could use some backwards compatible interface ScopeContext : IScope {} to make it clear that Scopes is the forking mechanism / API and ScopeContext simply the data holder.

Scope is part of https://develop.sentry.dev/sdk/unified-api/ so I don't think we should rename it.

Scopes is in fact a plural of Scope as it combines multiple scopes together to allow scoping things like tags etc. to the users needs.

markushi · 2024-04-17T08:30:34Z

sentry/src/main/java/io/sentry/Scope.java

+  }
+
+  @Override
+  public @NotNull SentryId getLastEventId() {


It's a bit weird but JS removed this feature altogether:

REMOVED - Use event processors or beforeSend instead

But to be honest I still think we need it though 😅

If we intend to remove it, we should deprecate it for a while and only remove this in a later major, not in 8.x.

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

adinauer commented Apr 4, 2024

View reviewed changes

adinauer marked this pull request as ready for review April 4, 2024 11:38

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

lbloder reviewed Apr 9, 2024

View reviewed changes

Base automatically changed from feat/hsm-13-replacements-in-samples to 8.x.x April 16, 2024 14:47

Merge branch '8.x.x' into feat/hubs-scopes-merge-2-add-scopes

d260706

markushi approved these changes Apr 17, 2024

View reviewed changes

lbloder approved these changes Apr 19, 2024

View reviewed changes

adinauer merged commit 7a0cd9f into 8.x.x Apr 19, 2024
6 of 20 checks passed

adinauer deleted the feat/hubs-scopes-merge-2-add-scopes branch April 19, 2024 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adinauer commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

adinauer Apr 4, 2024

adinauer Apr 16, 2024 •

edited

Loading

adinauer Apr 4, 2024

adinauer Apr 16, 2024

adinauer Apr 4, 2024

adinauer Apr 16, 2024

markushi Apr 17, 2024 •

edited

Loading

adinauer Apr 18, 2024

adinauer Apr 4, 2024

adinauer Apr 16, 2024

lbloder Apr 9, 2024

adinauer Apr 19, 2024

lbloder Apr 9, 2024

adinauer Apr 16, 2024

lbloder Apr 9, 2024

adinauer Apr 16, 2024

markushi left a comment

markushi Apr 17, 2024

adinauer Apr 18, 2024

markushi Apr 17, 2024 •

edited

Loading

markushi Apr 17, 2024

adinauer Apr 18, 2024

adinauer Apr 18, 2024

markushi Apr 17, 2024

adinauer Apr 18, 2024

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

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

Conversation

adinauer commented Apr 2, 2024 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Apr 2, 2024 • edited Loading

github-actions bot commented Apr 2, 2024 • edited Loading

Performance metrics 🚀

Choose a reason for hiding this comment

adinauer Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markushi Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markushi Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

adinauer commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

adinauer Apr 16, 2024 •

edited

Loading

markushi Apr 17, 2024 •

edited

Loading

markushi Apr 17, 2024 •

edited

Loading