CSHARP-3124: OpenTelemetry implementation #1837

adelinowona · 2025-12-09T16:57:25Z

No description provided.

sanych-sun

Partial review. Also please check failing tests

sanych-sun · 2025-12-09T21:51:13Z

src/MongoDB.Driver/MongoCollectionImpl.cs

+        {
+            // Check if all requests are of the same type
+            var firstType = requests[0].ModelType;
+            var allSameType = requests.All(r => r.ModelType == firstType);


Having this code here will lead to multiple enumerations of the requests. We had a ticket to remove the multiple enumerations. Should we move this code into the CreateBulkWriteOperation and merge it into the mapping loop?

sanych-sun · 2025-12-09T21:56:19Z

src/MongoDB.Driver/Core/Connections/CommandEventHelper.cs

+                    connectionId,
+                    $"{state.CommandName} command failed",
+                    null,
+                    reply);


So we construct the exception just to record it and never throw it? Looks suspicious.

Good catch - this does look suspicious at first glance. This pattern actually predates my telemetry changes and is part of the existing architecture.

Pre-existing design:
When the server returns ok: 0, there are two separate exceptions created for different layers:

Event/Observability exception - never thrown
- Created for the CommandFailedEvent public API
- Event subscribers (logging infrastructure, TraceSource) seem to need the exception object.
- This is the exception you're seeing created here

Protocol exception (ProcessReply in wire protocol layer) - actually thrown
- Created after event processing completes
- This is what user code catches

With my telemetry changes:
I'm just reusing the first exception for telemetry tags via RecordException.

sanych-sun · 2025-12-09T22:12:04Z

src/MongoDB.Driver/MongoDB.Driver.csproj

    <PackageReference Include="Snappier" Version="1.0.0" />
    <PackageReference Include="ZstdSharp.Port" Version="0.7.3" />
    <PackageReference Include="System.Buffers" Version="4.5.1" />
+    <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="8.0.0" />


I suppose the System.Diagnostics.DiagnosticSource version should match the target framework.

And looks like it's not needed for net6.

sanych-sun · 2025-12-09T22:16:57Z

src/MongoDB.Driver/OperationContext.cs

+            return new OperationContext(Clock, InitialTimestamp, Timeout, CancellationToken)
+            {
+                RootContext = RootContext,
+                OperationName = operationName,


We should copy values of all otel related fields in WithTimeout method as well. Otherwise they will be lost if we have to apply the timeout.

sanych-sun · 2025-12-09T22:18:52Z

src/MongoDB.Driver/OperationContext.cs

+
+        internal OperationContext WithOperationMetadata(string operationName, string databaseName, string collectionName, bool isTracingEnabled)
+        {
+            return new OperationContext(Clock, InitialTimestamp, Timeout, CancellationToken)


Providing InitialTimestamp and Timeout here looks wrong to me, because copied operationContext is suppose to "continue from the point where it was copied". We probably should provide RemainingTimeout instead, in the same way as WithTimeout doing..

sanych-sun · 2025-12-09T22:19:16Z

src/MongoDB.Driver/OperationContext.cs

            };
        }
+
+        internal OperationContext WithOperationMetadata(string operationName, string databaseName, string collectionName, bool isTracingEnabled)


Please add unit tests for the new method.

sanych-sun · 2025-12-09T23:47:41Z

src/MongoDB.Driver/ClusterKey.cs

        public bool LoadBalanced => _loadBalanced;
        public TimeSpan LocalThreshold { get { return _localThreshold; } }
        public LoggingSettings LoggingSettings { get { return _loggingSettings; } }
+        public TracingOptions TracingOptions { get { return _tracingOptions; } }


Should we add comparison of the TracingOptions to the Equals method?

BorisDog

Looks good overall, partial review.

BorisDog · 2025-12-10T18:11:51Z

src/MongoDB.Driver/MongoDB.Driver.csproj

    <PackageReference Include="Snappier" Version="1.0.0" />
    <PackageReference Include="ZstdSharp.Port" Version="0.7.3" />
    <PackageReference Include="System.Buffers" Version="4.5.1" />
+    <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="8.0.1" />


Should we include this in BSON lib instead, just in case we'd want to add serialization Otel?

BorisDog · 2025-12-10T18:12:39Z

src/MongoDB.Driver/MongoTelemetry.cs

+/// </summary>
+public static class MongoTelemetry
+{
+    private static readonly string s_driverVersion = ClientDocumentHelper.GetAssemblyVersion(typeof(MongoClient).Assembly);


__driverVersion

BorisDog · 2025-12-10T18:18:35Z

src/MongoDB.Driver/MongoTelemetry.cs

+    /// The ActivitySource used by MongoDB driver for OpenTelemetry tracing.
+    /// Applications can subscribe to this source to receive MongoDB traces.
+    /// </summary>
+    public static readonly ActivitySource ActivitySource = new("MongoDB.Driver", s_driverVersion);


Do we want/need to expose this publicly? Is it the recommended way?

BorisDog · 2025-12-11T01:29:52Z

src/MongoDB.Driver/MongoTelemetry.cs

+    {
+        var activity = ActivitySource.StartActivity(commandName, ActivityKind.Client);
+
+        if (activity == null)


minor: activity is checked for null by the callers, so maybe check in one place only?

BorisDog · 2025-12-11T01:32:27Z

src/MongoDB.Driver/MongoTelemetry.cs

+
+            SetConnectionTags(activity, connectionId);
+
+            if (command != null)


In what cases is the command null?

BorisDog · 2025-12-11T01:36:46Z

src/MongoDB.Driver/OperationExecutor.cs

+                    else
+                    {
+                        // For command-level exceptions, only set error status without recording details
+                        activity.SetStatus(ActivityStatusCode.Error);


minor: maybe this whole if/else block should move into MongoTelemetry.RecordException?

BorisDog · 2025-12-11T01:39:44Z

src/MongoDB.Driver/Core/Configuration/TracingOptions.cs

+    /// When set to true, no OpenTelemetry activities will be created for this client's operations.
+    /// Default is false (tracing enabled if configured via TracerProvider).
+    /// </summary>
+    public bool Disabled { get; set; }


Do we need this additional Disabled switch?

BorisDog · 2025-12-11T01:42:39Z

src/MongoDB.Driver/Core/Connections/CommandEventHelper.cs

-            _shouldTrackState = _shouldTrackSucceeded || _shouldTrackFailed;
+
+            // Check if tracing is disabled for this client
+            _shouldTrace = _tracingOptions?.Disabled != true;


This is probably not sufficient.
Do we still want to trace if there are not listeners?

BorisDog · 2025-12-11T01:56:42Z

src/MongoDB.Driver/OperationExecutor.cs

+                    operationContext.OperationName,
+                    operationContext.DatabaseName,
+                    operationContext.CollectionName)
+                : null;


Consider more compact overload:
MongoTelemetry.StartOperationActivity(operationContext);

And ideally (if it's reasonable of course) everything related to telemetry should be handled in single place, so should
TransactionActivityScope be under MongoTelemetry? Should MongoTelemetry.StartOperationActivity also invoke TransactionActivityScope ?

BorisDog · 2025-12-11T02:02:25Z

src/MongoDB.Driver/Core/Connections/CommandEventHelper.cs

            public CollectionNamespace QueryNamespace;
            public ExpectedResponseType ExpectedResponseType;
            public bool ShouldRedactReply;
+            public Activity CommandActivity;


Does this Activity need to be disposed?

adelinowona · 2025-12-11T20:25:54Z

src/MongoDB.Driver/MongoClient.cs

                _settings.TranslationOptions);

-        private OperationContext CreateOperationContext(IClientSessionHandle session, TimeSpan? timeout, CancellationToken cancellationToken)
+        private OperationContext CreateOperationContext(IClientSessionHandle session, TimeSpan? timeout, CancellationToken cancellationToken, string operationName = null)


btw I know Cancellation token should be the last parameter here but I did it this way for now since this work is still a draft and I didn't want to have to update every call.

Same idea stands for ExecuteReadOperation and ExecuteWriteOperation

ajcvickers

I don’t think that Activity can be used in this way. Activity maintains a single ambient instance per execution context. (By execution context, I mean the state around a thread of execution that acts like a conceptual thread to the CLR even though the physical thread may change underneath it.) This means that anything that sets Activity.Current (either implicitly or explicitly) must be paired with an associated resetting to the parent in such a way that the reset can only happen when the Activity.Current is pointing at the correct instance.

This has two consequences. First, a given activity pushed to Current when it starts must be executing in the same execution context when it ends. If the thing we are trying to capture into an activity can start in one execution context and end in another, then I don’t think there is a way of implementing that on .NET.

Second, activities can’t be interleaved. So I can do this:

Start Activity 1
- Start Activity 2
- End Activity 2
End Activity 1

But not this:

Start Activity 1
- Start Activity 2
End Activity 1
- End Activity 2

Because when I end Activity 1, it is not the Current.

These are inherent issues with having an ambient Activity.Current and as far as I know cannot be worked around in any meaningful way.

The design of Activity is that you set Current, make some calls, and then reset it before the method that set it goes out of scope–i.e. as the call stack unwinds. This means that the current activity should never need to be stored anywhere by Activity.Current, and doing so indicates an issue with the design.

ajcvickers · 2026-01-12T13:44:41Z

So, I spent a few hours doing more research on this, talking to Shay, and doing some experiments. Here’s the outcome:

We should not be touching Activity.Current ourselves, no matter what we do. It is handled internally by StartActivity and then the disposal of the Activity object.
But, Activity.Current can only be used when an activity is started and ended in the same execution context, and where activities will be disposed in the reverse order that they were created--see my test code below.
Since we cannot guarantee these things, as indeed Npgsql cannot, then we can’t use Activity.Current. So instead we need to start activities with Source.StartActivity(spanName, ActivityKind.Client) "Client" means it will not touch Activity.Current.
a. We need to store the Activity and dispose it when the activity ends.
This means that Activity.Current will never be the parent of any of the Activity instances that we create. This is what Npgsql does, and there have not been any complaints.

Test code showing Activity.Current corruption:

var source1 = new ActivitySource("Sample.DistributedTracing1", "1.0.0");
var source2 = new ActivitySource("Sample.DistributedTracing2", "1.0.0");

using var tracerProvider = Sdk.CreateTracerProviderBuilder()
    .SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("MySample"))
    .AddSource("Sample.DistributedTracing1")
    .AddSource("Sample.DistributedTracing2")
    .AddConsoleExporter()
    .Build();

Console.WriteLine($"Initial> Activity.CurrentId: {Activity.Current?.Id} with parent {Activity.Current?.Parent?.Id}");

var activity1 = source1.StartActivity("My First Activity");
Console.WriteLine($"After starting activity 1> Activity.CurrentId: {Activity.Current?.Id} with parent {Activity.Current?.Parent?.Id}");

var activity2 = source2.StartActivity("My Second Activity");
Console.WriteLine($"After starting activity 2> Activity.CurrentId: {Activity.Current?.Id} with parent {Activity.Current?.Parent?.Id}");

activity1?.Dispose();
Console.WriteLine($"After disposing activity 1> Activity.CurrentId: {Activity.Current?.Id} with parent {Activity.Current?.Parent?.Id}");

activity2?.Dispose();
Console.WriteLine($"After disposing activity 2> Activity.CurrentId: {Activity.Current?.Id} with parent {Activity.Current?.Parent?.Id}");

Output. Notice that:

After disposing one activity, Current is null when the other is still active.
At the end, even though both activities have been disposed, Activity.Current is still pointing at one of the disposed activities.

Initial> Activity.CurrentId:  with parent 
After starting activity 1> Activity.CurrentId: 00-816662818bab9ca191d40bd5c7464071-891dd16e565f1d39-01 with parent 
After starting activity 2> Activity.CurrentId: 00-816662818bab9ca191d40bd5c7464071-35373ab46a85ece2-01 with parent 00-816662818bab9ca191d40bd5c7464071-891dd16e565f1d39-01
After disposing activity 1> Activity.CurrentId:  with parent 
After disposing activity 2> Activity.CurrentId: 00-816662818bab9ca191d40bd5c7464071-891dd16e565f1d39-01 with parent

adelinowona added the feature Adds new user-facing functionality. label Dec 9, 2025

adelinowona requested review from BorisDog, rstam and sanych-sun December 9, 2025 16:58

adelinowona added 8 commits December 9, 2025 16:33

Add OpenTelemetry specification tests

119ebac

Add core OpenTelemetry infrastructure

e863c98

Integrate tracing configuration into MongoClientSettings

fc9f61f

Implement operation-level tracing

b549899

Add tracing to collection and database operations

039aa24

Implement session and transaction tracing

a075ce1

Implement command-level tracing

d763e6a

Add test infrastructure for OpenTelemetry

f2e383d

adelinowona force-pushed the csharp3124 branch from 53e5434 to f2e383d Compare December 9, 2025 22:20

sanych-sun requested changes Dec 9, 2025

View reviewed changes

adelinowona added 3 commits December 9, 2025 23:21

Fix ClientSessionHandleTests for tests.

723a6e6

Fix CommandEventHelper tests

823deee

fix some more CommandEventHelperTests

0e674f6

BorisDog requested changes Dec 11, 2025

View reviewed changes

adelinowona commented Dec 11, 2025

View reviewed changes

fix spans not capturing in net472 and netstandard test variants

d08095c

ajcvickers reviewed Dec 16, 2025

View reviewed changes


		SetConnectionTags(activity, connectionId);

		if (command != null)

CSHARP-3124: OpenTelemetry implementation #1837

Are you sure you want to change the base?

CSHARP-3124: OpenTelemetry implementation #1837

Uh oh!

Conversation

adelinowona commented Dec 9, 2025

Uh oh!

sanych-sun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelinowona Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanych-sun Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BorisDog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajcvickers left a comment

Choose a reason for hiding this comment

Uh oh!

ajcvickers commented Jan 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adelinowona Jan 9, 2026 •

edited

Loading

sanych-sun Dec 9, 2025 •

edited

Loading