Add OpenTelemetry event tracing to connections #3102

SeanFarrow · 2024-03-31T12:04:13Z

This PR add event tracing to IConnectionLifetime.

@jeremydmiller I have a few questions:
How do we want to test the EventConnectionLifetime and MartenTracing classes?
When do we want to update the documentation?
Do we want to wait to merge this until all the other OTel stuff is done?

Thanks,
Sean.

jeremydmiller · 2024-04-02T01:13:59Z

Honestly, just smoke tests. We'll have to rig up Prometheus or Signoz or something in a test harness to really test it manually.

The decorator around the connection lifetime, does that even need all the if/then checks? Shouldn't it be, only apply the decorator if that option is selected in the first place? You want to absolutely minimize the runtime steps because everything is in the hot path there.

For event metrics, I assumed we'd use an IDocumentSessionListener anyway

SeanFarrow · 2024-04-02T08:00:46Z

Yep, I’ll fix that, I wrote the thing bottom up, so the additional checks are not needed. Also, the LINQ tests are currently failing on CI. I haven’t investigated this as yet, do you want me to raise an issue for this? Let me have a look at tests later in the week, we might be able to use some of the fakes Microsoft recently added. Thanks, Sean. From: Jeremy D. Miller ***@***.***> Sent: Tuesday, April 2, 2024 2:14 AM To: JasperFx/marten ***@***.***> Cc: Sean Farrow ***@***.***>; Author ***@***.***> Subject: Re: [JasperFx/marten] Add OpenTelemetry event tracing to connections (PR #3102) Honestly, just smoke tests. We'll have to rig up Prometheus or Signoz or something in a test harness to really test it manually. The decorator around the connection lifetime, does that even need all the if/then checks? Shouldn't it be, only apply the decorator if that option is selected in the first place? You want to absolutely minimize the runtime steps because everything is in the hot path there. For event metrics, I assumed we'd use an IDocumentSessionListener anyway — Reply to this email directly, view it on GitHub<#3102 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AALDK7WJJPBWDFPMB7VN3TTY3IA63AVCNFSM6AAAAABFQLQCQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZQHEYDCMRZGY>. You are receiving this because you authored the thread.Message ID: ***@***.***>

…e SessionOptions class.

…connection events.

…ss and supporting classes.

gfoidl · 2024-04-02T09:08:25Z

src/Marten/Internal/OpenTelemetry/MartenTracing.cs

+    public static string MartenTenantId = "MartenTentantId";
+    public static string MartenCorelationId = "MartenCorelationId";


Can these be const are static readonly?
When they should be setable, then make them a property.

gfoidl · 2024-04-02T09:09:04Z

src/Marten/Internal/OpenTelemetry/MartenTracing.cs

+        "Marten",
+        typeof(MartenTracing).Assembly.GetName().Version!.ToString());
+
+    public static Activity? StartConnectionActivity(Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags =null)


Nit

Suggested change

public static Activity? StartConnectionActivity(Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags =null)

public static Activity? StartConnectionActivity(Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags = null)

gfoidl · 2024-04-02T09:09:24Z

src/Marten/Internal/OpenTelemetry/MartenTracing.cs

+        return StartActivity("connection", parentActivity, tags);
+    }
+
+    public static Activity StartActivity(string spanName, Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags =null, ActivityKind activityKind =ActivityKind.Internal)


Suggested change

public static Activity StartActivity(string spanName, Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags =null, ActivityKind activityKind =ActivityKind.Internal)

public static Activity StartActivity(string spanName, Activity? parentActivity = null, IEnumerable<KeyValuePair<string, object?>>? tags = null, ActivityKind activityKind = ActivityKind.Internal)

gfoidl · 2024-04-02T09:13:27Z

src/Marten/Internal/OpenTelemetry/MartenTracing.cs

+
+    public static Activity StartActivity(string spanName, Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags =null, ActivityKind activityKind =ActivityKind.Internal)
+    {
+        return ActivitySource.CreateActivity(spanName, activityKind, parentActivity?.ParentId, tags);


Should this be

Suggested change

return ActivitySource.CreateActivity(spanName, activityKind, parentActivity?.ParentId, tags);

return ActivitySource.StartActivity(spanName, activityKind, parentActivity?.ParentId, tags);

?

Otherwise an explicit Start needs to be done on the activity.

Or this method should be called CreateActivity too, else it's very misleading.

gfoidl · 2024-04-02T09:16:55Z

src/Marten/Internal/Sessions/EventTracingConnectionLifetime.cs

+    private const string MartenCommandExecutionStarted = "MartenCommandExecutionStarted";
+    private const string MartenCommandExecutionFailed = "MartenCommandExecutionFailed";
+    private const string MartenBatchExecutionStarted = "MartenBatchExecutionStarted";
+    private const string MartenBatchExecutionFailed = "MartenBatchExecutionFailed";
+    private const string MartenBatchPagesExecutionStarted = "MartenBatchPagesExecutionStarted";
+    private const string MartenBatchPagesExecutionFailed = "MartenBatchPagesExecutionFailed";


According OTel semantics these should have different names which we should follow.

Suggested change

private const string MartenCommandExecutionStarted = "MartenCommandExecutionStarted";

private const string MartenCommandExecutionFailed = "MartenCommandExecutionFailed";

private const string MartenBatchExecutionStarted = "MartenBatchExecutionStarted";

private const string MartenBatchExecutionFailed = "MartenBatchExecutionFailed";

private const string MartenBatchPagesExecutionStarted = "MartenBatchPagesExecutionStarted";

private const string MartenBatchPagesExecutionFailed = "MartenBatchPagesExecutionFailed";

private const string MartenCommandExecutionStarted = "marten.command.execution.started";

private const string MartenCommandExecutionFailed = "marten.command.execution.failed";

private const string MartenBatchExecutionStarted = "marten.batch.execution.started";

private const string MartenBatchExecutionFailed = "marten.batch.execution.failed";

private const string MartenBatchPagesExecutionStarted = "marten.batch.pages.execution.started";

private const string MartenBatchPagesExecutionFailed = "marten.batch.pages.execution.failed";

gfoidl · 2024-04-02T09:19:19Z

src/Marten/Internal/Sessions/EventTracingConnectionLifetime.cs

+        }
+        catch (Exception e)
+        {
+            if (_openTelemetryOptions.TrackConnectionEvents)


The same. I'll stop to comment on these, but there are some more below.

gfoidl · 2024-04-02T09:20:07Z

src/Marten/Internal/Sessions/EventTracingConnectionLifetime.cs

+    {
+        if (_openTelemetryOptions.TrackConnectionEvents)
+        {
+            _databaseActivity.AddEvent(new ActivityEvent("Database command execution started"));


Event name should be a constant too, and have a name following OTel semantics (see first comment).

gfoidl · 2024-04-02T09:21:37Z

src/Marten/Services/OpenTelemetryOptions.cs

+    /// <summary>
+    /// Used to track OpenTelemetry events for a connection, for example when a command or data reader has been executed. This defaults to false.
+    /// </summary>
+    public bool TrackConnectionEvents { get; set; } = false;


Suggested change

public bool TrackConnectionEvents { get; set; } = false;

public bool TrackConnectionEvents { get; set; }

false is the default by C# language specs, so can be omitted.

gfoidl · 2024-04-02T09:21:54Z

src/Marten/Services/SessionOptions.cs

@@ -93,6 +101,23 @@ internal IConnectionLifetime Initialize(DocumentStore store, CommandRunnerMode m
            throw new DefaultTenantUsageDisabledException();
        }

+        var innerConnectionLifetime =GetInnerConnectionLifetime(store, mode);


Suggested change

var innerConnectionLifetime =GetInnerConnectionLifetime(store, mode);

var innerConnectionLifetime = GetInnerConnectionLifetime(store, mode);

gfoidl · 2024-04-02T09:25:39Z

src/Marten/Services/SessionOptions.cs

@@ -93,6 +101,23 @@ internal IConnectionLifetime Initialize(DocumentStore store, CommandRunnerMode m
            throw new DefaultTenantUsageDisabledException();
        }

+        var innerConnectionLifetime =GetInnerConnectionLifetime(store, mode);
+
+        var corelationId = Activity.Current is not null ? Activity.Current.RootId : Guid.NewGuid().ToString();


When there's no activity that is recored (Current = null), is it OK to just create a new id?
I don't know what's best here.

As far as I understand things, if the current activity is null, this means you are the first activity in the chain, so creating a new activity/id is the right thing to do.

if the current activity is null, this means you are the first activity in the chain

Being the first one in the chain means that there's no parent, but Current is set an Activity.

When there's no listeneres interested in an activity or no listeners are registered, then ActivitySource.StartActivity will return null, hence Activity.Current will be null too.

So when there's no activity I don't know if just creating a new (random) Id is correct.
Maybe in that case it should just be empty?

The Activity would be null if the app isn't actually emitting Otel with a configured exporter. Sorry, just what @gfoidl already said

@jeremydmiller
So should we set the current activity if it is null? or not start an activity at all?

It's common for the Activity to be null, so just expect it. If you look in the Wolverine code, you'll see that every call to Activity is Activity?.AddTag() or similar

Is this code needed at all? The activity will internally check for a current activity and grab the correlation ID if required.

jeremydmiller · 2024-04-02T10:57:07Z

@SeanFarrow One combination of the LInq tests will fail periodically on CI, but always succeed on a subsequent run. I've never been able to reproduce it locally.

…ould aggregate to null

…ct uses the Id of the original document. Closes JasperFxGH-3096

JasperFxGH-2995

Hawxy · 2024-04-03T01:59:49Z

src/Marten/Internal/Sessions/EventTracingConnectionLifetime.cs

+
+    public Task<int> ExecuteAsync(NpgsqlCommand command, CancellationToken token = new CancellationToken())
+    {
+        if (_openTelemetryOptions.TrackConnectionEvents)


Why is this checked here if it's already checked when initializing this object?

Hawxy · 2024-04-03T02:24:05Z

src/Marten/Services/SessionOptions.cs

+        var innerConnectionLifetime =GetInnerConnectionLifetime(store, mode);
+
+        var corelationId = Activity.Current is not null ? Activity.Current.RootId : Guid.NewGuid().ToString();
+        var tags = new Dictionary<string, object>


Prefer TagsList when dealing with OTEL tags. This should also be lazy initialized only if EventTracingConnectionLifetime is being used.

Hawxy · 2024-04-03T02:27:41Z

src/Marten/Services/SessionOptions.cs

+        return OpenTelemetryOptions.TrackConnectionEvents
+            ? new EventTracingConnectionLifetime(innerConnectionLifetime, OpenTelemetryOptions,
+                StartConnectionActivity(Activity.Current, tags))
+            : innerConnectionLifetime;


Ideally we only allocate the event tracing lifetime if there's any active listeners:

Suggested change

return OpenTelemetryOptions.TrackConnectionEvents

? new EventTracingConnectionLifetime(innerConnectionLifetime, OpenTelemetryOptions,

StartConnectionActivity(Activity.Current, tags))

: innerConnectionLifetime;

return (OpenTelemetryOptions.TrackConnectionEvents && ActivitySource.HasListeners())

? new EventTracingConnectionLifetime(innerConnectionLifetime, OpenTelemetryOptions,

StartConnectionActivity(Activity.Current, tags))

: innerConnectionLifetime;

…tch pages fails.

…constants.

…time if we have listeners.

SeanFarrow · 2024-04-03T13:54:50Z

OK, I think I've fixed everything, can people please check they are happy. I'll then look at tests.
@jeremydmiller Do you have any opposition to using Testcontainers in a separate project to run prometheus?

SeanFarrow · 2024-04-17T20:51:53Z

@jeremydmiller I'm just adding tests. Things gone a bit mad at work as I've just switched teams.
I'll push what I've got later today. It's up to you whether you pick this last bit up.

I'll start the event/other stuff at the end of next week when things calm down.
Thanks,
Sean.

SeanFarrow · 2024-04-17T21:57:51Z

Update: I should be code complete by tomorrow evening UK time. The only thing to then do is the docs. Thanks, Sean. From: Jeremy D. Miller ***@***.***> Sent: Wednesday, April 17, 2024 7:41 PM To: JasperFx/marten ***@***.***> Cc: Sean Farrow ***@***.***>; Mention ***@***.***> Subject: Re: [JasperFx/marten] Add OpenTelemetry event tracing to connections (PR #3102) @SeanFarrow<https://github.com/SeanFarrow> Anything new on this? Is there anything you're blocked on? — Reply to this email directly, view it on GitHub<#3102 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AALDK7V6KZCOHDD4HRLZBLDY5263VAVCNFSM6AAAAABFQLQCQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRRHE3TGNBVGI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

…e SessionOptions class.

…connection events.

…ss and supporting classes.

…tch pages fails.

…constants.

…time if we have listeners.

…t is inline with OTel standards.

…tivity methods in the MartenTracing class.

…thod nullable in the MartenTracing class and remove unneeded using declarations.

…ing OpenTelemetry when the appropriate option is configured and we have listeners.

…lemetry activity.

…eanFarrow/marten into feature/sf/2909-open-telemetry

jeremydmiller · 2024-04-19T17:38:41Z

@SeanFarrow You'd done a ton man, I'll take you up on some docs, but I'm good to pull this in sooner rather than later. I can easily deal w/ whatever CI hiccups are happening.

SeanFarrow · 2024-04-19T23:45:18Z

@jeremydmiller I'm done and happy.
Please check you are happy with the test for the EventTracingConnectionLifetime class.
Other than that it's just docs and the CI issues.
I should be able to help with the event document listener in a couple of weeks if you don't get to it first.

Thanks.
Sean

jeremydmiller · 2024-04-23T00:43:46Z

@jeremydmiller I'm done and happy. Please check you are happy with the test for the EventTracingConnectionLifetime class. Other than that it's just docs and the CI issues. I should be able to help with the event document listener in a couple of weeks if you don't get to it first.

Thanks. Sean

Cool, and thanks Sean! I'll try to pull this in in the next couple days. Might be until next week before 7.9 gets out with this though.

SeanFarrow added 3 commits April 2, 2024 09:25

Add a class to hold options pertaining to OpenTelemetry and update th…

9ff77a8

…e SessionOptions class.

Add a raw implmenetation of IConnectionLifetime allowing us to track …

b823a91

…connection events.

Complete the implementation of the EventTracingConnectionLifetime cla…

72e9668

…ss and supporting classes.

SeanFarrow force-pushed the feature/sf/2909-open-telemetry branch from 9abe946 to 72e9668 Compare April 2, 2024 08:25

gfoidl reviewed Apr 2, 2024

View reviewed changes

erdtsieck and others added 3 commits April 2, 2024 09:21

JasperFx#3078 Providing a version for aggregating that is too high sh…

ba0515b

…ould aggregate to null

Fixed the combination of using an Include() + Select() where the sele…

5259eea

…ct uses the Id of the original document. Closes JasperFxGH-3096

Adjusted the usage of metadata column data in patch expressions. Closes

0306479

JasperFxGH-2995

Hawxy reviewed Apr 3, 2024

View reviewed changes

SeanFarrow and others added 15 commits April 3, 2024 13:01

Make strings constant where it makes sense.

421521c

Added lock to prevent duplicate code generation

3cd6ae3

Added test for concurrent code generation

6d69c8c

Extracted CreateDocumentProvider method

d02ce6c

Added test for concurrently appending DocumentProviders

1c094ea

Update vitepress and misc doc updates

0c4bc19

Use StartActivity rather than CreateActivity and fix formatting.

07132c7

Super minor naming issue

bcc0555

Refactor the code to add events when executing a command, batch or ba…

ac18d10

…tch pages fails.

Remove redundant conditional checks.

1e903f9

Make the database activity nullable.

52f29d1

Unneeded parameter removal, some reformatting and use of more string …

4622a59

…constants.

More string constants.

cc8d702

OpenTelemetryOptions updates.

9b96118

Remove unneeded constants and only use the EventTracingConnectionLife…

d64cebc

…time if we have listeners.

Bumping to 7.8

b64b96c

Add tests.

a6b5e22

SeanFarrow added 21 commits April 18, 2024 07:11

Add a class to hold options pertaining to OpenTelemetry and update th…

2119830

…e SessionOptions class.

Add a raw implmenetation of IConnectionLifetime allowing us to track …

9b7b8fd

…connection events.

Complete the implementation of the EventTracingConnectionLifetime cla…

149da3d

…ss and supporting classes.

Make strings constant where it makes sense.

6da793a

Use StartActivity rather than CreateActivity and fix formatting.

75dcee0

Refactor the code to add events when executing a command, batch or ba…

835c6c7

…tch pages fails.

Remove redundant conditional checks.

bcec43f

Make the database activity nullable.

1b0ba4f

Unneeded parameter removal, some reformatting and use of more string …

ab74f29

…constants.

More string constants.

afb72d2

OpenTelemetryOptions updates.

ac9b131

Remove unneeded constants and only use the EventTracingConnectionLife…

75e4cfb

…time if we have listeners.

Fix build errors.

abad98f

Use the RecordException method in the OpenTelemetry.Api NuGet packages.

6168adb

Remove unused constants and change the marten tenant id constant so i…

989ee3f

…t is inline with OTel standards.

Use the ActivityTagsCollection for the tags parameter in all start ac…

f923278

…tivity methods in the MartenTracing class.

Enable nullable annotations where required, make the StartActivity me…

5884ce9

…thod nullable in the MartenTracing class and remove unneeded using declarations.

Add tests for the Initialize method to ensure we only track events us…

b81e588

…ing OpenTelemetry when the appropriate option is configured and we have listeners.

Update the EventTracingConnectionLifetime class so it owns the OpenTe…

b150d00

…lemetry activity.

Add tests.

c76e556

Merge branch 'feature/sf/2909-open-telemetry' of https://github.com/S…

2f67edb

…eanFarrow/marten into feature/sf/2909-open-telemetry

Tests for the EventTracingConnectionLifetim class.

05692ce

jeremydmiller merged commit 05692ce into JasperFx:master Apr 29, 2024
7 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenTelemetry event tracing to connections #3102

Add OpenTelemetry event tracing to connections #3102

SeanFarrow commented Mar 31, 2024

jeremydmiller commented Apr 2, 2024

SeanFarrow commented Apr 2, 2024 via email

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

gfoidl Apr 2, 2024

SeanFarrow Apr 2, 2024

gfoidl Apr 2, 2024

jeremydmiller Apr 2, 2024

SeanFarrow Apr 2, 2024

jeremydmiller Apr 2, 2024

Hawxy Apr 3, 2024

jeremydmiller commented Apr 2, 2024

Hawxy Apr 3, 2024

Hawxy Apr 3, 2024

Hawxy Apr 3, 2024

SeanFarrow commented Apr 3, 2024

SeanFarrow commented Apr 17, 2024

SeanFarrow commented Apr 17, 2024 via email

jeremydmiller commented Apr 19, 2024

SeanFarrow commented Apr 19, 2024

jeremydmiller commented Apr 23, 2024

		public static string MartenTenantId = "MartenTentantId";
		public static string MartenCorelationId = "MartenCorelationId";

	public static Activity? StartConnectionActivity(Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags =null)
	public static Activity? StartConnectionActivity(Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags = null)

	public static Activity StartActivity(string spanName, Activity? parentActivity =null, IEnumerable<KeyValuePair<string, object?>>? tags =null, ActivityKind activityKind =ActivityKind.Internal)
	public static Activity StartActivity(string spanName, Activity? parentActivity = null, IEnumerable<KeyValuePair<string, object?>>? tags = null, ActivityKind activityKind = ActivityKind.Internal)

	return ActivitySource.CreateActivity(spanName, activityKind, parentActivity?.ParentId, tags);
	return ActivitySource.StartActivity(spanName, activityKind, parentActivity?.ParentId, tags);

	public bool TrackConnectionEvents { get; set; } = false;
	public bool TrackConnectionEvents { get; set; }

	var innerConnectionLifetime =GetInnerConnectionLifetime(store, mode);
	var innerConnectionLifetime = GetInnerConnectionLifetime(store, mode);

Add OpenTelemetry event tracing to connections #3102

Add OpenTelemetry event tracing to connections #3102

Conversation

SeanFarrow commented Mar 31, 2024

jeremydmiller commented Apr 2, 2024

SeanFarrow commented Apr 2, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremydmiller commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanFarrow commented Apr 3, 2024

SeanFarrow commented Apr 17, 2024

SeanFarrow commented Apr 17, 2024 via email

jeremydmiller commented Apr 19, 2024

SeanFarrow commented Apr 19, 2024

jeremydmiller commented Apr 23, 2024