[WIP] OWIN Instrumentation for OpenTelemetry #1472
Conversation
|
|
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| private Activity StartActivity(IOwinContext owinContext, out bool hasDiagnosticListener) |
There was a problem hiding this comment.
The recommendation is to use ActivitySource based instrumentation for any new instrumentations. DiagnosticListener based ones are "legacy".
This instructions are being documented here (WIP):
#1467
I can help with replacing the DS -> ActivitySource ones.
There was a problem hiding this comment.
Thanks @cijothomas, I'll replace DiagnosticListener with ActivitySource.
| var activity = new Activity(ActivityName); | ||
|
|
||
| // Based on https://github.com/microsoft/ApplicationInsights-dotnet/blob/2.15.0/WEB/Src/Web/Web/Implementation/RequestTrackingExtensions.cs#L41 | ||
| if (!activity.Extract(owinContext.Request.Headers)) |
There was a problem hiding this comment.
We need to move on to Propagagators.DefaultTextMapPropagator approach.
There was a problem hiding this comment.
Can you please point me to some examples?
There was a problem hiding this comment.
|
|
||
| if (traceParents != null && traceParents.Count > 0 && !string.IsNullOrEmpty(traceParents[0])) | ||
| { | ||
| // there may be several Request-Id or traceparent headers, but we only read the first one |
There was a problem hiding this comment.
This would go against the W3C TraceContext spec.
Here goes the compliance test FYI https://github.com/w3c/trace-context/blob/1ea757e565f3f68f453ebd62a010e6ae9d9be5f2/test/test.py#L134.
There was a problem hiding this comment.
Once Propagator API is used, this would get handled by the TraceContextPropagator, and libraries don't have to write this code.
| internal static class HeaderNames | ||
| { | ||
| public static readonly string CorrelationContext = "Correlation-Context"; | ||
| public static readonly string RequestId = "Request-Id"; |
| // // Microsoft.Net.Http.Headers headers (from https://github.com/dotnet/aspnetcore/blob/v5.0.0-rc.2.20475.17/src/Http/Headers/src/HeaderNames.cs) | ||
| internal static class HeaderNames | ||
| { | ||
| public static readonly string CorrelationContext = "Correlation-Context"; |
There was a problem hiding this comment.
The W3C Baggage standard has decided to call it "Baggage".
| /// <summary> | ||
| /// Instruments incoming request with System.Diagnostics.Activity and notifies listeners with DiagnosticsSource. | ||
| /// </summary> | ||
| public sealed class DiagnosticsMiddleware : OwinMiddleware |
|
|
||
| namespace OpenTelemetry.Instrumentation.Owin.Implementation | ||
| { | ||
| internal class HttpInListener : ListenerHandler |
There was a problem hiding this comment.
@denisivan0v, your middleware approach is pretty neat! My gut says you won't need this OpenTelemetry.Instrumentation.Owin project once you convert your instrumentation to use ActivitySource as suggested earlier. That is, by starting/stopping an Activity from your middleware using an ActivitySource allows the OpenTelemetry SDK to directly listen to the ActivitySource.
| /// <summary>Adds a component to the OWIN pipeline for instrumenting incoming request with System.Diagnostics.Activity and notifying listeners with DiagnosticsSource.</summary> | ||
| /// <param name="appBuilder">The application builder.</param> | ||
| /// <returns>The application builder instance.</returns> | ||
| public static IAppBuilder UseOpenTelemetry(this IAppBuilder appBuilder) |
There was a problem hiding this comment.
Might want to document that this middleware should be applied as early as possible so as to time the entire middleware chain.
|
Closing old PRs. Please re-open when resuming work on this. Thank you! |
Fixes #.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes