Feature request: add instanceId (Guid) to RelationalDataReader for DiagnosticSource

[NickCraver]

Related issues: #8001 and #7939

In RelationalDiagnostics there's an instanceId (a Guid) added to each event type so that things like WriteCommandBefore and WriteCommandAfter can be tracked together in the linear logging format that is DiagnosticSource.

For reference, the DiagnosticsListener bits are here: RelationalDiagnostics.cs and an example usage here: RelationalConnection#298-329. The gist of it is:

var instanceId = Guid.NewGuid();
DiagnosticSource.WriteConnectionOpening(_connection.Value,
    ConnectionId,
    instanceId,
    startTimestamp,
    async: false);

try
{
    _connection.Value.Open();
    wasOpened = true;

    var currentTimestamp = Stopwatch.GetTimestamp();
    DiagnosticSource.WriteConnectionOpened(_connection.Value, 
        ConnectionId,
        instanceId,
        startTimestamp, 
        currentTimestamp,
        async: false);
}
catch (Exception e)
{
    var currentTimestamp = Stopwatch.GetTimestamp();
    DiagnosticSource.WriteConnectionError(_connection.Value, 
        ConnectionId, 
        e,
        instanceId,
        startTimestamp,
        currentTimestamp,
        async: false);
    throw;
}

That instanceId allows a DiagnosticSource listener to tie these two events together. In my case of MiniProfiler, I can time these things and log them as a profiling step. However, RelationalDataReader is missing this. Where it's created in RelationalCommand via the Execute() call, there's no ID given, so when DataReaderDisposing is fired, there's no ID to go off of:

_diagnosticSource.WriteDataReaderDisposing(
    _connection.DbConnection,
    _connection.ConnectionId,
    _reader,
    _reader.RecordsAffected,
    _startTimestamp,
    currentTimestamp);

Ultimately, this results in a BeforeExecuteCommand , AfterExecuteCommand, and CommandExecutionError events that you can't correlate to the DataReaderDisposing event. So it's both inconsistent and without a way to resolve what's open vs. complete. A connection can have multiple readers (e.g. MARS connections), so the connectionId isn't useful, ordered, or unique. But the command instanceId already allocated is unique and 1:1.

Proposed Solution

1. Add a Guid InstanceId property to RelationalDataReader
2. Assign InstanceId from the one already present when creating it (here)
   • via new constructor? (non-breaking)
   • via internal property setter? (also non-breaking)
   • add to existing constructor (breaking)
3. Add instanceId to the WriteDataReaderDisposing call (internal, so non-breaking)
4. Add instanceId to the anonymous object inside the WriteDataReaderDisposing call for consumers

If this is a welcomed change, please advise on the options for Item 2 about backwards compatibility preferences and I'm happy to submit a PR.

[ajcvickers]

@NickCraver We would love to see a PR for this. With regard to item 2, please add it to the existing constructor. This is the cleanest and we don't think that any code outside of ours will be calling this constructor anyway. If they are, then 2.0 is a breaking release anyway and the change to their code should be minimal.

[ajcvickers]

Assigning to @anpete to handle the PR when it arrives.

[NickCraver]

@ajcvickers roger - really busy at the moment but 2.0 has some time so I'll try and get to this in the next week or so. Thanks for taking a look!

[NickCraver]

@anpete I've thrown up a PR in #8110, let me know what else you need!