What's the intended use pattern of RelationalDiagnosticSource* messages?

[NickCraver]

I'm trying to hook MiniProfiler up to Entity Framework Core, but I'm not really understanding what the intended interface is. My understanding is that my best (supported) route is DiagnosticSource, called via RelationalDiagnostics. That's the only path I see to get my hands on the DbCommand, DbConnection, etc. needed for various scenarios.

But the .Write() call for DiagnosticSource is:

public abstract void Write(string name, object value);

The way RelationalDiagnostics uses this passes in a typed object (good), example here:

diagnosticSource.Write(
    BeforeExecuteCommand,
    new RelationalDiagnosticSourceBeforeMessage
    {
        ConnectionId = connectionId,
        Command = command,
        ExecuteMethod = executeMethod,
        InstanceId = instanceId,
        Timestamp = startTimestamp,
        IsAsync = async
    });

...but those types passed are internal, for example here's RelationalDiagnosticSourceBeforeMessage:

namespace Microsoft.EntityFrameworkCore.Internal
{
    // TODO revert to anonymous types when https://github.com/dotnet/corefx/issues/4672 is fixed
    internal class RelationalDiagnosticSourceBeforeMessage
    {
        public Guid ConnectionId { get; set; }
        public DbCommand Command { get; set; }
        public string ExecuteMethod { get; set; }
        public bool IsAsync { get; set; }
        public Guid InstanceId { get; set; }
        public long Timestamp { get; set; }
    }
}

...and some other calls are anonymous objects, for example: WriteConnectionOpening(). So the usage pattern is inconsistent.

How am I supposed to get at the members in an inexpensive way? I'm hoping it's hot intended that everyone use reflection here to get the members, and I can't cast because they're internal. The TODO scares me because that means we have to use the DLR in what should be a code path as cheap as possible. Note: the #4672 issue has since been closed.

Currently I see as far as creating a DiagnosticSource for MiniProfiler and hooking up to logging that way, but not how to use the object passed to the write methods, since I'm unable to access those types. I sincerely hope I'm being an idiot - is there a way to go about this that's not "reflect everything"?

[NickCraver]

As an example of why dynamic/DLR access is something I really, really want to stay away from here, I added a quick benchmark of the performance impact (this is .NET 4.6.2, re-runnable benchmark code here):

private static SomeType _type = new SomeType
{
    Foo = nameof(SomeType.Foo),
    Foo2 = nameof(SomeType.Foo2),
    Conn = new DbConn
    {
        Name = "Test",
        ConnectionString = "MyServer"
    }
};
private static object _passed = _type;
public string Casting()
{
    var t = ((SomeType)_passed);
    return t.Conn.ConnectionString;
}     
public string Dynamic()
{
    dynamic t = _passed;
    return t.Conn.ConnectionString;
}

Results:

BenchmarkDotNet=v0.10.3.0, OS=Microsoft Windows NT 6.2.9200.0
Processor=Intel(R) Core(TM) i7-6900K CPU 3.20GHz, ProcessorCount=16
Frequency=3123189 Hz, Resolution=320.1856 ns, Timer=TSC
  [Host]     : Clr 4.0.30319.42000, 64bit RyuJIT-v4.6.1590.0
  DefaultJob : Clr 4.0.30319.42000, 64bit RyuJIT-v4.6.1590.0

Method	Mean	StdDev	Allocated
Casting	0.6203 ns	0.0138 ns	0 B
Dynamic	23.6551 ns	0.7789 ns	0 B

[pakrym]

You can use https://github.com/aspnet/eventnotification and SubscribeWithAdapter extension method that generates proxy for you (https://github.com/aspnet/EventNotification/blob/dev/src/Microsoft.Extensions.DiagnosticAdapter/DiagnosticListenerExtensions.cs#L16)

[pakrym]

Sample usage in ApllicationInsights MVC diagnostic listener: https://github.com/Microsoft/ApplicationInsights-aspnetcore/blob/develop/src/Microsoft.ApplicationInsights.AspNetCore/DiagnosticListeners/Implementation/MvcDiagnosticsListener.cs

[divega]

Thanks @pakrym!

@NickCraver please let us know if the suggested approach works for MiniProfiler. Otherwise we can keep looking into it (and also discuss #774 if if really necessary).

[NickCraver]

@pakrym @divega Thanks a lot for the info and example, there's a lot there but everything seems sane. While I'm not as concerned with the perf given the proxy IL generation in play, I am concerned about the overall interface.

Is there documentation for the parameters available to the listener methods? While I can see from reading source the parameter names on the interface or anonymous type must be matched (unless I'm reading the proxy generation wrong - please correct me if so). How are users supposed to find these parameters or know this? By digging through source code?...or is there a doc somewhere?

I'd feel a lot more at ease that this interface wouldn't break if it was documented/supported somewhere - without that I'm a little uncomfortable. Hopefully there's a doc that Google just isn't finding for me? The only reference I see outside the code that the parameter names need to match was one unofficial blog post...and to find that I was searching with information I gleaned from code.

A concrete example of this would be I searched for ConnectionOpening in the repo (results here), and there appear to be no unit tests. So there's a chance of unwittingly breaking these brittle connection points, IMO - are these fears unfounded?

[ajcvickers]

Assigning to @divega since work involved here is currently to make sure we are doing the right thing.

[NickCraver]

Opened #8007 for another issue I hit actually trying to hook up here - happy to PR it if someone will take a peek there.

[divega]

@NickCraver, sorry for the late answer (I am catching up with issues assigned to me). I got this response from @vancem a while ago:

The short answer, is that there is missing documentation. However realistically DiagnosticSource/Listener will be used by a handful of hard-core ‘profiler/monitoring’ tools, so the number of people who will use this documentation is small. Also frankly in an open-source world, it is not clear that the docs add much value, since the source is there, and typically there are questions of EXACTLY what the meaning of various events and properties are, which means that people probably will be looking at the source anyway. Thus fixing this may not be the highest priority for a while. We should log issues. My expectation is that this can be solved with a Markdown file in the appropriate code base.
There should be tests for any DiagnosticSource events that have been created. This is a more serious omission, as was pointed out, it is reasonably likely for them to be broken unless there is some testing. We should log issues in the appropriate code base to get this fixed.
It is possible to build a tool that ‘discovers’ what DiagnosticSource events exist. It would not be perfect, as you will have not visibility into an event unless you have a scenario that takes a code path that is instrumented. Still given a scenario you can discover all the events that that scenario might generate (and their arguments), which would be pretty nice. This tool does not yet exist however. I was thinking of adding it to the PerfView tool, but right now it is just a thought…

His answer seems relevant to the discussion you started on https://github.com/dotnet/corefx/issues/18114.

Note for triage: Here what I believe the follow up action items should be on the EF Core side:

• Decide what the appropriate level of documentation is for these messages is, e.g. do we want to include them in some sort of documentation or are we ok with profiler tool writers to inspect them or look at the code?
• Make sure we our unit test coverage for the events is adequate.

[divega]

@NickCraver BTW @ajcvickers is working on a bunch of changes in this area for 2.0 which will likely include breaking changes. It would be great if we can devise a way to help you track this work closely, e.g. we can mention you in any PRs.

[ajcvickers]

@ajcvickers to look at what MVC is doing.

[vancem]

There are two basic solutions to reading values out of objects passed with DiagnosticSource.

1. Use reflection to fetch items.

Standard reflection of course works, as does using the 'dynamic' feature of C#.
You can do efficient reflection (where you end up caching the delegate that does the fetch). See

https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs#L784

For a class that does this. Bascially you could do

static fetchExecuteMethod = new PropertySpec("ExecuteMethod");

Then to fetch the ExecuteMethod field from 'obj' you can do

string s =fetchExecuteMethod.Fetch(obj) as string;

Which should be pretty fast.

However it won't beat casting to a particular type. If you have need for that the recommendation is that the DiagnosticSource should expose the payload type explicitly (under the a namespace that ends in DiagnosticSource)

We are thinking about exposing the PropertySpec as a public class. We just need to nail down details of exactly what its name is and some details about how it is used. If you are interested in this create an issue in the corefx repo and we can track it.

In the very short term, using dynamic is pretty good, you can convert to other things at will as the need arises.

[ajcvickers]

We did some investigation and thinking on this today and we have decided to create nominal types for all our payloads. This will mean:

• There is a place we can easily document what the types are
• Our normal processes for detecting breaking changes will just work without anything special being needed
• Consumers can use the payloads in a performant and type-safe manner without needing any proxies,

We talked to @rynowak about the anonymous types/proxies approach that MVC uses and he indicated that based on what they have learned using nominal types is probably better. As of yet they haven't had anybody really pushing them to change to nominal types, but they would at least consider doing so if there was such a push.

[vancem]

I think having explicit types is very reasonable.

My recommendation is that they call get put into a namespace that ends with 'DiagnosticSource' Thus

namespace System.Data.YYY.DiagnosticSource {
public class EventXXXData {
string PayloadItem1;
int PayloadItem2;
}
}