-
Notifications
You must be signed in to change notification settings - Fork 861
Description
I used OpenTelemetry data from Npgsql to debug a performance issue yesterday and was very happy with it. However, it seems that the SQL from NpgsqlBatch
es is not collected. Looking at the current code in main, it seems that the reason is that the CommandText
property of the internal NpgsqlCommand
of an NpgsqlBatch
instance is null
:
internal NpgsqlCommand(int batchCommandCapacity, NpgsqlConnection? connection = null)
{
GC.SuppressFinalize(this);
InternalBatchCommands = new List<NpgsqlBatchCommand>(batchCommandCapacity);
InternalConnection = connection;
CommandType = CommandType.Text;
IsWrappedByBatch = true;
// These can/should never be used in this mode
_commandText = null!;
_parameters = null!;
}
It is the CommandText
property that is used in the TraceCommandStart
call:
public override string CommandText
{
get => _commandText;
set
{
Debug.Assert(!IsWrappedByBatch);
...
}
}
internal void TraceCommandStart(NpgsqlConnector connector)
{
Debug.Assert(CurrentActivity is null);
if (NpgsqlActivitySource.IsEnabled)
CurrentActivity = NpgsqlActivitySource.CommandStart(connector, CommandText, CommandType);
}
I find this strange, since the comment in the constructor said that the _commandText
field should never be used, and yet it's returned directly from the property.
I was wondering if there was a reason for not sending SQL from NpgsqlBatch
to OpenTelemetry, other than it's not implemented yet? Couldn't a simple solution be to change the CommandText
property getter for NpgsqlCommand
s that are IsWrappedByBatch
to return the CommandText
s of the contained NpgsqlBatchCommand
s concatenated? Or, if that's dangerous or has other side effects, to do such concatenation only when emitting telemetry? Would you accept a PR for that?