Skip to content
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

Experimenting with implicit OpenTelemetry initialization #3229

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Mar 18, 2024

Playing around with one idea to implement:

This is a total hack at the moment, just to get an idea of the kinds of challenges involved.

Approach

OpenTelemetry Tracing is configured via a Static API that returns a TracerProviderBuilderBase.

In the context of a Console application especially (where there's no common IServiceCollection for DI), it's relatively difficult to work out whether OpenTelemetry has already been initialized or not. The OpenTelemetry static API doesn't provide an Sdk.Current property or anything similar.

However one possible solution might be to leverage our own AddSentry extension method:

public static TracerProviderBuilder AddSentry(this TracerProviderBuilder tracerProviderBuilder, TextMapPropagator? defaultTextMapPropagator = null)
{
defaultTextMapPropagator ??= new SentryPropagator();
Sdk.SetDefaultTextMapPropagator(defaultTextMapPropagator);
return tracerProviderBuilder.AddProcessor(ImplementationFactory);
}

If we always initialise OpenTelemetry when initializing Sentry, and we do so using the above extension method... we can guarantee it will be called at least once... If this extension method gets called twice, we know the initialization we made internally was unecessary and can either skip our own initialization of the Otel SDK or dispose of any TracerProvider we created previously.

Broadly speaking, this seems to work... there are a few challenges to making this production ready however.

Challenges

  • This POC uses the Sdk.CreateTracerProviderBuilder method. In an ASP.NET Core app we would need to use the OpenTelemetryBuilder extensions instead.
    • If we knew OpenTelemetry was being used as the trace implementation for Sentry then we could wire this up in the SentryOptions class and override it in SentryAspNetCoreOptions etc. to be platform specific.
  • If we're automatically instrumenting OpenTelemetry we have to make some assumptions about the ActivitySource names to listen to. We'd probably need to create our own ActivitySources and use these internally (e.g. in the implementation of IHub.StartTransaction) to start new OpenTelemetry activities instead of creating Sentry Spans.
  • Wiring this up has a dependency on OpenTelemetry, so we'd have to move a bunch of the Tracing stuff to the Sentry.OpenTelemetry package... if people wanted Tracing, they'd need to add that NuGet package to their project...
  • If we did want to continue to support both Sentry's proprietary tracing and OpenTelemetry tracing, we'd need to create some common interface and have an implementation of this for Sentry plus another for OpenTelemetry.
  • We'd need to work out what to do with our own integrations that are part crash reporting and part tracing. In some cases the tracing part would be obsolete... for example our ASP.NET Core integration (the tracing has already been done in OpenTelemetry by Microsoft). In other cases, we'd need to work out what to do however. This needs a bit of an audit of our integrations and a plan for what to do about them.

Copy link
Contributor

github-actions bot commented Mar 18, 2024

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

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

- Experimenting with implicit OpenTelemetry initialization ([#3229](https://github.com/getsentry/sentry-dotnet/pull/3229))

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 20f807a

@bruno-garcia
Copy link
Member

Wiring this up has a dependency on OpenTelemetry, so we'd have to move a bunch of the Tracing stuff to the Sentry.OpenTelemetry package... if people wanted Tracing, they'd need to add that NuGet package to their project...

No way around this? We'd essentially be tying the sdk to OTels SDK and it's a no return thing with lots of things to be aware of

@bruno-garcia
Copy link
Member

We'd need to work out what to do with our own integrations that are part crash reporting and part tracing. In some cases the tracing part would be obsolete... for example our ASP.NET Core integration (the tracing has already been done in OpenTelemetry by Microsoft). In other cases, we'd need to work out what to do however. This needs a bit of an audit of our integrations and a plan for what to do about them.

this could be a good thing before we commit to anything.

A list of things we'd be losing and getting by changing for example. Suggestion migration path. etc.

It's a big enough change the would benefit having good upfront research and detailed documentation of what this would mean to us. It's 4 years worth of installations literally tens of thousands of apps out there at minimum that rely on this

@jamescrosswell
Copy link
Collaborator Author

Wiring this up has a dependency on OpenTelemetry, so we'd have to move a bunch of the Tracing stuff to the Sentry.OpenTelemetry package... if people wanted Tracing, they'd need to add that NuGet package to their project...

No way around this? We'd essentially be tying the sdk to OTels SDK and it's a no return thing with lots of things to be aware of

I'll look into abstracting tracing activities (like creating/finishing spans) out into an interface with the specific implementation of that interface (Sentry vs OTEL) as something that gets registered during Init.

@bruno-garcia bruno-garcia removed their request for review June 26, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants