Consider changing diagnostic events to use public declared types as their payload

[vitek-karas]

The events produced by ASP.NET to the diagnostic listener pass a payload object which is consumed by several different tools/libraries. For some of the events the payload object uses a public defined type and thus the consumer can directly cast it and access all the relevant data. But for some events the event uses either declared but internal type or just anonymous type, in these cases the consumer has to use reflection to access the relevant data.

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 ASP.NET 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 ASP.NET 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 hardcodes the name of the properties and then cast the value of those properties to public types (like Exception). 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.

Some events were already changed like this, for example #11730.

What remains based on OpenTelemetry usage:

• Event Microsoft.ASpNetCore.Hosting.UnhandledException uses a private type UnhandledExceptionData.
• Event Microsoft.AspNetCore.Diagnostics.UnhandledException uses anonymous type.

Currently we mitigate these problems by maintaining annotations on the type used as well as the logging method. These preserve the properties we know of are accessed dynamically (through reflection) by some of the consumers. This list is inherently incomplete and fragile. It also means that the consumer will get trim/AOT warnings due to reflection usage and it has to suppress these relying on annotations in the framework. This relationship is unverifiable by any tooling and thus fragile.

Note that there are other events which seem to have the same problem, they're just not used by OpenTelemetry which is the consumer I've been looking at:

• Microsoft.AspNetCore.Hosting.BeginRequest - uses private type
• Microsoft.AspNetCore.Hosting.EndRequest - uses private type
• Microsoft.AspNetCore.Hosting.HttpRequestIn.Start - uses public type (HttpContext), but still maintain a long list of hints to the trimmer - maybe this would be worth revisiting in the consumers (for example OpenTelemetry doesn't use reflection in this case at all).
• Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop - uses public type (HttpContext), but still maintain a long list of hints to the trimmer - maybe this would be worth revisiting in the consumers (for example OpenTelemetry doesn't use reflection in this case at all).
• Microsoft.AspNetCore.Diagnostics.HandledException - uses anonymous type
• Microsoft.AspNetCore.NodeServices.Npm.NpmStarted - uses anonymous type
• Microsoft.AspNetCore.Server.Kestrel.BadRequest - uses internal type HttpProtocol

Additionally, in some cases the naming of the properties is inconsistent. For example the public types use typical Pascal casing of property names, e.g. Request or Exception. But most of the anonymous types use lower case names, like exception. This requires the consumers to perform case insensitive search for the property making it less performant (they typically loop over all properties to achieve this).

Related to #45910 and open-telemetry/opentelemetry-dotnet#3429.

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

[eerhardt]

Microsoft.AspNetCore.Hosting.HttpRequestIn.Start - uses public type (HttpContext), but still maintain a long list of hints to the trimmer - maybe this would be worth revisiting in the consumers (for example OpenTelemetry doesn't use reflection in this case at all).

The "long list of hints to the trimmer" isn't for in-process listeners, but instead of out-of-proc listeners like dontet-monitor.
dotnet-monitor uses the DiagnosticSourceEventSource connector to log these properties. You can see the list in the comment in the code - https://github.com/dotnet/diagnostics/blob/7cc6fbef613cdfe5ff64393120d59d7a15e98bd6/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/HttpRequestSourceConfiguration.cs#L20-L33.

See also - dotnet/runtime#87064.

[amcasey]

@mitchdenny This sounds like a mechanical change for a meaningful quality-of-life improvement, but I'm not sure where it falls on our AOT priority list. Can you please take a look?

[mitchdenny]

@JamesNK had you had any thoughts about this for the gRPC side of things? I think that we are going to end up with a bunch of public loose types hanging around that we might want to put into a consistent namespace (or namespaces). For example: Microsoft.AspNetCore.Hosting.Diagnostics ... and any diagnostic event types could get put in there.

So there would be a *.Diagnostics namespace anywhere we had to do this. Based this issue we'll probably end up with 3-4 types in there just for the hosting namespace. The other alternative is to just have a single namespace that we share across all the assemblies that we ship to make it easy for folks to find the types.

Thoughts?