Strong typed diagnostics

[Wraith2]

fixes #2184

Currently when emitting diagnostic events we use anonymous types. The only way to access the information in those types is using reflection which makes them unusable in reflection-free situations such as trimmed or Native AOT applications.

This PR replaces the anonymous types with dedicated types for each event. The new types are put into a new Microsoft.Data.SqlClient.Diagnostics namespace because no users are ever like to want to deal with those types directly and their names would be confusing it they commonly appears in intellisense listings. Documentation is added for all public members. Ref assembly is updated. Internal references to the names of the events have been updated (please check these carefully).
The names of the events have moved from internal constants to public constants because I feel that the names provide part of the public contract for accessing these diagnostics. If there is pushback on this particular part of the PR I could fairly easily be persuaded to revert them. I'm not sure of it.

The changes I've made are based on the changes contributed to asp.net by @benaadams in dotnet/aspnetcore#11730 . They provide strongly typed properties and optimized access through IReadOnlyList<KeyValuePair<string,object>> so callers do not need to take a direct dependency on our library they can simply try the cast to that interface and cleanly access all the data. I considered also adding IReadOnlyDictionary<string,object> allowing direct access to known named properties without a direct dependency. I can do that if it will be used by callers but wanted to have a discussion about it first.

/cc @kant2002 for trimming, @mbakalov for appinsights/otel oversight

[Wraith2]

/me pokes @mbakalov

[kant2002]

Honestly I did not see anything related to trimming here. Are you have specific place in mind? Otherwise changes looks good. plain code very friendly to trimming 😄

[Wraith2]

If you check the original issue you'll see that this is a suggested method of removing the anonymous types which require the use of reflection. So it doesn't make this library a lot safer but it allows consumer binaries to be safer consuming this one and so propagates trimability up the dependency stack.

[mbakalov]

Confirming we are good with this for otel - thank you! Checking AppInsights next...

[Wraith2]

Any appinsights feedback? not expecting it to fail if otel worked but it'd be good to have confirmation.

[JRahnama]

/azurepipeline run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JRahnama]

failures are not related to your changes. I have fixed the pipelines, if you can push something that should pass.

[codecov]

Codecov Report

Attention: Patch coverage is 30.16591% with 463 lines in your changes missing coverage. Please review.

Project coverage is 72.40%. Comparing base (f7ab115) to head (1424ba7).

❗ Current head 1424ba7 differs from pull request most recent head 8f7f3d5

Please upload reports for the commit 8f7f3d5 to get more accurate results.

Files	Patch %	Lines
...rc/Microsoft/Data/SqlClient/SqlClientDiagnostic.cs	20.12%	381 Missing ⚠️
...SqlClient/SqlClientDiagnosticListenerExtensions.cs	54.44%	82 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2226      +/-   ##
==========================================
- Coverage   72.81%   72.40%   -0.41%     
==========================================
  Files         311      312       +1     
  Lines       61694    62183     +489     
==========================================
+ Hits        44922    45025     +103     
- Misses      16772    17158     +386     

Flag	Coverage Δ
addons	92.88% <ø> (ø)
netcore	76.48% <30.16%> (-0.70%)	⬇️
netfx	70.43% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mbakalov]

@Wraith2 -- sorry for the delay, just did some checking and turns out the change does break AppInsights! 😮

Error is:

AI (Internal): [Microsoft-ApplicationInsights-Extensibility-DependencyCollector] SqlClientDiagnosticSourceListener OnNext failed to call event handler. Error details 'System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.ApplicationInsights.DependencyCollector.Implementation.SqlClientDiagnostics.SqlClientDiagnosticSourceListener.BeforeOpenConnectionHelper(KeyValuePair`2 evnt, PropertyFetcher operationIdFetcher, PropertyFetcher connectionFetcher, PropertyFetcher operationFetcher, PropertyFetcher timestampFetcher, PropertyFetcher dataSourceFetcher, PropertyFetcher databaseFetcher)
   at Microsoft.ApplicationInsights.DependencyCollector.Implementation.SqlClientDiagnostics.SqlClientDiagnosticSourceListener.System.IObserver<System.Collections.Generic.KeyValuePair<System.String,System.Object>>.OnNext(KeyValuePair`2 evnt)'

Reason is the way AppInsights instrumentation reflects on the payload properties does not support inheritance. E.g. this code tries to get OperationId property:

private void BeforeOpenConnectionHelper(KeyValuePair<string, object> evnt,
    PropertyFetcher operationIdFetcher,
    PropertyFetcher connectionFetcher,
    PropertyFetcher operationFetcher,
    PropertyFetcher timestampFetcher,
    PropertyFetcher dataSourceFetcher,
    PropertyFetcher databaseFetcher)
{
    var operationId = (Guid)operationIdFetcher.Fetch(evnt.Value);

    DependencyCollectorEventSource.Log.SqlClientDiagnosticSubscriberCallbackCalled(operationId, evnt.Key);

    var connection = connectionFetcher.Fetch(evnt.Value);

    // ...

The PropertyFetcher does this:

if (fetch == null || fetch.Type != objType)
{
    this.innerFetcher = fetch = PropertyFetch.FetcherForProperty(objType, objType?.GetTypeInfo()?.GetDeclaredProperty(this.propertyName));
}

And GetDeclaredProperty does not see the OperationId defined on the SqlClientDiagnostic base class.

The reason Otel works is because it is lucky. The Otel instrumentation has the same issue, but it just happens to only access Command and Exception properties.

[Wraith2]

Interesting. I think that the "right" way to fix this is to fix app insights but I don't think that's realistic. It's something worth raising with appinsights and otel because in future if people aren't using anonymous objects then inheritance is something that could be used to avoid a lot of code duplication.

I'll need to rewrite without the base class and put in some notes that inheritance cannot be used.

[mbakalov]

Thanks! I agree on the right way. Will check with AppInsights and Otel if it is something that can be changed in the future.

[Wraith2]

Base class removed and everything implemented directly on the new classes. Once the CI finishes can you retest on appinsights please.

[mbakalov]

@Wraith2: confirm it works ok now for both Otel and AppInsights, thanks!

[Wraith2]

@David-Engel any particular blocker on this?

[David-Engel]

any particular blocker on this?

No, sorry. Just time and priorities. If you address the inheritDoc issue, we'll try to get this merged for 6.0 preview 1.

[JRahnama]

@Wraith2 and of course a conflict had to happen here right on this PR when we are ready to merge.

[Wraith2]

rebasing now. give me a minute to resolve things and check it.

[Wraith2]

Done, we're in the hands of the CI now.