CSHARP-5693: Allow Instantiated MongoClients to Send Client Metadata On-Demand by adelinowona · Pull Request #2044 · mongodb/mongo-csharp-driver

adelinowona · 2026-06-22T19:30:32Z

No description provided.

…ta on-demand

Copilot

Warning

Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

Pull request overview

Adds support for updating/sending MongoDB handshake client metadata on-demand from an already-instantiated MongoClient, and wires this through the cluster/connection initialization path. It also introduces unified and prose spec test coverage for the MongoDB handshake metadata update behavior.

Changes:

Add MongoClient.AppendMetadata(LibraryInfo) API and propagate metadata updates into cluster handshake metadata used for new connections.
Introduce ClientMetadata (cached/appendable client handshake document) and thread it through cluster/connection factory construction.
Add unified-spec operation support (appendMetadata) and handshake prose/unified tests.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTestOperationFactory.cs	Registers the new unified test operation name `appendMetadata` for client entities.
tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedAppendMetadataOperation.cs	Implements the unified test operation and argument parsing for `driverInfoOptions`.
tests/MongoDB.Driver.Tests/Specifications/UnifiedTestSpecRunner.cs	Adds a new unified spec suite entry for `mongodb_handshake.tests.unified`.
tests/MongoDB.Driver.Tests/Specifications/mongodb-handshake/prose-tests/MongoDbHandshakeProseTests.cs	Adds prose tests asserting metadata updates affect subsequent handshakes on new connections.
tests/MongoDB.Driver.Tests/MongoClientTests.cs	Adds unit tests for `MongoClient.AppendMetadata` argument validation and disposal behavior.
tests/MongoDB.Driver.Tests/Core/Connections/HelloHelperTests.cs	Updates reflection-based test to account for `ConnectionInitializer` now holding `ClientMetadata`.
tests/MongoDB.Driver.Tests/Core/Connections/ClientMetadataTests.cs	Adds targeted tests for `ClientMetadata` caching, append/dedup, and normalization behavior.
tests/MongoDB.Driver.Tests/Core/Connections/ClientDocumentHelperTests.cs	Adds coverage for appending library platform into the top-level `platform` handshake field.
tests/MongoDB.Driver.Tests/Core/Connections/BinaryConnectionFactoryTests.cs	Updates factory construction to pass `ClientMetadata`.
tests/MongoDB.Driver.Tests/Core/Configuration/LibraryInfoTests.cs	Adds tests for new `LibraryInfo.Platform` behavior and equality semantics.
src/MongoDB.Driver/MongoClient.cs	Adds public `AppendMetadata(LibraryInfo)` and validates separator constraints before delegating to cluster.
src/MongoDB.Driver/Core/Connections/ConnectionInitializer.cs	Switches from a static `_clientDocument` to `_clientMetadata.GetClientDocument()` at handshake time.
src/MongoDB.Driver/Core/Connections/ClientMetadata.cs	New class managing cached handshake client document + appended `LibraryInfo` entries (thread-safe).
src/MongoDB.Driver/Core/Connections/ClientDocumentHelper.cs	Appends `LibraryInfo.Platform` onto the handshake `platform` field when provided.
src/MongoDB.Driver/Core/Connections/BinaryConnectionFactory.cs	Accepts and validates `ClientMetadata`, passing it into `ConnectionInitializer`.
src/MongoDB.Driver/Core/Configuration/LibraryInfo.cs	Adds `Platform` field/ctor and updates equality/to-string accordingly.
src/MongoDB.Driver/Core/Configuration/ClusterBuilder.cs	Creates a shared `ClientMetadata` instance and threads it through server/pool/monitor factories and cluster factory.
src/MongoDB.Driver/Core/Clusters/SingleServerCluster.cs	Plumbs `ClientMetadata` through to base `Cluster`.
src/MongoDB.Driver/Core/Clusters/MultiServerCluster.cs	Plumbs `ClientMetadata` through to base `Cluster`.
src/MongoDB.Driver/Core/Clusters/LoadBalancedCluster.cs	Stores `ClientMetadata` and implements `AppendClientMetadata` for load balanced mode.
src/MongoDB.Driver/Core/Clusters/ICluster.cs	Adds internal `AppendClientMetadata(LibraryInfo)` to the `IClusterInternal` contract.
src/MongoDB.Driver/Core/Clusters/ClusterFactory.cs	Passes shared `ClientMetadata` when constructing clusters.
src/MongoDB.Driver/Core/Clusters/Cluster.cs	Stores `ClientMetadata` and implements `AppendClientMetadata` for non-load balanced clusters.

Files excluded by content exclusion policy (2)

specifications/mongodb-handshake/tests/unified/metadata-not-propagated.json
specifications/mongodb-handshake/tests/unified/metadata-not-propagated.yml

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sanych-sun · 2026-06-23T01:13:01Z

+
+        lock (_lock)
+        {
+            if (_libraryInfos.Contains(libraryInfo))


Should we use ConcurrentDictionary<LibraryInfo, bool> instead to avoid exclusive lock? As per discussion with @BorisDog most likely EF will need to AppendMetadata on each DbContext creation, which most likely is per-request. We might want to reduce on locks here.

I believe the hot path here will be GetClientDocument(), which runs on every connection handshake — and it's already lock-free: it does a volatile read of the cached document and returns without taking the lock once warmed. The lock only guards Append, which the EF per-DbContext pattern will probably call with the same LibraryInfo each time, so after the first call every subsequent one is a dedup no-op over a tiny List.Contains. The critical section is a few nanoseconds and uncontended in that pattern, so I don't think the per-request lock is a real cost.

ConcurrentDictionary<LibraryInfo, bool> also wouldn't be a solution: its enumeration order is unspecified, and we rely on insertion order to build the name|version|platform lists deterministically (covered by Append_should_accumulate_library_infos_in_order). Switching to it would make the handshake string nondeterministic.

_libraryInfos.Contains(libraryInfo) is under the lock as well. Should we switch to use ReaderWriterLockSlim then? So the Contains part could be done under shared read lock?

IMO, as Adelin mentioned, it's not crucial optimization, as it's not a hot path, happens once on startup. So we can prefer a simpler code in this case. But not harm in introducing a quick optimization, if we can agree on one that doesn't complicate things.

I think ReaderWriterLockSlim has materially higher per-acquire overhead than a plain Monitor/lock here. It only pays off when read critical sections are long and highly contended. Our read critical section is an O(n) Contains over a list of ~1–3 entries — a handful of nanoseconds. Wrapping that in RW-lock bookkeeping would likely make the common path slower, not faster but not sure on how to test this exactly. WDYT? @sanych-sun

it's not a hot path, happens once on startup

I do not think it's actually true. If EF for example has to set this on DbContext creation (probably need input from @damieng here) it means this code will be executed once per request if we are talking about Web API application:

A DbContext instance is designed to be used for a single unit-of-work. This means that the lifetime of a DbContext instance is usually very short.

https://learn.microsoft.com/en-us/ef/core/dbcontext-configuration/

I still think that Contains should be done outside of the exclusive lock. I suppose we can do it outside of lock at all, because underlying collection is append only. Even if a concurrent thread will change the collection it could be false-negative check, which we will mitigate by double-checking inside the exclusive lock while adding the library info.

a handful of nanoseconds.

It's true that Contains for a small lists are extremally fact, but having the exclusive lock - it makes all requests go through the single exclusive really short lock - which makes it a bottleneck for system performance.

Yes, I missed this scenario, append with same metadata can actually be on a hot path.
So we should try to avoid lock for duplicates check.

sanych-sun · 2026-06-23T01:22:00Z

+        // entries are normalized on insertion, so null is the only unset form
+        var name = string.Join("|", _libraryInfos.Select(libraryInfo => libraryInfo.Name));
+        var version = string.Join("|", _libraryInfos.Where(libraryInfo => libraryInfo.Version != null).Select(libraryInfo => libraryInfo.Version));
+        var platform = string.Join("|", _libraryInfos.Where(libraryInfo => libraryInfo.Platform != null).Select(libraryInfo => libraryInfo.Platform));


If we filtering out nulls, will it make the result readable?:
"A|B|C;5.1|1.2;"

How do we know if 1.2 it's a version for B or C?
Is this prescribed by spec?

The handshake spec only requires that appended values go to their respective fields delimited by "|" . It doesn't prescribe positional alignment or empty placeholders for libraries missing a version. Omitting unset versions also matches our original single-append behavior (AppendLibraryInfoToDriverDocument already appends name unconditionally and version only when present), so this keeps the two paths consistent.

I am not sure how the analytics side parses this info accurately but I'll check

Yeah that's a good question.
I'd expect "|" to be omitted for null values as well.

It's discussed here. I agree with Valentin that this should be clarified in the spec, and have proper tests.

- Move client metadata separator validation into LibraryInfo constructor - Move library info concatenation from ClientMetadata into ClientDocumentHelper - Remove unused ConnectionInitializer constructor overload - Clarify LibraryInfo.Platform documentation

BorisDog · 2026-06-24T18:15:37Z

+
+        lock (_lock)
+        {
+            if (_libraryInfos.Contains(libraryInfo))


IMO, as Adelin mentioned, it's not crucial optimization, as it's not a hot path, happens once on startup. So we can prefer a simpler code in this case. But not harm in introducing a quick optimization, if we can agree on one that doesn't complicate things.

BorisDog · 2026-06-24T18:28:15Z

+        // entries are normalized on insertion, so null is the only unset form
+        var name = string.Join("|", _libraryInfos.Select(libraryInfo => libraryInfo.Name));
+        var version = string.Join("|", _libraryInfos.Where(libraryInfo => libraryInfo.Version != null).Select(libraryInfo => libraryInfo.Version));
+        var platform = string.Join("|", _libraryInfos.Where(libraryInfo => libraryInfo.Platform != null).Select(libraryInfo => libraryInfo.Platform));


Yeah that's a good question.
I'd expect "|" to be omitted for null values as well.

It's discussed here. I agree with Valentin that this should be clarified in the spec, and have proper tests.

Copilot

Warning

Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.

Files excluded by content exclusion policy (2)

specifications/mongodb-handshake/tests/unified/metadata-not-propagated.json
specifications/mongodb-handshake/tests/unified/metadata-not-propagated.yml

- Add IMongoClient.AppendMetadata extension method as a bridge until the method is added to the interface in 4.0 - Remove unnecessary doc comment on internal ClientMetadata - Add delegation tests for the instance and extension AppendMetadata

BorisDog · 2026-06-24T21:45:24Z


        // constructors
-        public ClusterFactory(ClusterSettings settings, IClusterableServerFactory serverFactory, IEventSubscriber eventSubscriber, ILoggerFactory loggerFactory)
+        public ClusterFactory(ClusterSettings settings, IClusterableServerFactory serverFactory, IEventSubscriber eventSubscriber, ILoggerFactory loggerFactory, ClientMetadata clientMetadata = null)


In what cases clientMetadata can be null?

Only in tests. In production, ClusterFactory always receives a non-null clientMetadata instance from ClusterBuilder and forwards it to the created cluster. The null default exists purely so test code that constructs factories/clusters directly — for SDAM/topology tests that have nothing to do with metadata — doesn't have to supply one. I guess a nullable that "can't be null in prod" is a code smell? Lmk if you're not ok with that.

I'd prefer not to have a code tailored to our testing here.

BorisDog · 2026-06-24T21:51:57Z

+
+                if (!string.IsNullOrWhiteSpace(libraryInfo.Version))
+                {
+                    driverVersion = $"{driverVersion}|{libraryInfo.Version}";


Following up on this comment

Has a decision to not append a '|' for empty value been made?

I don't know if a decision has been made, I can't access the document you shared in the thread.

You should have access now.
Intuitively I am leaning towards emitting an pipe for null values, but let's align with that doc.

BorisDog · 2026-06-24T22:00:33Z

+
+        lock (_lock)
+        {
+            if (_libraryInfos.Contains(libraryInfo))


Yes, I missed this scenario, append with same metadata can actually be on a hot path.
So we should try to avoid lock for duplicates check.

sanych-sun · 2026-06-25T22:28:11Z

+
+            var updated = new LibraryInfo[current.Length + 1];
+            Array.Copy(current, updated, current.Length);
+            updated[current.Length] = libraryInfo;


Minor: I believe we can use fancy C# 12 feature here:

var updated = [..current, libraryInfo]

sanych-sun · 2026-06-25T22:32:42Z

+    }
+
+    // empty strings are considered unset, so normalize them to null for deduplication
+    private static LibraryInfo Normalize(LibraryInfo libraryInfo)


Should we move this to LibraryInfo too? Same way as we validate for | symbol, we can validate and adjust the values, instead of instantiating a new instance. Can save a little on allocations.

BorisDog · 2026-06-25T22:25:01Z


        // constructors
-        public ClusterFactory(ClusterSettings settings, IClusterableServerFactory serverFactory, IEventSubscriber eventSubscriber, ILoggerFactory loggerFactory)
+        public ClusterFactory(ClusterSettings settings, IClusterableServerFactory serverFactory, IEventSubscriber eventSubscriber, ILoggerFactory loggerFactory, ClientMetadata clientMetadata = null)


I'd prefer not to have a code tailored to our testing here.

BorisDog · 2026-06-25T22:26:55Z

+
+                if (!string.IsNullOrWhiteSpace(libraryInfo.Version))
+                {
+                    driverVersion = $"{driverVersion}|{libraryInfo.Version}";


You should have access now.
Intuitively I am leaning towards emitting an pipe for null values, but let's align with that doc.

… clusters - Store appended library infos as a copy-on-write immutable array so the dedup check and document build are lock-free; only a genuine addition takes the lock - Move empty/whitespace normalization (and keep '|' validation) into LibraryInfo, removing ClientMetadata.Normalize; simplify ClientDocumentHelper null checks - Require clientMetadata in ClusterFactory and the cluster constructors rather than defaulting to null for tests; remove the now-unreachable null guards

CSHARP-5693: Add MongoClient.AppendMetadata for sending client metada…

efb6c33

…ta on-demand

adelinowona requested review from BorisDog, Copilot and sanych-sun June 22, 2026 19:30

adelinowona requested a review from a team as a code owner June 22, 2026 19:30

adelinowona added the feature Adds new user-facing functionality. label Jun 22, 2026

Copilot started reviewing on behalf of adelinowona June 22, 2026 19:31 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Comment thread src/MongoDB.Driver/Core/Configuration/LibraryInfo.cs Outdated

Comment thread src/MongoDB.Driver/Core/Clusters/Cluster.cs

Comment thread src/MongoDB.Driver/Core/Clusters/LoadBalancedCluster.cs

adelinowona added 2 commits June 22, 2026 15:59

CSHARP-5693: Address Copilot review comments

fdbfe61

CSHARP-5693: Fix ClusterBuilderTests for threaded clientMetadata

2286ed8

sanych-sun reviewed Jun 22, 2026

View reviewed changes

Comment thread src/MongoDB.Driver/Core/Clusters/Cluster.cs

sanych-sun requested changes Jun 23, 2026

View reviewed changes

adelinowona requested review from Copilot and sanych-sun June 24, 2026 18:37

BorisDog reviewed Jun 24, 2026

View reviewed changes

Copilot started reviewing on behalf of adelinowona June 24, 2026 18:37 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

Comment thread src/MongoDB.Driver/Core/Connections/ClientMetadata.cs Outdated

adelinowona requested a review from BorisDog June 24, 2026 20:12

BorisDog requested changes Jun 24, 2026

View reviewed changes

adelinowona added 2 commits June 25, 2026 10:56

minor code review changes

a6d5bbc

implement copy on write approach clientmetadata concurrency

817ad95

adelinowona requested a review from BorisDog June 25, 2026 22:06

sanych-sun requested changes Jun 25, 2026

View reviewed changes

BorisDog requested changes Jun 25, 2026

View reviewed changes

adelinowona requested review from BorisDog and sanych-sun June 26, 2026 00:19

Uh oh!

Conversation

adelinowona commented Jun 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanych-sun Jun 24, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 Jun 24, 2026 •

edited

Loading