Hubs/Scopes Merge 27 - Discussions #3352

adinauer · 2024-04-12T12:56:06Z

#skip-changelog

📜 Description

There's a lot of open questions, we should discuss.

💡 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-12T12:56:36Z

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

Generated by 🚫 dangerJS against 2b05019

adinauer · 2024-04-12T12:57:14Z

build.gradle.kts

        }
        withType<JavaCompile> {
-            options.compilerArgs.addAll(arrayOf("-Xlint:all", "-Werror", "-Xlint:-classfile", "-Xlint:-processing"))
+            options.compilerArgs.addAll(arrayOf("-Xlint:all", "-Werror", "-Xlint:-classfile", "-Xlint:-processing", "-Xlint:-try"))


This allows us to write:

try(ISentryLifecycleToken ignored = scopes.makeCurrent()) { ... }

and rely on AutoClosable to clean up scopes.

👍 why do you have to disable this warning? Does it complain every time you're not using the API with a try block?

It complains about not using the variable (called ignored here) inside the try {} block.

adinauer · 2024-04-12T12:58:09Z

sentry/src/main/java/io/sentry/Breadcrumb.java

  @Override
  @SuppressWarnings("JavaUtilDate")
  public int compareTo(@NotNull Breadcrumb o) {
+    // TODO also use nano time if equal


To fix breadcrumb ordering across multiple collections when combining scopes - so we can maintain insertion order as well as possible. If this isn't good enough we might need some atomic counter we increment for each breadcrumb.

Implemented in #3355

adinauer · 2024-04-12T13:00:48Z

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


  @Override
  public void removeTag(@NotNull String key) {
+    // TODO should this go to all scopes?


Deleting certain things from scope can be tricky with new API since deletions might go to the wrong scope. Maybe we should just delete from all scopes. On the other hand a user might just want to shadow something temporarily so maybe deleting everywhere isn't wanted.

When calling removeX on current scope where scope has been forked since adding e.g. the tag in a parent scope, the remove only removes it from the current scope, not from the parent.

Good point, yeah that's tricky!
From a user perspective, I expect it to behave in-sync with setting a value, so I wouldn't remove the value from all scopes.
I'm wondering if we could treat remove<xy>() like a set<xy>(null) instead.

When retrieving scope values we then would need to change the behavior as well. From ~

return current.value ?: scope.value ?: global.value

To something like this ~

return if (current.isSet) { current.value // can still be null } else if (scope.isSet) { scope.value } else { global.value }

Yeah I agree on not removing from all and making it behave the same way set does.

Not so sure about setting null and returning that because how do you ever unset this?

As a workaround for users actually wanting to clear certain values from all scopes, we could offer a ScopeType.ALL where we send set, remove etc. to all scopes, when they run Sentry.configureScope(ScopeType.ALL) { scope -> ... }

If users want to shadow something in the sense of temporarily unsetting, we'd have to use Optional for all values I guess.

adinauer · 2024-04-12T13:02:31Z

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

+  GLOBAL,
+
+  // TODO do we need a combined as well so configureScope
+  COMBINED;


This would allow users to use .configureScope() and be able to access the combined view of all scopes relevant, so they can read the same way SentryClient would.

The use case would be relevant for reading data from the scope right?

Yeah, cross platform SDKs (via InternalSentrySdk) are the primary use case atm. I'm about to play around with this but I'm gonna need help testing cross platform SDKs after the change.

adinauer · 2024-04-12T13:02:59Z

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

  }

  private IScope getSpecificScope(final @Nullable ScopeType scopeType) {
+    // TODO extract and reuse


move to CombinedScopeView as well?

Implemented in #3361

github-actions · 2024-04-12T13:07:30Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	398.35 ms	447.49 ms	49.14 ms
Size	1.70 MiB	2.28 MiB	595.97 KiB

Previous results on branch: feat/hsm-27-discussions

Startup times

Revision	Plain	With Sentry	Diff
`fc49fab`	391.51 ms	458.73 ms	67.22 ms

App size

Revision	Plain	With Sentry	Diff
`fc49fab`	1.70 MiB	2.28 MiB	595.97 KiB

adinauer · 2024-04-16T12:25:14Z

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

  }

  private @NotNull IScope getDefaultWriteScope() {
+    // TODO use Scopes.getSpecificScope?


Implemented in #3361

adinauer · 2024-04-16T12:27:40Z

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

  }

  private IScope getCombinedScopeView() {
+    // TODO create in ctor?


Implemented in #3361

adinauer · 2024-04-22T14:24:05Z

All discussions have been addressed in follow up PRs.

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 2 commits April 12, 2024 14:52

Use new API for kotlin coroutines and wrapers for Supplier/Callable

f0af5c3

Discussion TODOs

2f02001

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

adinauer commented Apr 12, 2024

View reviewed changes

adinauer commented Apr 16, 2024

View reviewed changes

Base automatically changed from feat/hsm-26-coroutine-wrapper to 8.x.x April 22, 2024 14:17

Merge branch '8.x.x' into feat/hsm-27-discussions

2b05019

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

adinauer deleted the feat/hsm-27-discussions branch April 22, 2024 14:24

Uh oh!

Uh oh!

Hubs/Scopes Merge 27 - Discussions #3352

Hubs/Scopes Merge 27 - Discussions #3352

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinauer Apr 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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-27-discussions

Startup times

App size

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinauer commented Apr 22, 2024

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 12, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading