HttpClient Diagnostics: deprecated events issue

[lmolkova]

In PR dotnet/corefx#16393 we have changed logging (DiagnosticSource) in HttoClient and introduced a new events:

• System.Net.Http.HttpRequestOut.Start
• System.Net.Http.HttpRequestOut.Stop
• System.Net.Http.Exception

We kept events that have been previously fired, but deprecated them:

• System.Net.Http.Request
• System.Net.Http.Response

We also made new and old events mutually exclusive.

However there is a valid production scenario when there are two listeners in the application that want different set of events:
e.g. ApplicationInsights for the 'new' events and Glimpse for the 'old' events. With mutual exclusiveness, only 'new' listener will receive any events.

Besides Glimpse, VisualStudio consumes 'old' events, however I do not know whether it is internal logging or listener is somehow injected into the application where other listeners may exist; may be @nbilling can provide more details on it.
Potentially there are other consumers affected by this change.

Simple solution would be to check for deprecated event listener with IsEnabled method even if there was a listener to new event, however the assumption is that frequently there will be just one listener.
IsEnabled for deprecated events may be cached under the assumption that there are no consumers that dynamically subscribe and unsubscribe.

/cc @vancem @avanderhoorn @nbilling @cwe1ss

[lmolkova]

Another solution could be to use different DiagnosticListeners names so it will be cheaper to check if someone listens to one of them with no-arg IsEnabled()

[vancem]

Yes, we know about users of the old events get broken if there is also a client using the new events.

The idea was that the tools using the old events should quickly migrate to the new events.

In general to the extent possible our philosophy is to try not to compromise on the future for the sake of the past. Thus we should be investing effort in upgrading the tools that use the old events rather than in improving compatibility.

[lmolkova]

That is correct that tools could migrate to the new events, however end users need to update tools.
And the problem for end user that something just stopped working only because they updated AppInsights version to the latest one. It is not easy to find the root cause.
And while it should be easy to update Glimpse, updating VisualStudio could be much more difficult (however I still hope that it's internal VS logging).

[vancem]

If there is enough compatibility pain than we should be looking at fixing it. However we should have a clear understanding what the impact is, how likely it is to happen, and how easy is it to work around.

[cwe1ss]

Multiple listeners/tracers are a challenge for other reasons as well. Imagine both are calling SetParentId() with different IDs.
AFAIK, there's no good solution for this in OpenTracing yet. By default, it allows only one tracer but there are some concepts about a "TeeTracer" which is an indirection that allows for one main tracer (e.g. Application Insights) and one or more "passive" tracers (e.g. a prometheus metrics reporter).

[avanderhoorn]

I think part of the issue here is that if Glimpse or AI or anyone else wants to support v1 and v2. In this case our consuming code base would be listening to both events. The original though was to make these events mutually exclusive to avoid both events firing... hence if one consumer was listening to v2 events, the system would disable v1 events. We can probably handle this implementation detail for code we own (i.e. Glimpse, AI, etc) but others outside of MS have started listening to DS events and hence just arbitrary picking one vs the other doesn't seem to make much sense. Hence the thought was that if consumers who want to support v1 and v2 have some way of knowing if they should expect v2 events, they can disable their v1 listeners (i.e. they would return false from their IsEnabled callback). If we went this way, it would let v1 only consumers work just fine, it would let v2 consumers work just fine and it would let consumers listening to both v1 and v2 have a way of opting into v2 events (ignoring v1 events). The question is what does that "v2 is present" signal look like? Is it an event that is fired by DS or is it something else?

[ViktorHofer]

@karelz @danmosemsft @vancem I don't think somebody is working on this 2.0 issue?

[karelz]

@vancem @brianrob @valenis ping?

[vancem]

I am looking at this now.

[vancem]

Fixed by dotnet/corefx#19659