MongoDB 4 driver instrumentation #2046

malafeev · 2021-01-14T16:18:40Z

Based on https://github.com/DataDog/dd-trace-java/tree/master/dd-java-agent/instrumentation/mongo/driver-4

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

jyemin · 2021-01-14T22:35:11Z

instrumentation/mongo/mongo-4/javaagent/mongo-4-javaagent.gradle

+  pass {
+    group = "org.mongodb"
+    module = "mongodb-driver-core"
+    // this instrumentation is backwards compatible with early versions of the new API that shipped in 3.7


I'm not sure if this complicates matters, but the upcoming 4.2.0 release will allow construction of a legacy com.mongodb.MongoClient with a com.mongodb.MongoClientSettings. See mongodb/mongo-java-driver@15b70cf#diff-9edd83ced1b6f12502db95a96af13ed57e647efd84d2ddf5d040af4a653fb1edR211 (you have to expand the MongoClient diffs as they are large)

anuraaga · 2021-01-15T06:06:32Z

...va/io/opentelemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java

+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    public static void injectTraceListener(
+        @Advice.This MongoClientSettings.Builder builder,
+        @Advice.FieldValue("commandListeners") List<CommandListener> commandListeners) {


We generally prefer to use public API instead of fields to reduce the chance that testLatestDeps will fail when implementation details change :)

it is consistent with all other mongo versions instrumentations, I updated matcher.

anuraaga · 2021-01-15T06:07:10Z

...va/io/opentelemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java

+              .getClass()
+              .getName()
+              .startsWith("io.opentelemetry.")) {
+        // we'll replace it, since it could be the 3.x async driver which is bundled with driver 4


So we'd want to filter out calls to the API to add listeners rather than filter here

removed that code

anuraaga · 2021-01-15T06:08:11Z

...aagent/src/main/java/io/opentelemetry/javaagent/instrumentation/mongo/MongoClientTracer.java

+          | IllegalAccessException
+          | InvocationTargetException
+          | NoSuchMethodException ignored) {
+      }


What happens if this is null, is it ok?

no, null is not ok, we will get NPE later. But null shouldn't happen because constructor with boolean exists prior version 4.0. And since 4.0 there is a builder only.

...va/io/opentelemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java

settings.gradle

...va/io/opentelemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java

trask · 2021-01-15T06:22:20Z

...va/io/opentelemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java

+      if (!commandListeners.isEmpty()
+          && commandListeners
+              .get(commandListeners.size() - 1)
+              .getClass()
+              .getName()
+              .startsWith("io.opentelemetry.")) {
+        // we'll replace it, since it could be the 3.x async driver which is bundled with driver 4
+        commandListeners.remove(commandListeners.size() - 1);
+      }


Is the code above needed? I think the code below will prevent double registration?

…elemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

…elemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

anuraaga · 2021-01-16T05:02:28Z

.../io/opentelemetry/javaagent/instrumentation/mongo/v4_0/MongoClientInstrumentationModule.java

+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    public static void injectTraceListener(
+        @Advice.This MongoClientSettings.Builder builder,
+        @Advice.FieldValue("commandListeners") List<CommandListener> commandListeners) {


Thanks for using the public method - for suppression we would want to instrument addCommandListener instead to avoid accessing this private field. However, we've generally been skipping handling duplicate registration in an effort to come with a more general pattern so you don't have to worry about it either if it's tedious.

malafeev · 2021-01-21T12:29:27Z

Is anything else should be done in this PR?
I ported instrumentation from DD and made it similar to mongo-3.7 instrumentation.

anuraaga · 2021-01-22T05:31:05Z

@malafeev Do you think you can address #2046 (comment)? We should avoid accessing the private field. I believe you changed to instrumenting addCommandListener but my intention was to keep instrumenting build, but call addCommandListener from inside instead of accessing the private variable. Currently we'll run our instrumentation every time a user executes addCommandListener which is weird.

malafeev · 2021-01-22T05:54:28Z

@anuraaga , no, as I understand, we intercept only build method:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/2046/files#diff-68a7a03ca69546307dd8eda75056b354fc429e9f87a5ec3602a3f9a4816d8294R64

    @Override
    public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
      return singletonMap(
          isMethod().and(isPublic()).and(named("build")).and(takesArguments(0)),
          MongoClientInstrumentationModule.class.getName() + "$MongoClientAdvice");
    }

anuraaga

@malafeev sorry, was reading the type matcher, not the transformer!

In other agents we aren't really doing any deduping, so we could possibly remove it here too, but I guess if it causes a problem with testLatestDeps we could do so later too.

iNikem · 2021-01-22T09:38:58Z

instrumentation/mongo/mongo-4.0/javaagent/mongo-4.0-javaagent.gradle

+    versions = "[3.7,)"
+    assertInverse = true


I think it would be cleaner to

Suggested change

versions = "[3.7,)"

assertInverse = true

versions = "[4.0,)"

@trask @mateuszrzeszutek WDYT?

but before 4.0 non-uber jars have also been published, that's why 3.7 is here

I don't follow, sorry. If this module is meant for mongo 4.0+, then why do we want to verify muzzle for 3.7?

there are 2 types of mongo jars:

uber jar. But it doesn't exist since version 4.0.

non-uber jar which exists since version 3.7 up to current version.

