Skip to content

Commit 031a36a

Browse files
BrarBrar
authored
Set a valid activity span name in NpgsqlActivitySource.CommandStart (#4765)
Fixes #4757
1 parent d0b91b1 commit 031a36a

4 files changed

Lines changed: 119 additions & 45 deletions

File tree

‎src/Npgsql/Internal/NpgsqlConnector.cs‎

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,17 @@ public sealed partial class NpgsqlConnector : IDisposable
9696
/// </summary>
9797
internal int BackendProcessId { get; private set; }
9898

99+
string? _inferredUserName;
100+
101+
/// <summary>
102+
/// The user name that has been inferred when the connector was opened
103+
/// </summary>
104+
internal string InferredUserName
105+
{
106+
get => _inferredUserName ?? throw new InvalidOperationException($"{nameof(InferredUserName)} cannot be accessed before the connector has been opened.");
107+
private set => _inferredUserName = value;
108+
}
109+
99110
bool SupportsPostgresCancellation => BackendProcessId != 0;
100111

101112
/// <summary>
@@ -682,29 +693,47 @@ void WriteStartupMessage(string username)
682693
WriteStartup(startupParams);
683694
}
684695

685-
async ValueTask<string> GetUsernameAsync(bool async, CancellationToken cancellationToken)
696+
ValueTask<string> GetUsernameAsync(bool async, CancellationToken cancellationToken)
686697
{
687698
var username = Settings.Username;
688699
if (username?.Length > 0)
689-
return username;
700+
{
701+
InferredUserName = username;
702+
return new(username);
703+
}
690704

691705
username = PostgresEnvironment.User;
692706
if (username?.Length > 0)
693-
return username;
707+
{
708+
InferredUserName = username;
709+
return new(username);
710+
}
711+
712+
return GetUsernameAsyncInternal();
694713

695-
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
714+
async ValueTask<string> GetUsernameAsyncInternal()
696715
{
697-
username = await KerberosUsernameProvider.GetUsernameAsync(Settings.IncludeRealm, ConnectionLogger, async, cancellationToken);
716+
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
717+
{
718+
username = await KerberosUsernameProvider.GetUsernameAsync(Settings.IncludeRealm, ConnectionLogger, async,
719+
cancellationToken);
720+
721+
if (username?.Length > 0)
722+
{
723+
InferredUserName = username;
724+
return username;
725+
}
726+
}
698727

728+
username = Environment.UserName;
699729
if (username?.Length > 0)
730+
{
731+
InferredUserName = username;
700732
return username;
701-
}
702-
703-
username = Environment.UserName;
704-
if (username?.Length > 0)
705-
return username;
733+
}
706734

707-
throw new NpgsqlException("No username could be found, please specify one explicitly");
735+
throw new NpgsqlException("No username could be found, please specify one explicitly");
736+
}
708737
}
709738

710739
async Task RawOpen(SslMode sslMode, NpgsqlTimeout timeout, bool async, CancellationToken cancellationToken, bool isFirstAttempt = true)

‎src/Npgsql/KerberosUsernameProvider.cs‎

Lines changed: 33 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -18,66 +18,71 @@ sealed class KerberosUsernameProvider
1818
static string? _principalWithRealm;
1919
static string? _principalWithoutRealm;
2020

21-
#pragma warning disable CS1998
22-
internal static async ValueTask<string?> GetUsernameAsync(bool includeRealm, ILogger connectionLogger, bool async, CancellationToken cancellationToken)
23-
#pragma warning restore CS1998
21+
internal static ValueTask<string?> GetUsernameAsync(bool includeRealm, ILogger connectionLogger, bool async, CancellationToken cancellationToken)
2422
{
2523
if (_performedDetection)
26-
return includeRealm ? _principalWithRealm : _principalWithoutRealm;
24+
return new(includeRealm ? _principalWithRealm : _principalWithoutRealm);
2725
var klistPath = FindInPath("klist");
2826
if (klistPath == null)
2927
{
3028
connectionLogger.LogDebug("klist not found in PATH, skipping Kerberos username detection");
31-
return null;
29+
return new((string?)null);
3230
}
33-
3431
var processStartInfo = new ProcessStartInfo
3532
{
3633
FileName = klistPath,
3734
RedirectStandardOutput = true,
3835
RedirectStandardError = true,
3936
UseShellExecute = false
4037
};
38+
4139
var process = Process.Start(processStartInfo);
4240
if (process is null)
4341
{
4442
connectionLogger.LogDebug("klist process could not be started");
45-
return null;
43+
return new((string?)null);
4644
}
4745

46+
return GetUsernameAsyncInternal();
47+
48+
#pragma warning disable CS1998
49+
async ValueTask<string?> GetUsernameAsyncInternal()
50+
#pragma warning restore CS1998
51+
{
4852
#if NET5_0_OR_GREATER
49-
if (async)
50-
await process.WaitForExitAsync(cancellationToken);
51-
else
52-
// ReSharper disable once MethodHasAsyncOverloadWithCancellation
53-
process.WaitForExit();
53+
if (async)
54+
await process.WaitForExitAsync(cancellationToken);
55+
else
56+
// ReSharper disable once MethodHasAsyncOverloadWithCancellation
57+
process.WaitForExit();
5458
#else
5559
// ReSharper disable once MethodHasAsyncOverload
5660
process.WaitForExit();
5761
#endif
5862

59-
if (process.ExitCode != 0)
60-
{
61-
connectionLogger.LogDebug($"klist exited with code {process.ExitCode}: {process.StandardError.ReadToEnd()}");
62-
return null;
63-
}
63+
if (process.ExitCode != 0)
64+
{
65+
connectionLogger.LogDebug($"klist exited with code {process.ExitCode}: {process.StandardError.ReadToEnd()}");
66+
return null;
67+
}
6468

65-
var line = default(string);
66-
for (var i = 0; i < 2; i++)
67-
// ReSharper disable once MethodHasAsyncOverload
69+
var line = default(string);
70+
for (var i = 0; i < 2; i++)
71+
// ReSharper disable once MethodHasAsyncOverload
6872
#if NET7_0_OR_GREATER
69-
if ((line = async ? await process.StandardOutput.ReadLineAsync(cancellationToken) : process.StandardOutput.ReadLine()) == null)
73+
if ((line = async ? await process.StandardOutput.ReadLineAsync(cancellationToken) : process.StandardOutput.ReadLine()) == null)
7074
#elif NET5_0_OR_GREATER
71-
if ((line = async ? await process.StandardOutput.ReadLineAsync() : process.StandardOutput.ReadLine()) == null)
75+
if ((line = async ? await process.StandardOutput.ReadLineAsync() : process.StandardOutput.ReadLine()) == null)
7276
#else
73-
if ((line = process.StandardOutput.ReadLine()) == null)
77+
if ((line = process.StandardOutput.ReadLine()) == null)
7478
#endif
75-
{
76-
connectionLogger.LogDebug("Unexpected output from klist, aborting Kerberos username detection");
77-
return null;
78-
}
79+
{
80+
connectionLogger.LogDebug("Unexpected output from klist, aborting Kerberos username detection");
81+
return null;
82+
}
7983

80-
return ParseKListOutput(line!, includeRealm, connectionLogger);
84+
return ParseKListOutput(line!, includeRealm, connectionLogger);
85+
}
8186
}
8287

