Skip to content

Support for enriching activities #4284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Haydabase wants to merge 9 commits into from
Closed

Support for enriching activities #4284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e33930c
Support for enriching activities
Haydabase Jan 19, 2022
172f1db
Address minor PR feedback
Haydabase Jan 20, 2022
cb58126
Remove dedicated test project
Haydabase Jan 20, 2022
fe93d1d
Merge branch 'main' into otel-enrich
Haydabase Jan 20, 2022
dd77ff0
Rename Enrich -> EnrichCommandExecution
Haydabase Jan 20, 2022
21a616a
Pass command + exception to enrich for OnException event
Haydabase Mar 7, 2022
48badfe
Only enrich if IsAllDataRequested
Haydabase Apr 19, 2022
3f8b652
Merge branch 'main' into otel-enrich
Haydabase Sep 20, 2022
13f75f5
Merge branch 'main' into otel-enrich
Haydabase Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file modified .devcontainer/db/init-db.sh
100644 → 100755
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
<PackageVersion Include="GitHubActionsTestLogger" Version="2.0.1" />
<PackageVersion Include="AdoNet.Specification.Tests" Version="2.0.0-alpha8" />
<PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="6.0.1" />
<PackageVersion Include="OpenTelemetry" Version="1.3.1" />

<!-- Benchmarks -->
<PackageVersion Include="BenchmarkDotNet" Version="0.13.2" />
Expand Down
6 changes: 5 additions & 1 deletion src/Npgsql.OpenTelemetry/Npgsql.OpenTelemetry.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
</ItemGroup>

<ItemGroup>
<None Include="README.md" Pack="true" PackagePath="\"/>
<None Include="README.md" Pack="true" PackagePath="\" />
</ItemGroup>

<ItemGroup>
<Compile Remove="..\Shared\CodeAnalysis.cs" />
Comment thread
roji marked this conversation as resolved.
</ItemGroup>
</Project>
16 changes: 16 additions & 0 deletions src/Npgsql.OpenTelemetry/NpgsqlTracingInstrumentation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
using System;

namespace Npgsql.OpenTelemetry;

sealed class NpgsqlTracingInstrumentation : IDisposable
{
readonly NpgsqlTracingOptions _originalOptions;

public NpgsqlTracingInstrumentation(NpgsqlTracingOptions options)
{
_originalOptions = NpgsqlActivitySource.Options;
NpgsqlActivitySource.Options = options;
}

public void Dispose() => NpgsqlActivitySource.Options = _originalOptions;
}
11 changes: 9 additions & 2 deletions src/Npgsql.OpenTelemetry/TracerProviderBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using Npgsql.OpenTelemetry;
using OpenTelemetry.Trace;

// ReSharper disable once CheckNamespace
Expand All @@ -14,6 +15,12 @@ public static class TracerProviderBuilderExtensions
/// </summary>
public static TracerProviderBuilder AddNpgsql(
this TracerProviderBuilder builder,
Action<NpgsqlTracingOptions>? options = null)
=> builder.AddSource("Npgsql");
Action<NpgsqlTracingOptions>? configure = null)
{
var options = new NpgsqlTracingOptions();
configure?.Invoke(options);
return builder
.AddSource("Npgsql")
.AddInstrumentation(() => new NpgsqlTracingInstrumentation(options));

@roji roji Jan 20, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the OpenTelemetry very well - what's the advantage of AddInstrumentation with NpgsqlTracingInstrumentation here over simply injecting NpgsqlActivitySource.Options directly in this method?

@Haydabase Haydabase Jan 20, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only want to set NpgsqlActivitySource.Options if and when Build() is called on the TraceProviderBuilder. I also think technically we want to remove those options once the built TracerProvider has been disposed. Using an instrumentation factory that returns an IDisposable gives us that behaviour.
(See 3.i here)

@roji roji Jan 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that documentation section only relevant when the instrumentation library is separate/external, because the main library isn't instrumented? In other words, I think what we're doing qualifies as part of "make every library observable out of the box by having them call OpenTelemetry API directly" (the first sentence).

On a related note, I don't think this config stuff qualifies as "state management", which is what I think the AddInstrumentation API is about.

But I'm far from an expert on all this - let me know how you see things.

@tl-david-hayden tl-david-hayden Mar 7, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think regardless of if the documentation section is intended to be for just separate instrumentations, the behaviour it gives us is desirable here: only set NpgsqlActivitySource.Options if and when Build() is called on the TraceProviderBuilder, and unset it when the TraceProvider is disposed.

}
}
24 changes: 20 additions & 4 deletions src/Npgsql/NpgsqlActivitySource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ static NpgsqlActivitySource()

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

