SqlClient instrumentation doesn't work on netfx using Microsoft.Data.SqlClient 3.0.0

[mbakalov]

New major release of SqlClient has changed their EventSource from this:

[Event(BeginExecuteEventId, Keywords = Keywords.ExecutionTrace, Task = Tasks.ExecuteCommand, Opcode = EventOpcode.Start)]
internal void BeginExecute(int objectId, string dataSource, string database, string commandText)
{
	if (Log.IsExecutionTraceEnabled())
	{
		WriteEvent(BeginExecuteEventId, objectId, dataSource, database, commandText);
	}
}

To this (PR open-telemetry/opentelemetry-dotnet#897):

internal void TryBeginExecuteEvent(int objectId, Guid? connectionId, string commandText, [System.Runtime.CompilerServices.CallerMemberName] string memberName = "")
{
	if (Log.IsExecutionTraceEnabled())
	{
		BeginExecute(GetFormattedMessage(SqlCommand_ClassName, memberName, EventType.INFO,
			string.Format("Object Id {0}, Client Connection Id {1}, Command Text {2}", objectId, connectionId, commandText)));
	}
}

// ...

[Event(BeginExecuteEventId, Keywords = Keywords.ExecutionTrace, Task = Tasks.ExecuteCommand, Opcode = EventOpcode.Start)]
internal void BeginExecute(string message) =>
	WriteEvent(BeginExecuteEventId, message);

Our SqlEventSourceListener expects 4 items in the payload (ObjectId, DataSource, Database, CommandText) but now gets only "Message" and doesn't log anything.

It looks like we're unable to get the server/db anymore on netfx which is unfortunate. Opening this issue here to track resolution, but probably best to ask SqlClient team to make some changes in their EventSource if they can.

[mbakalov]

cc @cijothomas

[Wraith2]

What limitations are there on changes to the events that are emitted?

For example do you actually check that there are exactly 4 items or could we add more after the ones you're expecting? The ConnectionID that was added is very useful for debug tracing complex async issues and it would be good to be able to keep it without breaking upstream consumers. If we moved it to the end would it be out of your way or would it still break things?

[mbakalov]

@Wraith2, just checked and both OpenTelemetry and AppInsights expect simply >=4 items (see linked code). As long as the first 4 are what we expect, it should be safe to add any additional things you need to the end!

[Kielek]

Cloisng. I do not think that we will invest in this bug. It is well documented in https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/de38aecbabc195fbbb29a5078b0b66513638ac9c/src/OpenTelemetry.Instrumentation.SqlClient#sqlclient-instrumentation-for-opentelemetry