[API Proposal]: DiagnosticListener support for testing

johncrim commented 1 year ago

Background and motivation

An aspect of DiagnosticListener that has puzzled me is the fact that all DiagnosticListener instances are added to a global (DiagnosticListener.AllListeners), which makes any use or validation of diagnostics in tests difficult / or it requires that all tests be run serially.

To clarify: If more than one test is run at a time, and some code in the test creates a DiagnosticListener, the events sent to that listener can come from any test, and the events received by a test can be triggered by an operation in any other test that is running.

I'm sure this is a well-known limitation, but I would appreciate feedback on whether this is something that the dotnet team is interested in fixing; or if it's never going to be fixed and I should just work around it; or if I should prefer a newer API (eg OpenTelemetry) for exposing non-message (ILogger) diagnostics.

I'm most interested in being able to scope listeners and events to a DI Container; so the listeners could be per test or per fixture.

API Proposal

The leading requirement is obviously backward compatibility; so by default DiagnosticListener.AllListeners should be a global and behave the same as before.

A class like this would need to be added:

public sealed class DiagnosticListenerCollection 
{
    public void Add(DiagnosticListener listener);
    // Other ICollection methods?
}

And a constructor overload would need to be added to DiagnosticListener to store the DiagnosticListener in the DiagnosticListenerCollection instead of the global collection:

public partial class DiagnosticListener 
{

    public DiagnosticListener(string name, DiagnosticListenerCollection? listenerCollection)
    {
        Name = name;
        if (listenerCollection != null)
        {
            listenerCollection.Add(this);
        }
        else 
        {
            // Store listener in global list, same as current impl
        }
    }

...

And then different libraries which provide one or more DiagnosticListeners would (ideally) add support for using a DiagnosticListenerCollection, if present, to put the listener into, like below.

API Usage


serviceCollection.AddSingleton<DiagnosticListenerCollection>();
serviceCollection.AddSingleton<HttpClientFactory>();
serviceCollection.AddTransient<HttpClient>((HttpClientFactory factory) => factory.CreateHttpClient());

...

public class HttpClientFactory : IDisposable
{
    private readonly DiagnosticSource _httpLogger;

    public HttpClientFactory(DiagnosticListenerCollection? diagnosticListeners) {
        // Handle scoped or global DiagnosticListenerCollection
        _httpLogger = new DiagnosticListener("System.Net.Http", diagnosticListeners);
    }

    public HttpClient CreateHttpClient() => new HttpClient(_httpLogger);

    public void Dispose() => _httpLogger.Dispose();
}

public class HttpClient {

    private readonly DiagnosticSource _httpLogger;

    internal HttpClient(DiagnosticSource httpLogger)
    {
        _httpLogger; = httpLogger;
    }

