JAVA-6222 Support trace context propagation to the server - #2022
JAVA-6222 Support trace context propagation to the server#2022nhachicha wants to merge 49 commits into
Conversation
Spec proposal (OP_MSG section kind 3, W3C traceparent, hello tracingSupport negotiation) plus driver-sync POC design to validate it.
…tion, wire-version gate)
…mpled with flags 00
…version gate supersedes)
Executed end-to-end against mongodb-mongo-master ee3a67f8 (9.0.0-alpha0): ServerSpanLinkageProseTest passes. Confirmed real server parameters (featureFlagOtelTraceSampling, openTelemetryTracingSampling BSON doc, opentelemetryTraceDirectory) and NDJSON export format. Fixed the prose test to assert operation-span parentage: the driver injects the traceparent from the OperationContext's operation span (per design 3.1), so the server span is a sibling of the client command span.
…coded-bytes dependency)
…en, limitation noted)
There was a problem hiding this comment.
Pull request overview
Adds W3C traceparent propagation support to the server via a new OP_MSG telemetry section (wire version 29+), integrating with Micrometer tracing while keeping micrometer-tracing optional and guarded to avoid classpath linkage errors.
Changes:
- Add OP_MSG kind-3 telemetry section encoding (
{otel: {traceparent: ...}}) when a traced command span is available and the server wire version supports it. - Extend Micrometer tracing integration with
TraceContext#traceParent()and a classpath guard to safely operate withoutmicrometer-tracingpresent. - Add unit tests covering traceparent formatting/validation, classpath-absence safety, and command message telemetry section encoding/ordering.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Adds a Micrometer tracing library alias to support optional tracing integration. |
| driver-core/build.gradle.kts | Adds optional micrometer-tracing plus tracing test dependencies for new unit tests. |
| driver-core/src/test/unit/com/mongodb/internal/observability/micrometer/MicrometerTraceParentTest.java | Verifies traceparent formatting/validation and no-linkage-error behavior when tracing is absent. |
| driver-core/src/test/unit/com/mongodb/internal/connection/CommandMessageOtelTraceContextTest.java | Validates telemetry section encoding, ordering vs sequences, and command reconstruction behavior. |
| driver-core/src/main/com/mongodb/internal/operation/ServerVersionHelper.java | Introduces wire version constant for server 9.0 / wire version 29. |
| driver-core/src/main/com/mongodb/internal/observability/micrometer/TracingManager.java | Uses CommandMessage helpers instead of needing the encoded command document to populate tracing context. |
| driver-core/src/main/com/mongodb/internal/observability/micrometer/TraceContext.java | Adds traceParent() API for W3C context propagation. |
| driver-core/src/main/com/mongodb/internal/observability/micrometer/Span.java | Clarifies scope open/close semantics and no-op close behavior. |
| driver-core/src/main/com/mongodb/internal/observability/micrometer/MicrometerTracer.java | Implements traceParent() with optional-dependency guard and validation to avoid emitting rejected traceparents. |
| driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java | Ensures encoding can attach telemetry section by passing the tracing span into message encoding. |
| driver-core/src/main/com/mongodb/internal/connection/CommandMessage.java | Adds telemetry-section encoding + helpers (getCommandName, getGetMoreCursorId) and updates command reconstruction to ignore telemetry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
driver-core/src/main/com/mongodb/internal/operation/ServerVersionHelper.java:38
LATEST_WIRE_VERSIONis still set to the 8.0 value even though this PR introducesNINE_DOT_ZERO_WIRE_VERSION. This makes the constant name misleading and causes unit tests that useLATEST_WIRE_VERSIONto stop representing the newest supported wire version.
// Server 9.0 (WIRE_VERSION_90 = 29). Minimum wire version for the OP_MSG telemetry
// section (OTel trace-context propagation, DRIVERS-3454).
public static final int NINE_DOT_ZERO_WIRE_VERSION = 29;
public static final int LATEST_WIRE_VERSION = EIGHT_DOT_ZERO_WIRE_VERSION;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
driver-core/src/main/com/mongodb/internal/operation/ServerVersionHelper.java:37
LATEST_WIRE_VERSIONis now inconsistent with the newly introducedNINE_DOT_ZERO_WIRE_VERSIONconstant. Keeping it pinned to 8.0 makes test utilities and any future uses ofLATEST_WIRE_VERSIONsilently exclude wire version 29 features (like OP_MSG telemetry).
// Server 9.0 (WIRE_VERSION_90 = 29). Minimum wire version for the OP_MSG telemetry
// section (OTel trace-context propagation, DRIVERS-3454).
public static final int NINE_DOT_ZERO_WIRE_VERSION = 29;
public static final int LATEST_WIRE_VERSION = EIGHT_DOT_ZERO_WIRE_VERSION;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
driver-core/src/main/com/mongodb/internal/operation/ServerVersionHelper.java:38
LATEST_WIRE_VERSIONis now inconsistent with the newly addedNINE_DOT_ZERO_WIRE_VERSIONconstant (the file comments call 9.0 / wire version 29 the minimum for telemetry support, butLATEST_WIRE_VERSIONstill points at 8.0 / 25). This makes the meaning of “latest” ambiguous for test setups that importLATEST_WIRE_VERSION.
Consider either updating LATEST_WIRE_VERSION to NINE_DOT_ZERO_WIRE_VERSION, or adjusting naming/docs to clarify why 8.0 remains the “latest” for the driver/test suite.
// Server 9.0 (WIRE_VERSION_90 = 29). Minimum wire version for the OP_MSG telemetry
// section (OTel trace-context propagation, DRIVERS-3454).
public static final int NINE_DOT_ZERO_WIRE_VERSION = 29;
public static final int LATEST_WIRE_VERSION = EIGHT_DOT_ZERO_WIRE_VERSION;
driver-core/src/main/com/mongodb/internal/connection/CommandMessage.java:221
- Javadoc has an unbalanced parenthesis: “(first key of the command document.” should be “(first key of the command document).”
* The command name (first key of the command document. Available before {@code encode()} runs, unlike
rozza
left a comment
There was a problem hiding this comment.
Looks good - have a few comments / suggestions / questions and nits.
A wider question is should we bump micrometer in versions? Currently we have:
micrometer-tracing = "1.6.0-M3" # This version has a fix for https://github.com/micrometer-metrics/tracing/issues/1092
micrometer-observation-bom = "1.15.4"
Should we just bump to 1.16.6 (latest 1.16.6) or 1.17.0 (latest)?
| * key, since encoding only appends fields. | ||
| */ | ||
| public String getCommandName() { | ||
| return command.getFirstKey(); |
There was a problem hiding this comment.
Is it worth caching this in the constructor? I don't think theres a path where we don't call it?
| * on the classpath (e.g. metrics/logging-only observability setups). Guard any use of {@code io.micrometer.tracing.*} | ||
| * types with this flag so that such setups never trigger a {@link NoClassDefFoundError} or {@link LinkageError}. | ||
| */ | ||
| static final boolean MICROMETER_TRACING_ON_CLASSPATH = isMicrometerTracingOnClasspath(); |
There was a problem hiding this comment.
Nit: Lets annotation this as visible for testing otherwise private:
@VisibleForTesting(otherwise = AccessModifier.PRIVATE)
I would have suggested but imports are annoying with suggestions.
| * when {@code micrometer-tracing} is absent from the classpath. | ||
| */ | ||
| private static final class MicrometerTracingSupport { | ||
| @Nullable |
There was a problem hiding this comment.
Suggest adding a simple javadoc to help grok this.
| @Nullable | |
| /** | |
| * Computes the W3C {@code traceparent} for {@code observation}'s active span. | |
| * | |
| * @param observation the observation whose span supplies the trace context | |
| * @return the {@code traceparent} as specified by {@link TraceContext#traceParent()}, or {@code null} when | |
| * {@code observation} has no span or its trace/span ids are missing, zero, or malformed | |
| */ | |
| @Nullable |
| * The 55-char W3C {@code traceparent} string for this context | ||
| * ({@code 00-<32 hex traceId>-<16 hex spanId>-<flags>}; flags {@code 01} sampled / {@code 00} unsampled), | ||
| * or {@code null} when there is no valid context (no-op span, missing/zero/malformed ids). | ||
| * Never includes tracestate. |
| BsonInt64 getMoreCursorId = message.getGetMoreCursorId(); | ||
| if (getMoreCursorId != null) { | ||
| mongodbContext.setCursorId(getMoreCursorId.longValue()); |
There was a problem hiding this comment.
See previous suggestion for using a Long
| BsonInt64 getMoreCursorId = message.getGetMoreCursorId(); | |
| if (getMoreCursorId != null) { | |
| mongodbContext.setCursorId(getMoreCursorId.longValue()); | |
| Long getMoreCursorId = message.getGetMoreCursorId(); | |
| if (getMoreCursorId != null) { | |
| mongodbContext.setCursorId(getMoreCursorId); |
| /** | ||
| * Walks the OP_MSG sections skip the type-0 body document, then for each subsequent section read the kind byte; | ||
| * for kind 1 (document sequence) skip past the {@code int32} section size, and for kind 3 (telemetry) decode and | ||
| * return the BSON document payload. Returns {@code null} if no kind-3 section is found. |
There was a problem hiding this comment.
Nit: Slightly easier to read
| /** | |
| * Walks the OP_MSG sections skip the type-0 body document, then for each subsequent section read the kind byte; | |
| * for kind 1 (document sequence) skip past the {@code int32} section size, and for kind 3 (telemetry) decode and | |
| * return the BSON document payload. Returns {@code null} if no kind-3 section is found. | |
| /** | |
| * Walks the OP_MSG sections: skips the type-0 body document, then reads the kind byte of each subsequent | |
| * section. For kind 1 (document sequence) it skips past the {@code int32} section size; for kind 3 (telemetry) | |
| * it decodes and returns the BSON document payload. Returns {@code null} if no kind-3 section is found. | |
| */ |
| */ | ||
| public void encode(final ByteBufferBsonOutput bsonOutput, final OperationContext operationContext, | ||
| @Nullable final Span commandSpan) { | ||
| this.commandSpanForEncoding = commandSpan; |
There was a problem hiding this comment.
Question: This feels like an antipattern - global state that is safe with the try finally but its a bit hidden.
Would it be clearer if this was threaded into encode as a parameter instead?
| public BsonInt64 getGetMoreCursorId() { | ||
| BsonValue value = command.get("getMore"); | ||
| return value instanceof BsonInt64 ? (BsonInt64) value : null; | ||
| } |
There was a problem hiding this comment.
I'd recommend changing to a Long as that mirrors the getCursorId methods in the code base. See: CommandCursorResult setting of cursorId
| public BsonInt64 getGetMoreCursorId() { | |
| BsonValue value = command.get("getMore"); | |
| return value instanceof BsonInt64 ? (BsonInt64) value : null; | |
| } | |
| public Long getGetMoreCursorId() { | |
| BsonValue value = command.get("getMore"); | |
| return value.isNumber() ? value.asNumber().longValue() : null; | |
| } |
There was a problem hiding this comment.
I also made a suggestion below to change getGetMoreCursorId use.
| @@ -441,20 +439,19 @@ public boolean reauthenticationIsTriggered(@Nullable final Throwable t) { | |||
| @Nullable | |||
| private <T> T sendAndReceiveInternal(final CommandMessage message, final Decoder<T> decoder, | |||
There was a problem hiding this comment.
I got this feedback from Claude:
🟡 `[important]` — `InternalStreamConnection.java`: test gap
The connection-level rewiring introduced here has no direct coverage. The new CommandMessage/traceparent tests exercise encode(...) in
isolation and never construct an InternalStreamConnection, and the existing InternalStreamConnectionSpecification predates these changes.
Not covered:
1. That the sync and async paths actually call encode(bsonOutput, operationContext, tracingSpan) (the 3-arg overload) with the span from
createTracingSpan. A regression to the 2-arg overload or null would pass all current tests yet silently stop emitting the telemetry
section.
2. The reorder — span created before encode.
3. Exactly-once semantics under a send failure: the widened catch (Throwable) firing sendFailedEvent + span.error()/closeScope()/end()
once each, without the receive path double-firing.
4. The intentional sync/async asymmetry in the catch (async does no closeScope() / direct sendFailedEvent).
Notes:
• End-to-end server acceptance of the kind-3 section is 9.0-gated and reasonably deferred.
• The wiring / ordering / failure paths are unit-testable with the existing TestInternalConnection + Spock harness and a mocked
TracingManager/Span.
• Per AGENTS.md ("every code change must include tests"), this should be covered on a safety-critical send path.
It may be worth auto migrating InternalStreamConnectionSpecification.groovy to Junit 5 and adding extra coverage tests.
JAVA-6222
This is the reference implementation for the Spec DRIVERS-3454 Support trace context propagation to the server specifications#1966
This PR also bump the wire protocol to 29 per https://jira.mongodb.org/browse/JAVA-6164