internal static Activity? CommandStart(NpgsqlConnector connector, string sql)
internal static NpgsqlTracingOptions Options { get; set; } = new();

internal static Activity? CommandStart(NpgsqlConnector connector, NpgsqlCommand command)
{
var settings = connector.Settings;
var activity = Source.StartActivity(settings.Database!, ActivityKind.Client);
Expand All @@ -31,7 +33,7 @@ static NpgsqlActivitySource()
activity.SetTag("db.connection_string", connector.UserFacingConnectionString);
activity.SetTag("db.user", settings.Username);
activity.SetTag("db.name", settings.Database);
activity.SetTag("db.statement", sql);
activity.SetTag("db.statement", command.CommandText);
activity.SetTag("db.connection_id", connector.Id);

var endPoint = connector.ConnectedEndPoint;
Expand All @@ -55,6 +57,8 @@ static NpgsqlActivitySource()
throw new ArgumentOutOfRangeException("Invalid endpoint type: " + endPoint.GetType());
}

Options.EnrichCommandExecution?.Invoke(activity, "OnStartActivity", command);

return activity;
}

Expand All @@ -64,13 +68,19 @@ internal static void ReceivedFirstResponse(Activity activity)
activity.AddEvent(activityEvent);
}

internal static void CommandStop(Activity activity)
internal static void CommandStop(Activity activity, NpgsqlCommand command)
{
activity.SetTag("otel.status_code", "OK");
activity.SetEndTime(DateTime.UtcNow);
if (activity.IsAllDataRequested)
{
Options.EnrichCommandExecution?.Invoke(activity, "OnStopActivity", command);
}

activity.Dispose();
}

internal static void SetException(Activity activity, Exception ex, bool escaped = true)
internal static void SetException(Activity activity, NpgsqlCommand command, Exception ex, bool escaped = true)
{
var tags = new ActivityTagsCollection
{
Expand All @@ -83,6 +93,12 @@ internal static void SetException(Activity activity, Exception ex, bool escaped
activity.AddEvent(activityEvent);
activity.SetTag("otel.status_code", "ERROR");
activity.SetTag("otel.status_description", ex is PostgresException pgEx ? pgEx.SqlState : ex.Message);
activity.SetEndTime(DateTime.UtcNow);
if (activity.IsAllDataRequested)
{
Options.EnrichCommandExecution?.Invoke(activity, "OnException", (command, ex));
}

activity.Dispose();
}
}
6 changes: 3 additions & 3 deletions src/Npgsql/NpgsqlCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1609,7 +1609,7 @@ internal void TraceCommandStart(NpgsqlConnector connector)
{
Debug.Assert(CurrentActivity is null);
if (NpgsqlActivitySource.IsEnabled)
CurrentActivity = NpgsqlActivitySource.CommandStart(connector, CommandText);
CurrentActivity = NpgsqlActivitySource.CommandStart(connector, this);
}

internal void TraceReceivedFirstResponse()
Expand All @@ -1624,7 +1624,7 @@ internal void TraceCommandStop()
{
if (CurrentActivity is not null)
{
NpgsqlActivitySource.CommandStop(CurrentActivity);
NpgsqlActivitySource.CommandStop(CurrentActivity, this);
CurrentActivity = null;
}
}
Expand All @@ -1633,7 +1633,7 @@ internal void TraceSetException(Exception e)
{
if (CurrentActivity is not null)
{
NpgsqlActivitySource.SetException(CurrentActivity, e);
NpgsqlActivitySource.SetException(CurrentActivity, this, e);
CurrentActivity = null;
}
}
Expand Down
11 changes: 10 additions & 1 deletion src/Npgsql/NpgsqlTracingOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
using System;
using System.Diagnostics;

namespace Npgsql;

/// <summary>
/// Options to configure Npgsql's support for OpenTelemetry tracing.
/// Currently no options are available.
/// </summary>
public class NpgsqlTracingOptions
{
/// <summary>
/// Gets or sets an action to enrich a Command Execution Activity.
/// </summary>
/// <remarks>
/// <see href="https://www.npgsql.org/doc/diagnostics/tracing.html"/>
/// </remarks>
Comment thread
Haydabase marked this conversation as resolved.
public Action<Activity, string, object>? EnrichCommandExecution { get; set; }
}
7 changes: 7 additions & 0 deletions src/Npgsql/Properties/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,10 @@
"8078a5df97a62d83c9a2db2d072523a8fc491398254c6b89329b8c1dcef43a1e" +
"7aa16153bcea2ae9a471145624826f60d7c8e71cd025b554a0177bd935a78096" +
"29f0a7afc778ebb4ad033e1bf512c1a9c6ceea26b077bc46cac93800435e77ee")]