    public byte[] SendWebRequest(string url)
    {
        if (_httpLogger.IsEnabled("RequestStart"))
        {
            _httpLogger.Write("RequestStart", new { Url = url });
        }
        //Pretend this sends an HTTP request to the url and gets back a reply.
        byte[] reply = new byte[] { };
        return reply;
    }
}

Alternative Designs

An alternative would be to try to split or filter the diagnostic events when receiving them based on other information, eg HttpContextAccessor. In order to filter events that only apply to a test, info would be have to placed into the HttpContext that can be used to identify the test.

Risks

No response

johncrim commented 1 year ago

Oops, looks like the "area-System.Net.Http" tag was mistakenly added based on my example. This should be label: "area-System.Diagnostics" or similar.

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

Issue Details

### Background and motivation An aspect of `DiagnosticListener` that has puzzled me is the fact that all `DiagnosticListener` instances are added to a global (`DiagnosticListener.AllListeners`), which makes any use or validation of diagnostics in tests difficult / or it requires that all tests be run serially. To clarify: If more than one test is run at a time, and some code in the test creates a `DiagnosticListener`, the events sent to that listener can come from any test, and the events received by a test can be triggered by an operation in any other test that is running. I'm sure this is a well-known limitation, but I would appreciate feedback on whether this is something that the dotnet team is interested in fixing; or if it's never going to be fixed and I should just work around it; or if I should prefer a newer API (eg OpenTelemetry) for exposing non-message (`ILogger`) diagnostics. I'm most interested in being able to scope listeners and events to a DI Container; so the listeners could be per test or per fixture. ### API Proposal The leading requirement is obviously backward compatibility; so by default `DiagnosticListener.AllListeners` should be a global and behave the same as before. A class like this would need to be added: ```cs public sealed class DiagnosticListenerCollection { public void Add(DiagnosticListener listener); // Other ICollection methods? } ``` And a constructor overload would need to be added to `DiagnosticListener` to store the `DiagnosticListener` in the `DiagnosticListenerCollection` instead of the global collection: ```cs public partial class DiagnosticListener { public DiagnosticListener(string name, DiagnosticListenerCollection listenerCollection) { Name = name; listenerCollection.Add(this); } ... ``` And then different libraries which provide one or more `DiagnosticListener`s would (ideally) add support for using a `DiagnosticListenerCollection`, if present, to put the listener into, like below. ### API Usage ```cs serviceCollection.AddSingleton(); serviceCollection.AddSingleton(); serviceCollection.AddTransient((HttpClientFactory factory) => factory.CreateHttpClient()); ... public class HttpClientFactory : IDisposable { private readonly DiagnosticSource _httpLogger; public HttpClientFactory(DiagnosticListenerCollection? diagnosticListeners) { // Handle scoped or global DiagnosticListenerCollection _httpLogger = diagnosticListeners != null ? new DiagnosticListener("System.Net.Http", diagnosticListeners) : new DiagnosticListener("System.Net.Http"); } public HttpClient CreateHttpClient() => new HttpClient(_httpLogger); public void Dispose() => _httpLogger.Dispose(); } public class HttpClient { private readonly DiagnosticSource _httpLogger; internal HttpClient(DiagnosticSource httpLogger) { _httpLogger; = httpLogger; } public byte[] SendWebRequest(string url) { if (_httpLogger.IsEnabled("RequestStart")) { _httpLogger.Write("RequestStart", new { Url = url }); } //Pretend this sends an HTTP request to the url and gets back a reply. byte[] reply = new byte[] { }; return reply; } } ``` ### Alternative Designs An alternative would be to try to split or filter the diagnostic events when receiving them based on other information, eg [HttpContextAccessor](https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.httpcontextaccessor?view=aspnetcore-6.0). In order to filter events that only apply to a test, info would be have to placed into the `HttpContext` that can be used to identify the test. ### Risks _No response_

Author:	johncrim
Assignees:	-
Labels:	`api-suggestion`, `area-System.Net.Http`
Milestone:	-

johncrim commented 1 year ago

@area-System.Diagnostics @tommcdon

tommcdon commented 1 year ago

@noahfalk

noahfalk commented 1 year ago

Hi @johncrim, thanks for a nice write up of the issue!

I'm sure this is a well-known limitation, but I would appreciate feedback on whether this is something that the dotnet team is interested in fixing; or if it's never going to be fixed and I should just work around it; or if I should prefer a newer API (eg OpenTelemetry) for exposing non-message (ILogger) diagnostics.

Yeah this one is well-known and there is a similar global listener pattern at play for EventListener, ActivityListener, TraceListener, and MeterListener. Among .NET APIs ILogger is the odd one out in that regard :) Not sure if you find this history interesting, but beneath the covers this difference stems from ILogger being designed by the ASP.NET team which used to be in a separate part of Microsoft and they had different design principles in mind relative to many other .NET APIs. However lets dive into your questions!

is something that the dotnet team is interested in fixing

There are probably two different levels of "fixing" to distinguish:

DiagnosticListener exposes APIs that allows .NET developers who create their own instances of it to put those instances in a scoped container.
.NET in-box components that create their own instance of DiagnosticListener would add a container to their constructor parameters and use that container when it was provided. Your example of changing HttpClientFactory and HttpClient would fall in this category.

