New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
POTEL 1 - Use OpenTelemetry for Performance and Scopes
propagation
#3399
base: 8.x.x
Are you sure you want to change the base?
Conversation
…pes-merge-2-add-scopes
…ainScopes to rootScopes
Instructions and example for changelogPlease add an entry to 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 |
Performance metrics 🚀
|
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 |
public final class OtelContextScopesStorage implements IScopesStorage { | ||
|
||
@Override | ||
public ISentryLifecycleToken set(@Nullable IScopes scopes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NotNull
annotation missing, as defined in the interface
|
||
private static @Nullable IScopes getCurrentSpanScopesFromGlobalStorage( | ||
final @NotNull Context context) { | ||
@Nullable final Span span = Span.fromContext(context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
* can exist. | ||
*/ | ||
@ApiStatus.Internal | ||
public final class SentryWeakSpanStorage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename this to SentryWeakScopesStorage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We likely have to change what's stored in follow up PRs.
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
|
||
public final class PotelSentrySpanProcessor implements SpanProcessor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indirectly related, but the old SentrySpanProcessor
can be deleted as it isn't used anymore, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep the old way around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see above.
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the class comment here at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
private final @NotNull String id; | ||
|
||
// TODO [POTEL] should this be ReadableSpan? if so weak or strong ref? | ||
private @Nullable SpanData span; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revisit this when implementation is further along. Currently we may not need ReadableSpan
.
📜 Description
Context
forScopes
propagation by hooking into OpenTelemetryContext
storageScopes
when new OpenTelemetry spans are createdScopes
to use on startupContext
based storageThreadLocal
based storageSpanProcessor
andPropagator
are also involved inScopes
forkingSpanExporter
combines OpenTelemetry spans into Sentry transactions and sends themsentry-opentelemetry-bootstrap
so certain classes can be added to the bootstrap classloaderScopes
💡 Motivation and Context
Make Java SDK more compatible with OpenTelemetry by forking
Scopes
along OpenTelemetry and using OpenTelemetryContext
for propagation ofScopes
.💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps