Skip to content

Conversation

nhachicha
Copy link
Contributor

@nhachicha nhachicha commented Apr 29, 2025

@nhachicha nhachicha self-assigned this Apr 29, 2025
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm liking the approach.

Does this meet the spec for transactions and retryable operations?

I'm hoping so as both OperationContext#withSessionContext and OperationContext#withTimeoutContext reuse the operation id.

However, it would be worth clarifying before reviewing further.

@nhachicha nhachicha force-pushed the JAVA-5733 branch 3 times, most recently from de23d90 to 8e9d2b2 Compare July 20, 2025 22:38
@nhachicha nhachicha marked this pull request as ready for review July 21, 2025 00:27
@nhachicha nhachicha requested a review from a team as a code owner July 21, 2025 00:27
@nhachicha nhachicha requested review from NathanQingyangXu, rozza, vbabanin and Copilot and removed request for a team and NathanQingyangXu July 21, 2025 00:27
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive tracing support to the MongoDB Java driver using Micrometer as the tracing framework. The implementation provides tracing for commands, operations, and transactions with span hierarchies that allow for detailed observability of database interactions.

  • Integration of Micrometer tracing with a new MicrometerTracer implementation
  • Command, operation, and transaction span creation with proper parent-child relationships
  • Cursor tracking to link getMore operations with their originating queries
  • Test infrastructure including unified test support and utility classes for span validation

Reviewed Changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
gradle/libs.versions.toml Adds Micrometer tracing dependencies and test bundles
driver-core/src/main/com/mongodb/tracing/MicrometerTracer.java Main tracer implementation that bridges MongoDB driver with Micrometer
driver-core/src/main/com/mongodb/internal/tracing/TracingManager.java Core tracing management with span lifecycle and context handling
driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java Command-level tracing integration with connection handling
driver-sync/src/test/functional/com/mongodb/client/tracing/SpanTree.java Test utility for validating span hierarchies and structures
Multiple build.gradle.kts files Dependency updates across driver modules
Comments suppressed due to low confidence (1)

