[API Proposal]: make payload types in System.Net.Http.DiagnosticsHandler to be public

[xakep139]

Background and motivation

Currently all folks who want to listen to HttpClient events with DiagnosticListener, need to use reflection to get request/response data from a payload. Here's an example of a class used to get these properties in OpenTelemetry repo: PropertyFetcher
This applies to both .NET Core/.NET 5+ and .NET Framework (HttpHandlerDiagnosticListener and System.Net.Http.Desktop listeners respectively).

API Proposal

To improve both user experience and performance, these types can be made public:

• DiagnosticsHandler.ExceptionData
• DiagnosticsHandler.RequestData
• DiagnosticsHandler.ResponseData

For .NET Framework, anonymous types used for payload can be also done as regular public types:

• in HttpHandlerDiagnosticListener.RaiseResponseEvent()
• in HttpHandlerDiagnosticListener.RaiseRequestEvent()

API Usage

var observer = new HttpClientDiagnosticListener();
using var listenerSubscription = DiagnosticListener.AllListeners.Subscribe(observer);

// ...

class HttpClientDiagnosticListener : IObserver<DiagnosticListener>, IObserver<KeyValuePair<string, object?>>, IDisposable
{
    private IDisposable? _listenerSubscription;

    public void Dispose() => _listenerSubscription?.Dispose();

    public void OnCompleted() => /* ... */;

    public void OnError(Exception error) => /* ... */;

    public void OnNext(DiagnosticListener listener)
    {
        if (listener.Name == "HttpHandlerDiagnosticListener")
        {
            _listenerSubscription?.Dispose();
            _listenerSubscription = listener.Subscribe(this);
        }
    }

    public void OnNext(KeyValuePair<string, object?> @event)
    {
        // NB: No reflection here, we just cast payload:
        switch (@event.Key)
        {
            case "System.Net.Http.Request":
                if (@event.Value is RequestData request)
                {
                    // ...
                }
                break;

            case "System.Net.Http.Response":
                if (@event.Value is ResponseData response)
                {
                    // ...
                }
                break;

            case "System.Net.Http.Exception":
                if (@event.Value is RequestData request)
                {
                    // ...
                }
                break;
        }
    }
}

Alternative Designs

No response

Risks

No response

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

Issue Details

---

Background and motivation

Currently all folks who want to listen to HttpClient events with DiagnosticListener, need to use reflection to get request/response data from a payload. Here's an example of a class used to get these properties in OpenTelemetry repo: PropertyFetcher
This applies to both .NET Core/.NET 5+ and .NET Framework (HttpHandlerDiagnosticListener and System.Net.Http.Desktop listeners respectively).

API Proposal

To improve both user experience and performance, these types can be made public:

• DiagnosticsHandler.ExceptionData
• DiagnosticsHandler.RequestData
• DiagnosticsHandler.ResponseData

For .NET Framework, anonymous types used for payload can be also done as regular public types:

• in HttpHandlerDiagnosticListener.RaiseResponseEvent()
• in HttpHandlerDiagnosticListener.RaiseRequestEvent()

API Usage

var observer = new HttpClientDiagnosticListener();
using var listenerSubscription = DiagnosticListener.AllListeners.Subscribe(observer);

// ...

class HttpClientDiagnosticListener : IObserver<DiagnosticListener>, IObserver<KeyValuePair<string, object?>>, IDisposable
{
    private IDisposable? _listenerSubscription;

    public void Dispose() => _listenerSubscription?.Dispose();

    public void OnCompleted() => /* ... */;

    public void OnError(Exception error) => /* ... */;

    public void OnNext(DiagnosticListener listener)
    {
        if (listener.Name == "HttpHandlerDiagnosticListener")
        {
            _listenerSubscription?.Dispose();
            _listenerSubscription = listener.Subscribe(this);
        }
    }

    public void OnNext(KeyValuePair<string, object?> @event)
    {
        // NB: No reflection here, we just cast payload:
        switch (@event.Key)
        {
            case "System.Net.Http.Request":
                if (@event.Value is RequestData request)
                {
                    // ...
                }
                break;

            case "System.Net.Http.Response":
                if (@event.Value is ResponseData response)
                {
                    // ...
                }
                break;

            case "System.Net.Http.Exception":
                if (@event.Value is RequestData request)
                {
                    // ...
                }
                break;
        }
    }
}

Alternative Designs

No response

Risks

No response

Author:	xakep139
Assignees:	-
Labels:	api-suggestion, area-System.Net.Http, untriaged
Milestone:	-

[dpk83]

@tarekgh @noahfalk

[tarekgh]

CC @karelz @ericstj @davidfowl

Currently System.Net.Http library is shipped as inbox only and not released anymore as OOB in NuGet. Changing anything for .NET Framework will be challenging.

[tarekgh]

Looks the part need to be exposed for .NET Framework is in the System.Diagnostics.DiagnosticSource and not in System.Net.Http. So, supporting the .NET Framework part will not be difficult.

Now, we need to look if the request is reasonable or should we have it.

[tarekgh]

CC @reyang @CodeBlanch

[noahfalk]

I think exposing a strong type in .NET Core is reasonable as long as the owners of HttpClient believe they can offer the same level of back compatibility for that data as we would offer for other APIs.

For desktop we made a choice in the past not to do new feature work in the HttpHandlerDiagnosticsListener both to focus effort on .NET Core and to reduce the chance of accidentally breaking compatibility. When OpenTelemetry last asked for new features in that code we suggested they fork it and add any features they wanted that way: https://github.com/open-telemetry/opentelemetry-dotnet/blob/ab3e40790dde1d4cbc990e1871d831820430781d/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs#L33. Any other project that needs specific new desktop .NET functionality for that type could do something similar if desired.

[ManickaP]

Triage: This looks useful for instrumentation like OpenTelemetry to avoid unnecessary reflection. It is nice to have and not critical for 8.0, planning for future.

As for Framework, we will not expose any new APIs there.

[eerhardt]

Triage: This looks useful for instrumentation like OpenTelemetry to avoid unnecessary reflection. It is nice to have and not critical for 8.0, planning for future.

We have a goal of getting OpenTelemetry AOT-compatible in .NET 8. See the "Stage 2" section of EPIC: Support publishing ASP.NET Core API apps with Native AOT. And open-telemetry/opentelemetry-dotnet#3429.

@vitek-karas - given your work in trying to make OpenTelemetry AOT compatible, do you see this as a "nice to have"? Or is doing something here to eliminate Reflection critical to get OpenTelemetry AOT-compatible?

[vitek-karas]

See #87008 for another request on this.

I don't think this is essential for making OpenTelemetry AOT-compatible, because the current annotations in the HTTP will preserve the necessary properties. That said, the system is VERY fragile and unverifiable by tooling. It forces warning suppression in the consumer library. See #87008 for more details on the reasoning.

I also think a big benefit of doing this would be that we codify the fact that it's a public contract which we should not break.