Changes for #8007: RelationalDataReader instance ID

[NickCraver]

Changes for the proposal discussed in #8007

This tracks the already-existing instanceId for diagnostics from the command to the reader, so one can associate the data reader disposal (which happens upon the completion of results reading, not when the command does) with the place it opened. This is needed for consistency in correlation of relational diagnostics.

Note: I was able to test this easily against a slightly out of date dev, but on latest rebase I'm getting all sorts of issues with the InMemory (and other) packages wanting 2.0.0.0. I don't see how this would be related at all, so not spending the time to fight the current tooling...I bet the CI handles this just fine.

[NickCraver]

^ looks like a dynamic in netcore failure in the test itself, perhaps due to https://github.com/dotnet/corefx/issues/4672 ...which is likely why RelationalDiagnosticSourceAfterMessage and RelationalDiagnosticSourceBeforeMessage exist (but have conflicting comments on them).

Should I make this another strong type in the diagnostics write just for the sake of testing or...? It's basically a 1:1 of netcore test support and strongly typed, at the moment.

@karelz how are other libraries handling this? It's kind of a fundamental problem with DiagnosticSource, anonymous types (often used all over the Microsoft libs), and consumption.

[karelz]

Sorry, not sure if I fully follow ...

Is the problem that anonymous types cannot be properly tested?
The DiagnosticSource.Write line is basically a contract. Tests should behave the same way consumption will -- if you want to keep the payload as anonymous class, you have to "magically know" the contract on the other side (consumption or test). If you want strong-names, you gotta create type.

Or is the key problem with the types being internal?

@vancem @brianrob can you please chime in?

[NickCraver]

@karelz I think the problem here is that the anonymous types exist at all. In my opinion, as a consumer, I'd like all of these to have a proper type and that type be exposed. The same problem that presents itself in unit tests is the same one I have elsewhere (MiniProfiler, in my case).

You're doing pattern matching, comment notes...it's a mess and there's no compile-time assurances in play.

Perhaps I should open another issue to strongly type all writes and expose all of these types? As I alluded to on Twitter: this isn't just an Entity Framework problem. For example, ASP.NET MVC does the same thing with anonymous types everywhere.

It's very inconsistent and not consumer friendly. Even the event names for the diagnostics are internal, why not expose these? Unless you go digging through source, you're consuming the output of a black box that (AFAIK) isn't documented either. To be blunt: it currently feels as though DiagnosticSource was put in for ApplicationInsights and nothing more. It's far from a proper API surface, IMO.

[karelz]

@vancem will have much more insights into the plans.
BTW: The feature was NOT designed only for AppInsights. @vancem is working on tracing for years and this was a natural next step from EventSource. Aligned with enabling multiple profilers.

Anonymous types were added for ease of use AFAIK. Also, you have to consider the implications of exposing the types publicly -- suddenly you have API contract, which is harder to change. I believe one key value of the "lose" contract via anonymous types is that there are only a handful of consumers around the world and they have to adapt to every latest version. DiagnosticSource is basically exposing internal things. If you change implementation, you might need to change the DiagnosticSource and you expect that consumers have to adjust to the change (potentially multiplex based on version). It's a lot of work, but I am not aware of any easier alternative.
I view DiagnosticSource as better way how to expose internals without profilers partying via RefEmit on the code and injecting their own hooks based on "known code patterns" (which does not work with multiple profilers attached). While not perfect, DiagnosticSource feels like less fragile than previously used RefEmit hijacking pattern.

[NickCraver]

@karelz I disagree on the fragility. If a typo is corrected, everyone breaks at runtime. The current method is reflection based building dynamic methods via .SubscribeWithAdpater(). All of the downsides with an API are present today with the anonymous types, they're just hidden and cause a lot of work for the consumer who can still break at runtime if they do everything right.

We're still hooking into known code patterns. Case in point: I had to dig through all of the diagnostic writing code in GitHub to figure out what the hell the source is writing here. And so does everyone else. It's an undocumented API we're asking people to use, and one that will break and has few tests in any project due to the very nature of consumption being so difficult.

I'll open a CoreFX issue on it.

[karelz]

I see your point - I would like @vacem to chime on it. Good idea to move the discussion to CoreFX.

One last thought: If you use strong-names, I think that rather than breaking types, you should create new types (MyPayload2). Breaking types means that you cannot multiplex based on version (which is IMO important scenario here).
Some guidance for DiagnosicSource.Write users wouldn't hurt I guess (assuming @vancem doesn't shoot down my understanding as incorrect :)).

[NickCraver]

@karelz @vancem I've kicked up a discussion here on the overall architecture of DiagnosticSource: https://github.com/dotnet/corefx/issues/18114 Maybe I'm way off base, but I'd love to be corrected or see if we can make progress either way and improve things for everyone. Thanks for the Saturday back and forth here :)

[ajcvickers]

Closing since I believe this is now fixed in the update diagnostics code--see comment on #8007