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

.NET SDK powered by OTel #3159

Closed
Tracked by #4
smeubank opened this issue Jan 23, 2024 · 3 comments · Fixed by #3288
Closed
Tracked by #4

.NET SDK powered by OTel #3159

smeubank opened this issue Jan 23, 2024 · 3 comments · Fixed by #3288

Comments

@smeubank
Copy link
Member

smeubank commented Jan 23, 2024

Description:

See parent ticket for more project background.

We would like to investigate using OpenTelemetry's .NET SDK to replace Sentry performance package in our own SDK. This would allow our users to benefit from all of the instrumentation of the open standard's SDK, while allowing us to tailor the experience.

Requirement:

The OTel SDK should run the show in terms of instrumentations and collecting telemetry, our SDK should only do some fine tuning to ensure data is flagged for ingestion properly (OTel attributed as tags for example)

Open Point:

  • Sentry will have a product ready OTLP endpoint, with this, we have an opportunity to exclude the transaction model all together from our implementation. It should be investigated if we need to adapt our existing trxn model.
  • Does OTLP give us a simpler route?

Information:

Updated requirements

See Discord chat for context.

  • When Sentry is initialized using OpenTelemetry instrumentation, calls to .StartSpan() or .StartChild() on the Sentry SDK should result in spans that fit seamlessly into the auto-instrumentation provided by OpenTelemetry.
  • We also need to ensure the correct trace id is used regardless of whether the backend is otel or sentry only

Sentry Transactions with OTEL Child Spans

Currently leads to unexpected outcomes.

var transaction = SentrySdk.StartTransaction("Program", "Main");
try
{
   Console.WriteLine("Hello World!");
   using (var task = activitySource.StartActivity("Task 1"))
   {
       task?.SetTag("Answer", 42);
       Thread.Sleep(100); // simulate some work
   }


   using (var task = activitySource.StartActivity("Task 2"))
   {
       task?.SetTag("Question", "???");
       Thread.Sleep(100); // simulate some more work
   }


   Console.WriteLine("Goodbye cruel world...");
}
finally
{
   transaction.Finish();
}

The Sentry transaction gets ignored. If we turn on Debug mode we see the following warning:

Warning: Attempted to start a transaction via Sentry instrumentation when the SDK is configured for OpenTelemetry instrumentation.  The transaction will not be created.

And as a result the two Activities show up as separate transactions:
image

We could run into this if people wanted to use OTel to take advantage of automatic tracing for some tech (e.g. Redis) but otherwise they had been instrumenting their app for performance using Sentry... so we probably need to fix this.

Scopes

@smeubank smeubank changed the title .NET SDK .NET SDK powered by OTel Jan 23, 2024
@smeubank smeubank transferred this issue from getsentry/team-sdks Feb 20, 2024
@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Feb 23, 2024

A couple of thoughts:

  • It seems like a prerequisite to this is aligning the way Sentry and OTEL represent traces... specifically, OTEL doesn't distinguish between Transactions and Spans (in OTEL you can have a span that has no parent, but otherwise it has all the same properties etc. of other parented spans).
  • Are we planning on adding a hard dependency on OpenTelemetry to the Sentry package? Or would we move Tracing capabilities to the Sentry.OpenTelemetry package?
  • For integrations where OpenTelemetry instrumentation already exists, would we be removing our proprietry instrumentation? For example, Sentry.AspNetCore has two bits of middleware... SentryMiddleware and SentryTracingMiddleware. Would the SentryTracingMiddleware be obsolete (replaced by OpenTelemetry tracing for ASP.NET Core)? All that we would really retain then would be the crash reporting?
  • Are there some integrations that we maintain that don't have OpenTelemetry equivalents and what do we do with those? For example, if we want to move to using OpenTelemetry Activity for tracing, presumably integrations like DiagnosticSource that take events from DiagnosticListener and create SentrySpans from these would need to generate OpenTelemetry Activities from those DiagnosticListener events instead. These are not really Sentry specific integrations/packages anymore - they become general OpenTelemetry adapters that could potentially be donated to and maintained in another repo (assuming equivalents don't already exist - ideally someone has already built these for OpenTelemetry and we could just obsolete/retire our own integrations... either way, I think we're moving to a model where we're leveraging integrations that are maintained by the broader .NET community).
  • Does it make sense for Sentry to have it's own OTLP_ENDPOINT_URL? From what I can tell, you'd do that if Sentry was not running in the same process as the application that was generating the telemetry... so the telemetry would be serialized via json/protobuf or whatever and sent to the endpoint, where it got deserialized and processed/stored. In our case, if Sentry is running in the same process, perhaps we're better off retaining the SentrySpanProcessor to collect traces? Or are we thinking of removing this from the SDKs (i.e. the endpoints would sit on Sentry.io). Probably needs a bit of research/consideration...

Overall, this seems like a really really big piece of work. We might need to break it down into bite sized chunks. Possibly merging Spans and Transactions would be a good first step?

@bitsandfoxes
Copy link
Contributor

Are we planning on adding a hard dependency on OpenTelemetry to the Sentry package? Or would we move Tracing capabilities to the Sentry.OpenTelemetry package?

This remains to be discussed. Ideally, users would not need to know or care where spans come from.

For integrations where OpenTelemetry instrumentation already exists, would we be removing our proprietary instrumentation?

There is quite a big gap in what versions are supported by Sentry vs OTel. I'd go on a case-by-case basis to go with the one offering more/better/more detailed support.

@jamescrosswell jamescrosswell self-assigned this Mar 11, 2024
@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Mar 12, 2024

Transaction vs Span

+ AddBreadcrumb(Breadcrumb breadcrumb) : void
+ Breadcrumbs : IReadOnlyCollection<Breadcrumb>
+ Contexts : SentryContexts
 Description : string?
+ Distribution : string?
 EndTimestamp : DateTimeOffset?
+ Environment : string?
+ EventId : SentryId
 Extra : IReadOnlyDictionary<string,object?>
+ Fingerprint : IReadOnlyList<string>
 GetTraceHeader() : SentryTraceHeader
 IsFinished : bool
+ IsParentSampled : bool?
 IsSampled : bool?
+ Level : SentryLevel?
 Measurements : IReadOnlyDictionary<string,Measurement>
+ Name : string
+ NameSource : TransactionNameSource
 Operation : string
 ParentSpanId : SpanId?
+ Platform : string?
+ Release : string?
+ Request : SentryRequest
+ SampleRate : double?
+ Sdk : SdkVersion
 SetExtra(string key, object? value) : void
 SetMeasurement(string name, Measurement measurement) : void
 SetTag(string key, string value) : void
 SpanId : SpanId
+ Spans : IReadOnlyCollection<SentrySpan>
 StartTimestamp : DateTimeOffset
 Status : SpanStatus?
 Tags : IReadOnlyDictionary<string,string>
 TraceId : SentryId
 UnsetTag(string key) : void
+ User : SentryUser
 WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? logger) : void

- SentrySpan(ISpan tracer)
- SentrySpan(SpanId? parentSpanId, string operation)
+ SentryTransaction(ITransactionTracer tracer)
+ SentryTransaction(string name, string operation, TransactionNameSource nameSource)
+ SentryTransaction(string name, string operation)

-FromJson(JsonElement json) : SentrySpan
+FromJson(JsonElement json) : SentryTransaction

Additionally ITransactionTracer contains a GetLastActiveSpan() : ISpan? method, which ISpanTracer and SpanTracer lack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants