Start and Stop an Activity for each request

[glennc]

corefx just added the Activity type and Start/Stop extension methods on DiagnosticSource: dotnet/corefx#15103

Usage guide is here: https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.DiagnosticSource/src/ActivityUserGuide.md

The intent here is that we would call StartActivity as a request starts and StopActivity as it ends. We would use the value of TBD headers to flow the parentId and baggage values. I would imagine this is a case where we look for a default header but the user could change it with hosting config.

[glennc]

@Tratcher I assume we could do this either as something like a startup filter or in HttpApplication wrapping the call to the requestDelegate. What do you think we should do?

@cwe1ss IIRC you talked about the name of the default header and wanting to make sure we are inline with OpenTracing. That conversation should happen with @vancem and @lmolkova. Do any of you have a preferred place to do that?

[davidfowl]

The only unfortunate thing is that we'd have duplicate events fired. Pre-activity but we can live with it. This also needs to be zero cost when off. It would go into HostingApplication with the rest of his logic

[benaadams]

Diagnostics is currently not zero cost when off 😢 dotnet/corefx#15971 (comment)

[glennc]

@benaadams Yeah, I saw that. We will have to chase it and make sure we get perf numbers for all this.

I figured HostingApplication (is what I meant when I said HttpApplication since HttpApplication is the interface didn't register that the class implementation wasn't the same). We could technically remove the pre-activity Begin/EndRequest events. But I don't think it would be worth it assuming it is cheap enough to do the IsEnabled check.

[lmolkova]

@glennc
We have an HTTP header proposal, it have been reviewed by several interested teams in Microsoft. I will present it to OneAPI Working Group on next Tuesday and plan to drive it further to IETF community.

@cwe1ss could you please also have a look at the proposal? It would be great to hear your feedback.
It does not align with Zipkin headers and we want to provide compatibility story and possibly support multiple sets of incoming requests headers.

[Tratcher]

Observations:

• This is only a draft and is likely to change
• There are already several other 'standards' like this
• HostingApplication operates on the request before any possible developer intervention. This would be the first case where it reacted to external input.

Recommendation: We should implement this first as middleware.

• The middleware can be added by various components (templates, UseAzure, Host injection, etc..).
• We don't want to bake anything into Hosting that's likely to change
• The dev may choose to use an alternate header/standard to track their activities
• As middleware the dev still has some control over the ordering, and/or has the chance to inspect/reject/tweak the request before any actions are taken on it.

[davidfowl]

@Tratcher agreed.

[lmolkova]

@Tratcher @davidfowl @SergeyKanzhelev @vancem

Regarding 'standard':

• We are going to implement it in System.Net.Http, and once .net ships we cannot really change it anyway
• Several interested teams including ApplcationInsights signed it off.
• Could you please share other similar standards?

Middleware:

• control over ordering is also a bad thing, because lets developer do logging before activity instrumentation. I can assume that some things may happen before activity creation: e.g. throttling, but for other cases it should be first thing to happen. If user wants to inspect/reject/tweak Activity instrumentation, he can do it in DiagnosticLitener.IsEnabled callbacks for all scenarios including throttling.

• As I mentioned before, headers will be added to System.Net.Http, so the will probably never change after .netstandard 2.0 ships.

• We thought about supporting other headers/standards via some configuration: e.g. environment variable. Header parsing is a small part and it would be a pity if we require developers to implement the whole Activity creation logic, keeping in mind all possible scenarios, just to support other header name. Note that every service may expect to receive different correlation headers schemas.

[Tratcher]

The spec already links to other related tracing standards: https://github.com/lmolkova/correlation/blob/master/http_protocol_proposal_v1.md#industry-standards

This is un-trusted input from an external party. The developer must remain in control of any request processing and the order in which that happens. We have templates and other means of putting the middleware where we think it belongs, but the developer is free to move/exclude/replace it as they see fit.

Keep in mind that RFC submissions rarely survive intact. If you really want it to be a standard you need to be prepared to update your implementation accordingly. Otherwise why submit it at all?

[lmolkova]

@Tratcher

The problem with opentracing or dapper, that they don't really define any headers.
Zipkin defines some pretty custom ones, that also don't fully support opentracing:
X-B3-TraceId, X-B3-SpanId, X-B3-ParentSpanId, X-B3-Sampled, X-B3-Flags

Activity is designed to be controlled with DiagnosticSource, if it's implemented in middleware, then there will be 2 ways to contol it and users would need to do both:

1. use middleware
2. subscribe to diagnostic source.

It's quite useful to keep Activity creation first thing to happen I don't really see why it is different from diagnostics logic (and context creation) already implemented in hosting layer these days:

Activity creation also fires DiagnosticSource events and then you have to deal with ambiguity where some events (Microsoft.AspNetCore.Hosting.BeginRequest) happen before Activity events and users need to remember that Activity.Current is not available in those callbacks.

So, practically logging (with Microsoft.AspNetCore.Hosting events) already happens before any middleware and Activity.Current must be available before these events are fired.

Keep in mind that RFC submissions rarely survive intact. If you really want it to be a standard you need to be prepared to update your implementation accordingly. Otherwise why submit it at all?

Next step is to show it to OneAPI group. Depending on the feedback we will decide on RFC submissions. We have not considered all options, but there are different paths for IETF standards including "informational" and "experimental" publications.

[benaadams]

Could move DiagnosticSource to middleware and then all the Diagnostics logging is together? Also means you don't pay the virtual dispatch cost on .IsEnabled(...) for false if you take the middleware out?