POTEL 1 - Use OpenTelemetry for Performance and `Scopes` propagation #3399

adinauer · 2024-04-30T08:45:09Z

📜 Description

Rely on Context for Scopes propagation by hooking into OpenTelemetry Context storage
Fork Scopes when new OpenTelemetry spans are created
Add a util for figuring out which storage mechanism for Scopes to use on startup
- If our OpenTelemetry code is available, it'll use Context based storage
- Otherwise it'll default to our own ThreadLocal based storage
SpanProcessor and Propagator are also involved in Scopes forking
SpanExporter combines OpenTelemetry spans into Sentry transactions and sends them
Add a new Gradle module sentry-opentelemetry-bootstrap so certain classes can be added to the bootstrap classloader
We use a global storage for linking OpenTelemetry spans and Sentry Scopes

💡 Motivation and Context

Make Java SDK more compatible with OpenTelemetry by forking Scopes along OpenTelemetry and using OpenTelemetry Context for propagation of Scopes.

💚 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-30T08:45:34Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

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

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- POTEL 1 - Use OpenTelemetry for Performance and `Scopes` propagation ([#3399](https://github.com/getsentry/sentry-java/pull/3399))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 9b41d51

github-actions · 2024-04-30T08:56:02Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	375.65 ms	447.29 ms	71.64 ms
Size	1.70 MiB	2.28 MiB	596.71 KiB

Baseline results on branch: 8.x.x

Startup times

Revision	Plain	With Sentry	Diff
`3e1e45b`	373.72 ms	439.33 ms	65.60 ms
`9f2c855`	424.20 ms	506.34 ms	82.14 ms

App size

Revision	Plain	With Sentry	Diff
`3e1e45b`	1.70 MiB	2.28 MiB	596.32 KiB
`9f2c855`	1.70 MiB	2.28 MiB	596.32 KiB

Previous results on branch: feat/potel-1-context-forking-and-basics

Startup times

Revision	Plain	With Sentry	Diff
`9931715`	374.85 ms	431.48 ms	56.62 ms
`9a9af11`	376.72 ms	442.33 ms	65.61 ms
`f5f9136`	395.57 ms	504.94 ms	109.36 ms

App size

Revision	Plain	With Sentry	Diff
`9931715`	1.70 MiB	2.28 MiB	596.01 KiB
`9a9af11`	1.70 MiB	2.28 MiB	596.17 KiB
`f5f9136`	1.70 MiB	2.28 MiB	596.01 KiB

lbloder · 2024-05-02T05:48:16Z

...-opentelemetry-bootstrap/src/main/java/io/sentry/opentelemetry/OtelContextScopesStorage.java

+public final class OtelContextScopesStorage implements IScopesStorage {
+
+  @Override
+  public ISentryLifecycleToken set(@Nullable IScopes scopes) {


@NotNull annotation missing, as defined in the interface

lbloder · 2024-05-02T05:59:55Z

...ntry-opentelemetry-bootstrap/src/main/java/io/sentry/opentelemetry/SentryContextWrapper.java

+
+  private static @Nullable IScopes getCurrentSpanScopesFromGlobalStorage(
+      final @NotNull Context context) {
+    @Nullable final Span span = Span.fromContext(context);


Shouldn't this be Span.fromContextOrNull()?
As Span.fromContext() would return PropagatedSpan.INVALID if the key is not present in the context, thus the null check here would not work

lbloder · 2024-05-02T06:01:29Z

...try-opentelemetry-bootstrap/src/main/java/io/sentry/opentelemetry/SentryWeakSpanStorage.java

+ * can exist.
+ */
+@ApiStatus.Internal
+public final class SentryWeakSpanStorage {


Should we rename this to SentryWeakScopesStorage ?

We likely have to change what's stored in follow up PRs.

lbloder · 2024-05-03T07:34:02Z

...entry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/PotelSentrySpanProcessor.java

+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+public final class PotelSentrySpanProcessor implements SpanProcessor {


Indirectly related, but the old SentrySpanProcessor can be deleted as it isn't used anymore, right?

We'll keep it around for a while and maybe deprecate it so users can upgrade to the new version when they need it. There should either be some config to switch between POTel and old way of OTel or separate dependencies.

lbloder · 2024-05-03T07:35:44Z

...ization/src/main/java/io/sentry/opentelemetry/SentryAutoConfigurationCustomizerProvider.java

@@ -140,7 +144,11 @@ private static class VersionInfoHolder {

  private SdkTracerProviderBuilder configureSdkTracerProvider(
      SdkTracerProviderBuilder tracerProvider, ConfigProperties config) {
-    return tracerProvider.addSpanProcessor(new SentrySpanProcessor());
+    // TODO [POTEL] configurable or separate packages for old vs new way


Do we want to keep the old way around?

Yes, see above.

lbloder · 2024-05-03T07:37:27Z

...try-opentelemetry-bootstrap/src/main/java/io/sentry/opentelemetry/SentryWeakSpanStorage.java

+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+/**


We should update the class comment here at some point

lbloder · 2024-05-03T07:44:29Z

...-opentelemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SpanNode.java

+  private final @NotNull String id;
+
+  // TODO [POTEL] should this be ReadableSpan? if so weak or strong ref?
+  private @Nullable SpanData span;


Do we ever need to store a ReadableSpan for information that is not stored in a SpanData?
If we get a ReadableSpan as input from somewhere we could call ReadableSpan.toSpanData

Let's revisit this when implementation is further along. Currently we may not need ReadableSpan.

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 19 commits April 12, 2024 14:55

Discussion TODOs

2f02001

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

Changes according to reviews of previous PRs

23506c5

more

c9b6f8b

even more

696a809

isEnabled checks client instead of having a property on Scopes

847200d

Use SentryOptions.empty

8e86d3b

Remove Hub

06db228

Use OpenTelemetry for Performance and Scopes propagation

37ab4d0

adinauer requested review from romtsn, stefanosiano and markushi as code owners April 30, 2024 08:45

adinauer requested a review from lbloder April 30, 2024 08:45

Base automatically changed from feat/hsm-42-remove-hub to 8.x.x May 2, 2024 13:17

Merge branch '8.x.x' into feat/potel-1-context-forking-and-basics

c57b2d3

lbloder reviewed May 3, 2024

View reviewed changes

adinauer added 2 commits May 7, 2024 06:37

Merge branch '8.x.x' into feat/potel-1-context-forking-and-basics

5b15128

Merge branch '8.x.x' into feat/potel-1-context-forking-and-basics

9b41d51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POTEL 1 - Use OpenTelemetry for Performance and `Scopes` propagation #3399

POTEL 1 - Use OpenTelemetry for Performance and `Scopes` propagation #3399

adinauer commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

Baseline results on branch: 8.x.x

Startup times

App size

Previous results on branch: feat/potel-1-context-forking-and-basics

Startup times

App size

lbloder May 2, 2024

lbloder May 2, 2024

adinauer May 10, 2024

lbloder May 2, 2024

adinauer May 10, 2024

lbloder May 3, 2024

adinauer May 10, 2024

lbloder May 3, 2024

adinauer May 10, 2024

lbloder May 3, 2024

adinauer May 10, 2024

lbloder May 3, 2024

adinauer May 10, 2024

POTEL 1 - Use OpenTelemetry for Performance and Scopes propagation #3399

Are you sure you want to change the base?

POTEL 1 - Use OpenTelemetry for Performance and Scopes propagation #3399

Conversation

adinauer commented Apr 30, 2024 • edited

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Apr 30, 2024 • edited

Instructions and example for changelog

github-actions bot commented Apr 30, 2024 • edited

Performance metrics 🚀

Baseline results on branch: 8.x.x

Startup times

App size

Previous results on branch: feat/potel-1-context-forking-and-basics

Startup times

App size

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

POTEL 1 - Use OpenTelemetry for Performance and `Scopes` propagation #3399

POTEL 1 - Use OpenTelemetry for Performance and `Scopes` propagation #3399

adinauer commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited