Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
vitek-karas opened this issue Jun 9, 2023 · 0 comments
Labels
enhancement New feature or request
Milestone

Comments

@vitek-karas
Copy link

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 dotnet/aspnetcore#11730.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants