dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.21k stars 4.72k forks source link

HttpHandlerDiagnosticListener may have issues with W3C tracecontext handling #37124

Open cg110 opened 4 years ago

cg110 commented 4 years ago

Description

I was looking adopt W3C trace contexts, but with .net framework (we have WCF and workflow code), so have been looking at the Activity class in: System.Diagnostics.DiagnosticSource

The Activity and Diagnostics have proved very useful.

There is an accompanying helper class: HttpHandlerDiagnosticListener

I was considering using it to track outbound http requests, but it appears there is some inconsistency in it's RaiseResponseEvent events. This version, for System.Net.Http.Desktop.HttpRequestOut.Stop: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs#L661 handles W3C headers.

It appears this version, for System.Net.Http.Desktop.HttpRequestOut.Ex.Stop : https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs#L674 doesn't handle the W3C header, so won't write the event.

I also suspect the raise event check here: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs#L586 should also look for the w3c header.

Configuration

System.Diagnostics.DiagnosticSource 4.7.1 .net Framework 4.7.2 windows 10 1909

Regression?

Probably not a regression, as W3C support is relatively new.

Other information

I'm still getting my head around trace context, so I could be mis-understanding how they should work

ghost commented 4 years ago

Tagging subscribers to this area: @dotnet/ncl Notify danmosemsft if you want to be subscribed.

karelz commented 4 years ago

@lmolkova can you please advise here?

jnovick commented 4 years ago

I am facing the same issue.

cg110 commented 4 years ago

Having had time to digest, I think the last part is my mis-understanding, I've updated the main issue to indicate that.

I realized that what it's doing is capturing the client side duration etc of a http call (which means that the tls handshake, latency etc can be measured)

So you end up with:

This means that telemetry etc can track the times, as we've seen fast server calls, but the client be slow, and it's the overhead of connecting, authing, handshaking etc.

I believe that the first parts still stand that the code should be checking the TraceParent Header as well as the request header.