grpc-dotnet icon indicating copy to clipboard operation
grpc-dotnet copied to clipboard

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

Open vitek-karas opened this issue 1 year ago • 0 comments

Is your feature request related to a problem? Please describe.

The events produced by GRPC to the diagnostic listener pass a payload object which is consumed by several different tools/libraries. The events use declared but internal type, 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 GRPC 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 GRPC code and careful reflection usage in the consuming library. The consuming library has to use suppressions which is always fragile.

Describe the solution you'd like

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 library 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.

Describe alternatives you've considered

There are no good alternatives, every other solution will probably need to rely on reflection and will run into the same problems as described above.

Additional context

The diagnostic events produced by this library already use declared, but internal types GrpcCall.ActivitStartData and GrpcCall.ActivityStopData. The change would be making these public (and possibly moving them to a different namespace, unnest from the parent class).

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

Similar request for System.Net.Http and ASP.NET.

ASP.NET already migrated some of their events to public types for very similar reasons in https://github.com/dotnet/aspnetcore/pull/11730.

Related to https://github.com/open-telemetry/opentelemetry-dotnet/issues/3429 and https://github.com/dotnet/aspnetcore/issues/45910.

vitek-karas avatar Jun 09 '23 09:06 vitek-karas