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

[vitek-karas]

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

The events produced by SqlClient to the diagnostic listener pass a payload object which is consumed by several different tools/libraries. The events use mostly anonymous types, 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 properties may be trimmed away because there's no attempt in SqlClient to hint the trimmer to preserve them

Some other libraries use similar approach. For example, GRPC uses declared but internal types and makes an effort to preserve the necessary properties. But even that approach as its problems. 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. For more details on the similar GRPC problem see: grpc/grpc-dotnet#2154

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. For example this code gets the Command property from the payload instance, using reflection to get to it. The returned instance is still typed as System.Object, so it has to use additional reflection to get access to its properties and so on. If the library (SqlClient) 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

Note that this would not remove the need for trimmer annotations in the SqlClient 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. SqlClient should investigate what (if any) typical usages of its events are through diagnostic event source and what properties are accessed in that case. Then it should use trimmer annotations to preserve those properties, so that even trimmed applications will work with diagnostic event source. Note that this approach is never 100%, it's a tradeoff between the size of the trimmed app and the exposed functionality via diagnostic event source - diagnostic event source is inherently not trim compatible.

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

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.

[roji]

FWIW EF Core also has publicly-exposed types as its diagnostics payloads (codehttps://github.com/dotnet/efcore/tree/release/8.0/src/EFCore.Relational/Diagnostics)).

[Wraith2]

Can you give an example in the sqlclient code of what needs to be changed? I had a look around for anonymous types but couldn't see anything.

[vitek-karas]

For example here:

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlClientDiagnosticListenerExtensions.cs

Lines 47 to 57 in e719601

	@this.Write(
	SqlBeforeExecuteCommand,
	new
	{
	OperationId = operationId,
	Operation = operation,
	ConnectionId = sqlCommand.Connection?.ClientConnectionId,
	Command = sqlCommand,
	transaction?.InternalTransaction?.TransactionId,
	Timestamp = Stopwatch.GetTimestamp()
	});

This writes an instance of an anonymous as the payload.