[assembly: InternalsVisibleTo("Npgsql.OpenTelemetry, PublicKey=" +
"0024000004800000940000000602000000240000525341310004000001000100" +
"2b3c590b2a4e3d347e6878dc0ff4d21eb056a50420250c6617044330701d35c9" +
"8078a5df97a62d83c9a2db2d072523a8fc491398254c6b89329b8c1dcef43a1e" +
"7aa16153bcea2ae9a471145624826f60d7c8e71cd025b554a0177bd935a78096" +
"29f0a7afc778ebb4ad033e1bf512c1a9c6ceea26b077bc46cac93800435e77ee")]
2 changes: 2 additions & 0 deletions src/Npgsql/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ Npgsql.NpgsqlDataSourceBuilder.UnmapComposite<T>(string? pgName = null, Npgsql.I
Npgsql.NpgsqlDataSourceBuilder.UnmapEnum<TEnum>(string? pgName = null, Npgsql.INpgsqlNameTranslator? nameTranslator = null) -> bool
Npgsql.NpgsqlDataSourceBuilder.UsePhysicalConnectionInitializer(System.Action<Npgsql.NpgsqlConnection!>? connectionInitializer, System.Func<Npgsql.NpgsqlConnection!, System.Threading.Tasks.Task!>? connectionInitializerAsync) -> Npgsql.NpgsqlDataSourceBuilder!
Npgsql.NpgsqlLoggingConfiguration
Npgsql.NpgsqlTracingOptions.EnrichCommandExecution.get -> System.Action<System.Diagnostics.Activity!, string!, object!>?
Npgsql.NpgsqlTracingOptions.EnrichCommandExecution.set -> void
Npgsql.Schema.NpgsqlDbColumn.IsIdentity.get -> bool?
Npgsql.Schema.NpgsqlDbColumn.IsIdentity.set -> void
Npgsql.StatementType.Call = 11 -> Npgsql.StatementType
Expand Down
2 changes: 2 additions & 0 deletions test/Npgsql.Tests/Npgsql.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
<PackageReference Include="Microsoft.CSharp" />
<PackageReference Include="Microsoft.NET.Test.Sdk" />
<PackageReference Include="NUnit3TestAdapter" />
<PackageReference Include="OpenTelemetry" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="../../src/Npgsql/Npgsql.csproj" />
<ProjectReference Include="../../src/Npgsql.OpenTelemetry/Npgsql.OpenTelemetry.csproj" />
</ItemGroup>
</Project>
98 changes: 98 additions & 0 deletions test/Npgsql.Tests/OpenTelemetry/NpgsqlTracingOptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using NUnit.Framework;
using OpenTelemetry;
using OpenTelemetry.Trace;

namespace Npgsql.Tests.OpenTelemetry;

[NonParallelizable]
public class NpgsqlTracingOptionsTests : TestBase
{
[Test]
public void CommandExecution_start_stop()
{
using (var conn = OpenConnection())
{
conn.ExecuteScalar("SELECT 1");
}

Assert.That(_enrichInvocations, Has.Count.EqualTo(2));

var (startActivity, startEventName, startObject) = _enrichInvocations[0];
Assert.That(startEventName, Is.EqualTo("OnStartActivity"));
Assert.That(startObject, Is.TypeOf<NpgsqlCommand>().With.Property("CommandText").EqualTo("SELECT 1"));
Assert.That(startActivity.Kind, Is.EqualTo(ActivityKind.Client));

var (stopActivity, stopEventName, stopObject) = _enrichInvocations[1];
Assert.That(stopEventName, Is.EqualTo("OnStopActivity"));
Assert.That(stopObject, Is.SameAs(startObject));
Assert.That(stopActivity, Is.SameAs(startActivity));
}

[Test]
public void CommandExecution_start_exception()
{
var exception = Assert.Throws<PostgresException>(() =>
{
using var conn = OpenConnection();
conn.ExecuteScalar("BO SELECTA");
});

Assert.That(_enrichInvocations, Has.Count.EqualTo(2));

var (startActivity, startEventName, startObject) = _enrichInvocations[0];
Assert.That(startEventName, Is.EqualTo("OnStartActivity"));
Assert.That(startObject, Is.TypeOf<NpgsqlCommand>().With.Property("CommandText").EqualTo("BO SELECTA"));
Assert.That(startActivity.Kind, Is.EqualTo(ActivityKind.Client));

var (stopActivity, stopEventName, stopObject) = _enrichInvocations[1];
Assert.That(stopEventName, Is.EqualTo("OnException"));
Assert.That(stopObject, Is.TypeOf<ValueTuple<NpgsqlCommand, Exception>>());
var (stopCommand, stopException) = (ValueTuple<NpgsqlCommand, Exception>)stopObject;
Assert.That(stopCommand.CommandText, Is.EqualTo("BO SELECTA"));
Assert.That(stopException, Is.SameAs(exception));
Assert.That(stopActivity, Is.SameAs(startActivity));
}

[Test]
public void CommandExecution_start_exception_patternmatch()
{
var exception = Assert.Throws<PostgresException>(() =>
{
using var conn = OpenConnection();
conn.ExecuteScalar("BO SELECTA");
});

Assert.That(_enrichInvocations, Has.Count.EqualTo(2));
var (_, stopEventName, stopObject) = _enrichInvocations[1];

switch (stopEventName, stopObject)
{
case ("OnException", (NpgsqlCommand stopCommand, Exception stopException)):
Assert.That(stopCommand.CommandText, Is.EqualTo("BO SELECTA"));
Assert.That(stopException, Is.SameAs(exception));
break;
default:
Assert.Fail($"{nameof(stopEventName)}: '{stopEventName}', {nameof(stopObject)}.GetType(): '{stopObject.GetType()}'");
break;
}
}

[SetUp]
public void SetUp()
{
_enrichInvocations.Clear();
_tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddNpgsql(o => o.EnrichCommandExecution = (activity, eventName, rawObject) => _enrichInvocations.Add((activity, eventName, rawObject)))
.Build();
}

[TearDown]
public void TearDown() => _tracerProvider.Dispose();

TracerProvider _tracerProvider = null!;

readonly List<(Activity activity, string eventName, object rawObject)> _enrichInvocations = new();
}