I think the short answer is that nothing seems inherently problematic with either of these solutions, but so far I haven't seen much demand for it from .NET developers. If we expected this would impact a lot of developers the priority would rise, but my understanding for now is that DiagnosticListener is somewhat obscure among our users and the impact would be low. Its possible the priority would rise as we learned more in the future, but I wouldn't feel right suggesting that you wait for a .NET in-box solution at this point.

or if it's never going to be fixed and I should just work around it

For now I'd guess even if does get updated in the future, it probably isn't certain enough that you would want to wait for it.

if I should prefer a newer API

All of the diagnostic APIs new or old that I can think of (other than ILogger) tend to follow this global listener list pattern. If your data isn't suitable for ILogger than I don't expect you will dodge the issue regardless of what other API you pick. That said, knock on wood, this probably isn't too hard to work around. If you needed a solution for case 1 where you are in control of creating the Listener something like this might do it:

class TaggedDiagnosticListener : DiagnosticListener
{
    object _tag;
    public TaggedDiagnosticListener(string name, object tag) : base(name) { _tag = tag; }

    public IObservable<DiagnosticListener> GetTaggedListeners(object tag) => new TaggedDiagnosticListenerObservable(tag);

    class TaggedDiagnosticListenerObservable : IObservable<DiagnosticListener>
    {
        object _tag;
        public TaggedDiagnosticListenerObservable(object tag) { _tag = tag; }
        public IDisposable Subscribe(IObserver<DiagnosticListener> observer)
        {
            return DiagnosticListener.AllListeners.Subscribe(new FilteredObserver(observer, _tag));
        }
    }

    class FilteredObserver : IObserver<DiagnosticListener>
    {
        IObserver<DiagnosticListener> _observer;
        object _tag;
        public FilteredObserver(IObserver<DiagnosticListener> inner, object tag) { _observer = inner; _tag = tag; }

        public void OnCompleted() => _observer.OnCompleted();
        public void OnError(Exception error) => _observer.OnError(error);
        public void OnNext(DiagnosticListener value)
        {
            if(value is TaggedDiagnosticListener tagged && tagged._tag == _tag)
            {
                _observer.OnNext(value);
            }
        }
    }
}

For the API above you could treat that tag object like a collection though it could be anything as long as it is unique to your DI container and easily accessible. I didn't test that code above in any way so apologies in advance if it has some bugs.

For case 2 where you want different components in .NET to become DI or multi-tenant aware, that is probably more a case by case basis. The most broadly applicable solution I am aware of for that is to create an AsyncLocal and assign it some scope object prior to calling in to whatever .NET code, then check the value of that AsyncLocal in any callbacks. This allows you to flow a scope through the call and filter on it in the callback.

One last bit of context that maybe is useful - we have gotten some requests similar to this for MeterListener and EventListener. Those types I expect have more usage so the priority to address it is a bit higher and we are likely to look into further for .NET 8. If we wound up solving those cases that might create a precedent for how we add DI-awareness to these diagnostic APIs that aren't currently DI-aware, and then if it was simple enough maybe we would apply that same technique to all the other APIs such as DiagosticListener. I don't want to make any promises here, but just mentioning it so you aren't surprised if it does show up later.

Hope this all helps!

ghost commented 1 year ago

Tagging subscribers to this area: @tommcdon See info in area-owners.md if you want to be subscribed.

Issue Details