8388
static string? ParseKListOutput(string line, bool includeRealm, ILogger connectionLogger)

‎src/Npgsql/NpgsqlActivitySource.cs‎

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using Npgsql.Internal;
22
using System;
3+
using System.Data;
34
using System.Diagnostics;
45
using System.Net;
56
using System.Net.Sockets;
@@ -20,19 +21,58 @@ static NpgsqlActivitySource()
2021

2122
internal static bool IsEnabled => Source.HasListeners();
2223

23-
internal static Activity? CommandStart(NpgsqlConnector connector, string sql)
24+
internal static Activity? CommandStart(NpgsqlConnector connector, string commandText, CommandType commandType)
2425
{
2526
var settings = connector.Settings;
26-
var activity = Source.StartActivity(settings.Database!, ActivityKind.Client);
27+
28+
var dbName = settings.Database ?? connector.InferredUserName;
29+
string? dbOperation = null;
30+
string? dbSqlTable = null;
31+
string activityName;
32+
switch (commandType)
33+
{
34+
case CommandType.StoredProcedure:
35+
dbOperation = NpgsqlCommand.EnableStoredProcedureCompatMode ? "SELECT" : "CALL";
36+
// In this case our activity name follows the concept of the CommandType.TableDirect case
37+
// ("<db.operation> <db.name>.<db.sql.table>") but replaces db.sql.table with the procedure name
38+
// which seems to match the spec's intent without being explicitly specified that way (it suggests
39+
// using the procedure name but doesn't mention using db.operation or db.name in that case).
40+
activityName = $"{dbOperation} {dbName}.{commandText}";
41+
break;
42+
case CommandType.TableDirect:
43+
dbOperation = "SELECT";
44+
// The OpenTelemetry spec actually asks to include the database name into db.sql.table
45+
// but then again mixes the concept of database and schema.
46+
// As I interpret it, it actually wants db.sql.table to include the schema name and not the
47+
// database name if the concept of schemas exists in the database system.
48+
// This also makes sense in the context of the activity name which otherwise would include the
49+
// database name twice.
50+
dbSqlTable = commandText;
51+
activityName = $"{dbOperation} {dbName}.{dbSqlTable}";
52+
break;
53+
case CommandType.Text:
54+
activityName = dbName;
55+
break;
56+
default:
57+
throw new ArgumentOutOfRangeException(nameof(commandType), commandType, null);
58+
}
59+
60+
var activity = Source.StartActivity(activityName, ActivityKind.Client);
2761
if (activity is not { IsAllDataRequested: true })
2862
return activity;
2963

3064
activity.SetTag("db.system", "postgresql");
3165
activity.SetTag("db.connection_string", connector.UserFacingConnectionString);
32-
activity.SetTag("db.user", settings.Username);
33-
activity.SetTag("db.name", settings.Database);
34-
activity.SetTag("db.statement", sql);
66+
activity.SetTag("db.user", connector.InferredUserName);
67+
// We trace the actual (maybe inferred) database name we're connected to, even if it
68+
// wasn't specified in the connection string
69+
activity.SetTag("db.name", dbName);
70+
activity.SetTag("db.statement", commandText);
3571
activity.SetTag("db.connection_id", connector.Id);
72+
if (dbOperation != null)
73+
activity.SetTag("db.operation", dbOperation);
74+
if (dbSqlTable != null)
75+
activity.SetTag("db.sql.table", dbSqlTable);
3676

3777
var endPoint = connector.ConnectedEndPoint;
3878
Debug.Assert(endPoint is not null);

‎src/Npgsql/NpgsqlCommand.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1609,7 +1609,7 @@ internal void TraceCommandStart(NpgsqlConnector connector)
16091609
{
16101610
Debug.Assert(CurrentActivity is null);
16111611
if (NpgsqlActivitySource.IsEnabled)
1612-
CurrentActivity = NpgsqlActivitySource.CommandStart(connector, CommandText);
1612+
CurrentActivity = NpgsqlActivitySource.CommandStart(connector, CommandText, CommandType);
16131613
}
16141614

16151615
internal void TraceReceivedFirstResponse()

0 commit comments

Comments
 (0)