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

WIP: System.Diagnostic wrappers #3238

Closed
wants to merge 12 commits into from
Closed

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Mar 25, 2024

Another experiment related to #3159

The plan here is to leverage System.Diagnostics.DiagnosticSource under the hood for tracing (this is the technology underpinning OTel in .NET). This has been part of .NET since version 5.0.0 so we'll enable it by default for users targeting net5.0 and later.

For users targeting .NET Framework or .NET Core 3.1 and earlier this will be opt in via the Sentry.DiagnosticSource package.

TODO

  • Sentry: SentryHttpMessageHandler and SentryGraphQLHttpMessageHandler create spans and propagates context
    • Tests
  • Sentry.AspNet: HttpContextExtensions creates spans and propagates context
    • Tests
  • Sentry.AspNetCore: SentryTracingMiddleware creates spans and propagates context
    • Tests
  • Sentry.Azure.Functions.Worker: SentryFunctionsWorkerMiddleware creates spans and propagates context
    • Tests
  • Sentry.DiagnosticSource: SentrySqlListener and EFDiagnosticSourceHelper create spans
    • Tests
  • Sentry.EntityFramework: SentryQueryPerformanceListener creates spans
    • Tests
  • Investigate some equivalent of SentryPropagator. That extends OpenTelemetry.Context.Propagation.BaggagePropagator so we'll need to extract the relevant bits to some helper class that is used by both BaggagePropagator and ActivitySpanProcessor. Need to work out when/why this gets leveraged in the OpenTelemetry code so that we can mimic this as well.
  • Do we need to wire up [some equivalent of] the OpenTelemetryTransactionProcessor? Possibly rename this to ActivityTransactionProcessor and move it to Sentry.DiagnosticSource...
  • Check uses of Hub.GetSpan(), such as Hub.GetTraceHeader and Hub.GetBaggage... how will these work with Activity.Current? How will propagation etc. work?
  • More tests... this is quite a big change and we want good test coverage of all the changes

Out of scope

Sentry.OpenTelemetry uses OpenTelemetry instrumentation (not the new tracing interfaces).

The following integrations are also out of scope as they don't include any tracing functionality:

  • Sentry.Android.AssemblyReader
  • Sentry.AspNetCore.Grpc
  • Sentry.Bindings.Android
  • Sentry.Bindings.Cocoa
  • Sentry.Extensions.Logging
  • Sentry.Google.Cloud.Functions
  • Sentry.Hangfire
  • Sentry.Log4Net
  • Sentry.Maui
  • Sentry.NLog
  • Sentry.Profiling
  • Sentry.Serilog

@jamescrosswell jamescrosswell changed the title Experimenting with System.Diagnostic wrappers WIP: System.Diagnostic wrappers Mar 26, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@jamescrosswell
Copy link
Collaborator Author

Unfortunately in some of our code the details of our internal tracing bleed out via a public interface in the SentryMessageHandler class:

/// <summary>
/// Starts a span for a request
/// </summary>
/// <param name="request">The <see cref="HttpRequestMessage"/></param>
/// <param name="method">The request method (e.g. "GET")</param>
/// <param name="url">The request URL</param>
/// <returns>An <see cref="ISpan"/></returns>
protected internal abstract ISpan? ProcessRequest(HttpRequestMessage request, string method, string url);
/// <summary>
/// Provides an opportunity for further processing of the span once a response is received.
/// </summary>
/// <param name="response">The <see cref="HttpResponseMessage"/></param>
/// <param name="span">The <see cref="ISpan"/> created in <see cref="ProcessRequest"/></param>
/// <param name="method">The request method (e.g. "GET")</param>
/// <param name="url">The request URL</param>
protected internal abstract void HandleResponse(HttpResponseMessage response, ISpan? span, string method, string url);

Both the ProcessRequest and HandleResponse methods include an ISpan? in their signature... and both of these methods are protected internal (which means available internally within any assembly that implements the abstract base class they're part of). 🤔

To avoid a breaking change then, it might be necessary to create copies of these classes that use the new ITraceSpan interface in their method signatures instead.

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