Consider making HTTP diagnostic event payload strongly typed with public types

[vitek-karas]

The events produced by the HTTP stack to the diagnostic listener pass a payload object which is consumed by several different tools/libraries. Currently the payload object has a well defined type, but the type is internal. The consumer of the event has to use reflection to query data from the payload.

This has distinct disadvantages in performance impact and trim/AOT compatibility. Currently for example Open Telemetry library uses several different tools to overcome these:

• It includes a relatively complicated reflection based "cache" of accessors to make the access to the properties as fast as possible
• It has to code defensively to make it work in cases the reflection fails
• The HTTP code uses DynamicDependencyAttribute and DynamicallyAccessedMembersAttribute attributes to hint trimmer/AOT compiler to keep properties on the payload object

There are still unsolved problems though. The trimmer compatibility is not verifiable by any tools, since it relies on the combination of attributes in the HTTP code and careful reflection usage in the consuming library. The consuming library has to use suppressions which is always fragile.

Another observation is that the shape of the payload object is effectively a public contract, since the consuming libraries hardcode the name of the properties and then cast the value of those properties to public types (like HttpRequestMessage). If the framework changed the name of the property, it would be an effective breaking change, even though technically it's not changing any public property.

It seems it would be beneficial to consider this a public contract/API in all meanings of that. Mainly declare the type of the payload object as a public type so that the consumer library can just cast the payload instance and access everything on it directly. No need for complicated and less performant reflection based solutions. The types in question are ActivityStartData, ActivityStopData, RequestData, ResponseData and ExceptionData all are nested in System.Net.Http.DiagnosticsHandler.

Note that this would not remove the need for annotations in the HTTP code because these payloads are read fully dynamically through diagnostic event source, but it would greatly improve all consumers which listen to the events in-proc.

ASP.NET already did something very similar for some of its events. For example, dotnet/aspnetcore#11730 changed the payload type to a public type along with several other improvements, like implementing a IReadOnlyList<KeyValuePair<string, object>> on it, to make it easier for the consuming library to enumerate all accessible properties without use of reflection and knowing the exact type.

/cc @Yun-Ting, @LakshanF, @eerhardt

[ghost]

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

Issue Details

---

The events produced by the HTTP stack to the diagnostic listener pass a payload object which is consumed by several different tools/libraries. Currently the payload object has a well defined type, but the type is internal. The consumer of the event has to use reflection to query data from the payload.

This has distinct disadvantages in performance impact and trim/AOT compatibility. Currently for example Open Telemetry library uses several different tools to overcome these:

• It includes a relatively complicated reflection based "cache" of accessors to make the access to the properties as fast as possible
• It has to code defensively to make it work in cases the reflection fails
• The HTTP code uses DynamicDependencyAttribute and DynamicallyAccessedMembersAttribute attributes to hint trimmer/AOT compiler to keep properties on the payload object

There are still unsolved problems though. The trimmer compatibility is not verifiable by any tools, since it relies on the combination of attributes in the HTTP code and careful reflection usage in the consuming library. The consuming library has to use suppressions which is always fragile.

Another observation is that the shape of the payload object is effectively a public contract, since the consuming libraries hardcode the name of the properties and then cast the value of those properties to public types (like HttpRequestMessage). If the framework changed the name of the property, it would be an effective breaking change, even though technically it's not changing any public property.

It seems it would be beneficial to consider this a public contract/API in all meanings of that. Mainly declare the type of the payload object as a public type so that the consumer library can just cast the payload instance and access everything on it directly. No need for complicated and less performant reflection based solutions. The types in question are ActivityStartData, ActivityStopData, RequestData, ResponseData and ExceptionData all are nested in System.Net.Http.DiagnosticsHandler.

Note that this would not remove the need for annotations in the HTTP code because these payloads are read fully dynamically through diagnostic event source, but it would greatly improve all consumers which listen to the events in-proc.

ASP.NET already did something very similar for some of its events. For example, dotnet/aspnetcore#11730 changed the payload type to a public type along with several other improvements, like implementing a IReadOnlyList<KeyValuePair<string, object>> on it, to make it easier for the consuming library to enumerate all accessible properties without use of reflection and knowing the exact type.

/cc @Yun-Ting, @LakshanF, @eerhardt

Author:	vitek-karas
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[ManickaP]

Isn't this basically dupe of #82910?

[vitek-karas]

Yes - it is a duplicate, maybe the details are slightly different but the core idea is the same.