driver-core/src/test/unit/com/mongodb/internal/connection/CommandHelperTest.java:121

  • [nitpick] Empty line removed unnecessarily. This appears to be an unrelated formatting change that should not be included in a tracing feature PR.
    OperationContext createOperationContext() {

notNull("fullName", fullName);
this.fullName = fullName;
this.databaseName = getDatatabaseNameFromFullName(fullName);
this.databaseName = getDatabaseNameFromFullName(fullName);
Copy link
Preview

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method name corrected from 'getDatatabaseNameFromFullName' to 'getDatabaseNameFromFullName' - this appears to be an unrelated typo fix that should not be included in a tracing feature PR.

Copilot uses AI. Check for mistakes.

private static void assertValid(final SpanNode reportedNode, final SpanNode expectedNode,
final BiConsumer<BsonValue, BsonValue> valueMatcher) {
// Check that the span names match
if (!reportedNode.getName().equalsIgnoreCase(expectedNode.getName())) {
Copy link
Preview

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Using case-insensitive comparison for span names may hide actual mismatches. Consider using exact string comparison unless case variations are explicitly expected.

Suggested change
if (!reportedNode.getName().equalsIgnoreCase(expectedNode.getName())) {
if (!reportedNode.getName().equals(expectedNode.getName())) {

Copilot uses AI. Check for mistakes.

return false;
}
final SpanNode spanNode = (SpanNode) o;
return name.equalsIgnoreCase(spanNode.name)
Copy link
Preview

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Inconsistent case handling in equals method. The case-insensitive comparison should match the behavior used in assertValid method, but consider if this is the intended behavior.

Copilot uses AI. Check for mistakes.

Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review - overall looks really good, just a couple of nits.

My main point of contention is: Is an com.mongodb.internal.tracing the correct place? Given that MongoClientSettings accepts a TracingManager that seems against the norm in the code base. Should there be a TracingManager interface along with the other interfaces and then internal implementations of it? eg: TracingManagerImpl

What do you think?

* @return this
* @since 5.5
*/
@Alpha(Reason.CLIENT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Is this going to be released as alpha?

Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a high level review of the API and the overall design.

import static java.lang.String.format;

/**
* Manages tracing spans for MongoDB driver activities.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear why a class like this is necessary. As I understand it, there is a single instance created at the scope of MongoClientSettings, and treated as effectively a singleton by the MongoClient instance. Would it be sufficient, and also simpler, to create a new instance of some class at the point where we start execution of an operation or transaction, and allow it to garbage collected when the operation or transaction completes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New implementation uses a single instance per MongoClient.

private <T> T sendAndReceiveInternal(final CommandMessage message, final Decoder<T> decoder,
final OperationContext operationContext) {
CommandEventSender commandEventSender;
Span tracingSpan = createTracingSpan(message, operationContext);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider pushing all the changes made to this class down into LoggingCommandEventSender, which already contains a lot of similar logic. LoggingCommandEventSender might need some refactoring as a result, but it seems better than messing too much with this class, which already has enough responsibilities as it is.

@mp911de
Copy link

mp911de commented Aug 6, 2025

Paging @jonatan-ivanov

@jonatan-ivanov
Copy link

Hi,

I'm one of the Maintainers of Micrometer and Micrometer Tracing.
I'm happy you want to use the Tracing API! I can share some feedback on this PR if you would like to receive some.

I think the most important feedback I'd like to give is recommending to use the Observation API instead of "just" the Tracing API. The reason behind it is that this PR adds tracing capabilities but if you also want to add metrics support, this needs to be instrumented again. Also if users want (custom) logs or any custom output that they can figure out based on the instrumentation in these components, that also need to happen through a separate mechanism.
The idea behind the Observation API, is to put these Observability needs behind one API so that you need to instrument your components once and you can get any output you like from that instrumentation (tracing, metrics, logs, anything that your users can come up with) without modifying the instrumentation itself.
The usage of it is similar to the Tracing API so what you already instrumented should be relatively easy to migrate but it's much more powerful than that and if you register tracing, it will call the Micrometer Tracing API to create spans. The whole Spring Portfolio is instrumented using this API including most of the popular frameworks/libraries.
If you want to learn more, here's a talk here we explain some basics: https://www.youtube.com/watch?v=Qyku6cR6ADY (starts at 14:00 but I recommend watching the whole talk).

Regarding to the Tracing API usage, I think the most important thing I would like to mention is that it is a Tracing Facade. It is what SLF4J is for loggers. As SLF4J can delegate to logging libraries, Micrometer Tracing can delegate to tracing libraries (Brave and OpenTelemetry right now but custom ones can be implemented by users). Because of this, it's usually not needed to pull another facade in front of it (just like it is usually not needed for SLF4J).

Also, the Tracing and Observation APIs provide the features that are implemented here: NOOP, unit test support (spans in-memory), integration test support (with Brave/Zipkin, OTel/OTLP, ...), etc.

The Observation API also has a nice feature that the Tracing API does not and it seems you could benefit from that (based on the Tags class and span names): it has a component called ObservationConvention which lets the MongoDB team define their own convention(s) but the users can define theirs too and decide which one they want to use. So users can use the MongoDB team's convention (default), the OpenTelemetry naming convention, or their company's convention (regardless of which Tracing library is under the hood). You can see this in action in the video I linked above.

@jyemin
Copy link
Collaborator

jyemin commented Aug 20, 2025

Thanks, @jonatan-ivanov. That's very useful context.

Seems like @mp911de commit to the Redis driver is close to what we would need to do here should we choose this route? Then configuration would be similar to this.

@mp911de
Copy link

mp911de commented Aug 21, 2025

Agree with #1695 (comment). I think this is the best way forward to get the most flexibility.

@nhachicha nhachicha requested a review from rozza October 8, 2025 14:33
@jonatan-ivanov
Copy link

@jyemin Sorry for the late response.
I think so except I would check if you can use the Observation API directly without directly depending on a tracer.

@mp911de
Copy link

mp911de commented Oct 9, 2025

FTR, you might want to ensure that your implementation isn't prone to the bug we fixed at spring-projects/spring-data-mongodb#5067

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants