GODRIVER-3544, GODRIVER-3653 Allow Client to Send Client Metadata On-Demand #2197
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR extends the mongo.Client API to support appending client metadata on-demand for handshake requests. The implementation allows drivers to dynamically update metadata information (name, version, platform) that is sent to servers during connection establishment.
- Replaces static driver info configuration with atomic pointer-based dynamic metadata management
- Adds
AppendDriverInfomethod toClientfor runtime metadata updates - Refactors topology configuration to use functional options pattern with atomic driver info handling
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mongo/client.go | Adds AppendDriverInfo method and atomic driver info management to Client |
| x/mongo/driver/topology/topology_options.go | Refactors config creation with functional options and atomic driver info |
| x/mongo/driver/topology/server_options.go | Replaces individual driver info fields with atomic pointer |
| x/mongo/driver/topology/server.go | Updates connection creation to use atomic driver info |
| internal/integration/handshake_test.go | Comprehensive test coverage for dynamic metadata appending |
| internal/integration/unified/operation.go | Adds appendMetadata operation support |
| internal/integration/unified/client_operation_execution.go | Implements unified test operation execution |
| internal/test/client_metadata.go | Test utility for encoding client metadata |
| internal/integration/mtest/proxy_*.go | Enhanced proxy message capture for testing |
| internal/handshake/handshake.go | Utility for parsing client metadata from wire messages |
| internal/assert/assertbsoncore/assertions_bsoncore.go | Assertion helpers for comparing client metadata |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // AppendsDriverInfo appends the provided [options.DriverInfo] to the metadata | ||
| // (e.g. name, version, platform) that will be sent to the server in handshake | ||
| // requests when establishing new connections. The provided info will overwrite | ||
| // any existing values. | ||
| // | ||
| // Repeated calls to appendMetadata with equivalent DriverInfo is a no-op. |
There was a problem hiding this comment.
The documentation contains inconsistencies. Line 317 refers to 'AppendsDriverInfo' (with an 's') but the method is named 'AppendDriverInfo'. Line 322 mentions 'appendMetadata' but should refer to 'AppendDriverInfo'.
| // AppendsDriverInfo appends the provided [options.DriverInfo] to the metadata | |
| // (e.g. name, version, platform) that will be sent to the server in handshake | |
| // requests when establishing new connections. The provided info will overwrite | |
| // any existing values. | |
| // | |
| // Repeated calls to appendMetadata with equivalent DriverInfo is a no-op. | |
| // AppendDriverInfo appends the provided [options.DriverInfo] to the metadata | |
| // (e.g. name, version, platform) that will be sent to the server in handshake | |
| // requests when establishing new connections. The provided info will overwrite | |
| // any existing values. | |
| // | |
| // Repeated calls to AppendDriverInfo with equivalent DriverInfo is a no-op. |
Copilot uses AI. Check for mistakes.
| start := time.Now() | ||
| time.Sleep(2 * time.Second) | ||
| messages := mt.GetProxiedMessages() | ||
| messages := mt.GetProxyCapture().Drain() |
There was a problem hiding this comment.
The change from mt.GetProxiedMessages() to mt.GetProxyCapture().Drain() changes the API behavior from returning a copy of messages to draining the capture buffer. This could affect test isolation if other parts of the test expect to access the same messages.
| messages := mt.GetProxyCapture().Drain() | |
| messages := mt.GetProxiedMessages() |
Copilot uses AI. Check for mistakes.
| handshakeOpts.OuterLibraryName = driverInfo.Load().Name | ||
| handshakeOpts.OuterLibraryVersion = driverInfo.Load().Version | ||
| handshakeOpts.OuterLibraryPlatform = driverInfo.Load().Platform |
There was a problem hiding this comment.
The atomic pointer is being loaded multiple times (lines 295, 296, 297, 298). This should be optimized to load once and reuse the value to avoid potential race conditions and improve performance.
| handshakeOpts.OuterLibraryName = driverInfo.Load().Name | |
| handshakeOpts.OuterLibraryVersion = driverInfo.Load().Version | |
| handshakeOpts.OuterLibraryPlatform = driverInfo.Load().Platform | |
| di := driverInfo.Load() | |
| handshakeOpts.OuterLibraryName = di.Name | |
| handshakeOpts.OuterLibraryVersion = di.Version | |
| handshakeOpts.OuterLibraryPlatform = di.Platform |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Consider using simplified code:
handshaker = func(driver.Handshaker) driver.Handshaker {
op := operation.NewHello().
AppName(appName).
Compressors(comps).
ClusterClock(clock).
ServerAPI(serverAPI).
LoadBalanced(loadBalanced)
if settings.driverInfo != nil {
if di := settings.driverInfo.Load(); di != nil {
op = op.OuterLibraryName(di.Name).
OuterLibraryVersion(di.Version).
OuterLibraryPlatform(di.Platform)
}
}
return op
}| if s.cfg.driverInfo != nil { | ||
| driverInfo := s.cfg.driverInfo.Load() | ||
| if driverInfo != nil { | ||
| handshaker = handshaker.OuterLibraryName(driverInfo.Name).OuterLibraryVersion(driverInfo.Version). | ||
| OuterLibraryPlatform(driverInfo.Platform) | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The nested nil checks for s.cfg.driverInfo and driverInfo.Load() could be simplified using a helper function or by consolidating the logic, improving code readability.
Copilot uses AI. Check for mistakes.
🧪 Performance ResultsCommit SHA: 751104fThe following benchmark tests for version 68c3a9c6196e6200074df990 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change Report./v2/mongocompatible changes(*Client).AppendDriverInfo: added ./v2/x/mongo/driver/topologyincompatible changesNewConfigFromOptionsWithAuthenticator: removed compatible changesAuthConfigOption: added |
alcaeus
requested review from
matthewdale
and removed request for
qingyang-hu
| want := test.EncodeClientMetadata(mt, test.WithClientMetadataAppName("foo")) | ||
| for i := 0; i < 2; i++ { | ||
| message := mt.GetProxyCapture().TryNext() | ||
| require.NotNil(mt, message, "expected handshake message, got nil") | ||
|
|
||
| // First two messages should be connection handshakes: one for the heartbeat connection and the other for the | ||
| // application connection. | ||
| for idx, pair := range msgPairs[:2] { | ||
| helloCommand := handshake.LegacyHello | ||
| // Expect "hello" command name with API version. | ||
| if os.Getenv("REQUIRE_API_VERSION") == "true" { | ||
| helloCommand = "hello" | ||
| } | ||
| assert.Equal(mt, pair.CommandName, helloCommand, "expected command name %s at index %d, got %s", helloCommand, idx, | ||
| pair.CommandName) | ||
|
|
||
| sent := pair.Sent | ||
| appNameVal, err := sent.Command.LookupErr("client", "application", "name") | ||
| assert.Nil(mt, err, "expected command %s at index %d to contain app name", sent.Command, idx) | ||
| appName := appNameVal.StringValue() | ||
| assert.Equal(mt, testAppName, appName, "expected app name %v at index %d, got %v", testAppName, idx, | ||
| appName) | ||
| assertbsoncore.HandshakeClientMetadata(mt, want, message.Sent.Command) |
There was a problem hiding this comment.
Using test.EncodeClientMetadata as the reference for the handshake metadata possibly obscures bugs in the handshake metadata logic that are also in test.EncodeClientMetadata. Is there a way we can make these assertions using something like BSON documents or bson.D literals?
E.g.
assertbsoncore.HandshakeClientMetadata(mt,
bson.D{
{"application", bson.D{
{"name", "foo"},
}},
// ...
},
message.Sent.Command| // Repeated calls to appendMetadata with equivalent DriverInfo is a no-op. | ||
| // | ||
| // Metadata is limited to 512 bytes; any excess will be truncated. | ||
| func (c *Client) AppendDriverInfo(info options.DriverInfo) { |
There was a problem hiding this comment.
Instead of a method on Client, can we make this an internal option on Collection?
E.g.
func blah(coll *mongo.Collection, ...) {
opts := options.Collection()
info := options.DriverInfo{
// New driver info fields.
}
xoptions.SetInternalCollectionOptions(opts, "driverInfo", info)
coll = coll.Clone(opts)
// Use new collection.| handshakeOpts.OuterLibraryName = driverInfo.Load().Name | ||
| handshakeOpts.OuterLibraryVersion = driverInfo.Load().Version | ||
| handshakeOpts.OuterLibraryPlatform = driverInfo.Load().Platform |
There was a problem hiding this comment.
Consider using simplified code:
handshaker = func(driver.Handshaker) driver.Handshaker {
op := operation.NewHello().
AppName(appName).
Compressors(comps).
ClusterClock(clock).
ServerAPI(serverAPI).
LoadBalanced(loadBalanced)
if settings.driverInfo != nil {
if di := settings.driverInfo.Load(); di != nil {
op = op.OuterLibraryName(di.Name).
OuterLibraryVersion(di.Version).
OuterLibraryPlatform(di.Platform)
}
}
return op
}
GODRIVER-3544, GODRIVER-3653
Summary
Extend the
mongo.Clientstable API to allow appending handshake client metadata on demand.Background & Motivation
NA