DD named module to support non-uber jar mongo-4.
Should I rename this one to mongo-non-uber-3.7? There is already mongo-3.7 module to support uber jars.

Wait, so actual instrumentations are the same, right? The only difference is, essentially, the collection of packages used for testing? So this is not a new module for instrumentation, but a new module for testing? Like glassfish-testing?

How does it differ from already existing io.opentelemetry.javaagent.instrumentation.mongo.v3_7.MongoClientInstrumentationModule? Instrumentation looks the same to me.

yes, instrumentation looks the same.
but mongo-3.7 will not work for mongodb-driver-core 3.7 jar because of muzzle.
mongo-3.7 depends on mongo-java-driver
mongo-4.0 depends on mongodb-driver-core

Instead of copying the 3.7 instrumentation, would it be possible to add another muzzle task there and just move the tests to a separate 4.0/uberjar testing module?

Adding

pass { group = "org.mongodb" module = "mongodb-driver-core" // this instrumentation is backwards compatible with early versions of the new API that shipped in 3.7 // the legacy API instrumented in mongo-3.1 continues to be shipped in 4.x, but doesn't conflict here // because they are triggered by different types: MongoClientSettings(new) vs MongoClientOptions(legacy) versions = "[3.7,)" extraDependency "org.mongodb:bson:3.7.0" assertInverse = true }

to the 3.7 muzzle task definition should cover all the uberjar releases.

@mateuszrzeszutek done in 5e5ea23

instrumentation/mongo/mongo-4.0/javaagent/mongo-4.0-javaagent.gradle

.../io/opentelemetry/javaagent/instrumentation/mongo/v4_0/MongoClientInstrumentationModule.java

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

MongoDB 4 driver instrumentation

da3d689

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

malafeev requested review from anuraaga, iNikem, jkwatson, mateuszrzeszutek, trask and tylerbenson as code owners January 14, 2021 16:18

malafeev mentioned this pull request Jan 14, 2021

Support non-uber jar releases of mongo-java-driver #1588

Closed

Sergei Malafeev added 2 commits January 15, 2021 00:59

fix getting constructor

8ae8673

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

fix formatting

226a5cb

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

jyemin reviewed Jan 14, 2021

View reviewed changes

anuraaga reviewed Jan 15, 2021

View reviewed changes

trask reviewed Jan 15, 2021

View reviewed changes

Sergei Malafeev and others added 5 commits January 15, 2021 16:55

Update instrumentation/mongo/mongo-4/javaagent/src/main/java/io/opent…

68fe769

…elemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update settings.gradle

c921fe1

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update instrumentation/mongo/mongo-4/javaagent/src/main/java/io/opent…

e49ffac

…elemetry/javaagent/instrumentation/mongo/v4/MongoClientInstrumentationModule.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

use mongo-4.0

a84f7d7

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

use public api

b7c6a87

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

anuraaga reviewed Jan 16, 2021

View reviewed changes

anuraaga approved these changes Jan 22, 2021

View reviewed changes

iNikem suggested changes Jan 22, 2021

View reviewed changes

use testImplementation

d6093e2

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

malafeev requested a review from pavolloffay as a code owner January 22, 2021 12:20

Sergei Malafeev added 3 commits January 22, 2021 20:48

Merge branch 'master' into mongo-4

06d4423

use declaresField(named("commandListeners")) in typeMatcher

55a028f

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

migrate to mongo-4.0-testing

5e5ea23

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

iNikem approved these changes Jan 25, 2021

View reviewed changes

iNikem merged commit bbcbeb5 into open-telemetry:master Jan 25, 2021

trask mentioned this pull request Mar 31, 2021

No MongoDB spans when upgrading from 0.9.0 to 0.12.0 #1885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MongoDB 4 driver instrumentation #2046

MongoDB 4 driver instrumentation #2046

malafeev commented Jan 14, 2021 •

edited

Loading

jyemin Jan 14, 2021

anuraaga Jan 15, 2021

malafeev Jan 15, 2021

anuraaga Jan 15, 2021

malafeev Jan 15, 2021

anuraaga Jan 15, 2021

malafeev Jan 15, 2021

trask Jan 15, 2021

malafeev Jan 15, 2021

anuraaga Jan 16, 2021

malafeev commented Jan 21, 2021 •

edited

Loading

anuraaga commented Jan 22, 2021

malafeev commented Jan 22, 2021

anuraaga left a comment

iNikem Jan 22, 2021

malafeev Jan 22, 2021 •

edited

Loading

iNikem Jan 22, 2021

malafeev Jan 25, 2021 •

edited

Loading

iNikem Jan 25, 2021

iNikem Jan 25, 2021

malafeev Jan 25, 2021

mateuszrzeszutek Jan 25, 2021

mateuszrzeszutek Jan 25, 2021

malafeev Jan 25, 2021

MongoDB 4 driver instrumentation #2046

MongoDB 4 driver instrumentation #2046

Conversation

malafeev commented Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malafeev commented Jan 21, 2021 • edited Loading

anuraaga commented Jan 22, 2021

malafeev commented Jan 22, 2021

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malafeev Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malafeev Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malafeev commented Jan 14, 2021 •

edited

Loading

malafeev commented Jan 21, 2021 •

edited

Loading

malafeev Jan 22, 2021 •

edited

Loading

malafeev Jan 25, 2021 •

edited

Loading