Runtime: API review: Allow suppressing Activity creation in System.Net.HttpClient

[lmolkova]

Background

See dotnet/runtime#29291: it enables propagation of Activity in HttpClient if there is a parent Activity. I.e. allows ASP.NET Core (or other incoming layer instrumentation) to control distributed tracing.

End-to-end distributed tracing requires Activity creating on incoming (ASP.NET Core) layer and outgoing. Incoming instrumentation gets context from request headers, outgoing request instrumentation propagates tracing context further.

Now It is controlled with DiagnosticListener: user app needs to listen to both: ASP.NET Core and HttpClient sources to enable end-to-end and enable Activity creation (and propagation).

ASP.NET Core wants to support distributed tracing by default. Using DiagnosticListener for HttpClient is not desirable:

• events with payloads are not needed, only propagation is needed - it also affects perf
• listening to DiagnosticListener is complicated and may interfere with user app or tracing system

In the absence of parent Activity instrumentation is still enabled with DiagnosticListener.

We also want to allow users to disable the feature at various levels as we expect some scenarios where propagation is not desirable because of trust boundaries, CORS configuration, etc.

Proposal

public partial class HttpClientHandler : System.Net.Http.HttpMessageHandler
 {
+  public bool SuppressActivityPropagation { get { ... } set { ... } }
+  public static bool SuppressGlobalActivityPropagation { get { ... } set { ... } }
}

• static HttpClientHandler.SuppressGlobalActivityPropagation. Turns off propagation for AppDomain (even if there is an active DiagnosticListener).

• instance HttpclientHandler.SuppressActivityPropagaion. Turns off propagation for a specific client (even if there is an active DiagnosticListener).

• For request-level control, users can set parent Activity to null (and even implement handler with this behavior). If we'll find that this is actively used, we may come up with better API for it:

Draft PR with implementation dotnet/corefx#38280

[davidsh]

cc: @dotnet/ncl @stephentoub

[davidsh]

IMHO, I think it premature to mark this issue as "api-ready-for-review". I understand the motivations of it, but I think more discussions with area owners (networking) is needed before it can be discussed in the API review meeting.

The current diagnostic tracing model that DiagnosticListener uses in HttpClientHandler uses the HttpClient architecture handler pipeline with minimal exposure on the API surface of the HttpClient and related objects (HttpClientHandler et. al). I'm concerned with adding explicit API properties for tracing control (instance and static) on the HttpClientHandler object. Perhaps a better place for control knobs would be on a new public API for a DiagnosticListener class type and properties.

If we need control like this for .NET Core 3.0, perhaps adding another mechanism like AppContext switches etc. would be better for now for .NET Core 3.0.

[davidsh]

@karelz

[stephentoub]

For request-level control, users can set parent Activity to null (and even implement handler with this behavior). If we'll find that this is actively used, we may come up with better API for it:

Could this be sufficient for 3.0? Seems like everything else could be done in terms of it, and we wouldn't need to try to jam in a whole new set of APIs last minute.

[lmolkova]

@davidsh you are right, I changed label to api-needs-work

Perhaps a better place for control knobs would be on a new public API for a DiagnosticListener class type and properties.

Today DiagnosticsHandler is not public. Exposing it creates other issues:

• it will be possible to add it to the pipeline in addition to the one added automatically in HttpClientHandler
• need to pass SuppressActivityPropagation to the instance of DiagnosticsHandler created by HttpClientHandler.

So as long as DiagnosticsHandler is added implicitly, we have to control its behavior with HttpClientHandler.

AppContext switch may be an alternative to static SuppressGlobalActivityPropagation, while handler with Activity.Current = null can substitute to some extent SuppressActivityPropagation (instance property).

@davidfowl @glennc do you think this is reasonable?

[davidfowl]

Yes it's reasonable for 3.0

[lmolkova]

Thanks for the feedback everyone, closing this.