### Background and motivation An aspect of `DiagnosticListener` that has puzzled me is the fact that all `DiagnosticListener` instances are added to a global (`DiagnosticListener.AllListeners`), which makes any use or validation of diagnostics in tests difficult / or it requires that all tests be run serially. To clarify: If more than one test is run at a time, and some code in the test creates a `DiagnosticListener`, the events sent to that listener can come from any test, and the events received by a test can be triggered by an operation in any other test that is running. I'm sure this is a well-known limitation, but I would appreciate feedback on whether this is something that the dotnet team is interested in fixing; or if it's never going to be fixed and I should just work around it; or if I should prefer a newer API (eg OpenTelemetry) for exposing non-message (`ILogger`) diagnostics. I'm most interested in being able to scope listeners and events to a DI Container; so the listeners could be per test or per fixture. ### API Proposal The leading requirement is obviously backward compatibility; so by default `DiagnosticListener.AllListeners` should be a global and behave the same as before. A class like this would need to be added: ```cs public sealed class DiagnosticListenerCollection { public void Add(DiagnosticListener listener); // Other ICollection methods? } ``` And a constructor overload would need to be added to `DiagnosticListener` to store the `DiagnosticListener` in the `DiagnosticListenerCollection` instead of the global collection: ```cs public partial class DiagnosticListener { public DiagnosticListener(string name, DiagnosticListenerCollection? listenerCollection) { Name = name; if (listenerCollection != null) { listenerCollection.Add(this); } else { // Store listener in global list, same as current impl } } ... ``` And then different libraries which provide one or more `DiagnosticListener`s would (ideally) add support for using a `DiagnosticListenerCollection`, if present, to put the listener into, like below. ### API Usage ```cs serviceCollection.AddSingleton(); serviceCollection.AddSingleton(); serviceCollection.AddTransient((HttpClientFactory factory) => factory.CreateHttpClient()); ... public class HttpClientFactory : IDisposable { private readonly DiagnosticSource _httpLogger; public HttpClientFactory(DiagnosticListenerCollection? diagnosticListeners) { // Handle scoped or global DiagnosticListenerCollection _httpLogger = new DiagnosticListener("System.Net.Http", diagnosticListeners); } public HttpClient CreateHttpClient() => new HttpClient(_httpLogger); public void Dispose() => _httpLogger.Dispose(); } public class HttpClient { private readonly DiagnosticSource _httpLogger; internal HttpClient(DiagnosticSource httpLogger) { _httpLogger; = httpLogger; } public byte[] SendWebRequest(string url) { if (_httpLogger.IsEnabled("RequestStart")) { _httpLogger.Write("RequestStart", new { Url = url }); } //Pretend this sends an HTTP request to the url and gets back a reply. byte[] reply = new byte[] { }; return reply; } } ``` ### Alternative Designs An alternative would be to try to split or filter the diagnostic events when receiving them based on other information, eg [HttpContextAccessor](https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.httpcontextaccessor?view=aspnetcore-6.0). In order to filter events that only apply to a test, info would be have to placed into the `HttpContext` that can be used to identify the test. ### Risks _No response_

Author:	johncrim
Assignees:	-
Labels:	`api-suggestion`, `area-System.Diagnostics`, `untriaged`
Milestone:	-

MihaZupan commented 1 year ago

If you are looking for this sort of functionality specifically when working with HttpClient, I would recommend you take a look at https://learn.microsoft.com/dotnet/fundamentals/networking/networking-telemetry. The majority of telemetry in the networking stack is done via EventSource, and that is the main approach for it that we will be improving in the future.

johncrim commented 1 year ago

@noahfalk - thank you for the excellent reply! I have reported many issues on various Microsoft and .NET projects, and your reply above is the clearest and most useful I have received. The TaggedDiagnosticListener is a great suggestion, thank you for writing it out.

I assume it's useful to keep this issue open to collect votes and similar, but your response was both complete and useful - my immediate questions are addressed.

I haven't studied up on how the .NET runtime and/or App Insights is changing things to be OpenTelemetry compliant, but if a new diagnostic collection API is added I would think this type of isolation would be desirable (especially since App Domains are no longer a thing).

johncrim commented 1 year ago

I wasn't asking specifically about HttpClient (that was just an obvious example), but thank you @MihaZupan - that is useful to know. It looks like EventSource has the same limitation as DiagnosticListener, in that all sources are added to a process global. It might be possible to use the same approach (Noah's TaggedDiagnosticListener) for both, for diagnostics on home-grown libraries.

I dug in a little bit just now and it looks like HttpClient sends different but potentially overlapping data to both EventSource and DiagnosticSource. The app insights library is using the DiagnosticListener API for most of the HttpClient info that it collects. Part of the reason is that app insights needs to add HTTP headers for outgoing requests, and that's only possible via DiagnosticListener.

dotnet / runtime