cleanup

Signed-off-by: Adrian Cole <[email protected]>
openzipkin · Apr 10, 2024 · 018f6b6 · 018f6b6
1 parent 34e84d4
commit 018f6b6
Show file tree

Hide file tree

Showing 8 changed files with 108 additions and 43 deletions.
diff --git a/instrumentation/mongodb/RATIONALE.md b/instrumentation/mongodb/RATIONALE.md
@@ -1,5 +1,13 @@
 # brave-instrumentation-mongodb rationale
 
+## Floor JRE version
+
+MongoDB client 3.x has a floor JRE version of 1.6, while 4.x moved to 8. We
+test on MongoDB client 3.x, despite it being 1.6 bytecode. This is a
+maintenance compromise beginning with MongoDB 5.x. We believe this will be less
+significant than more widely used libraries, such as Servlet or OkHttp, which
+could be used in old environments or Android.
+
 ## Default data policy
 We tried to make the default data policy similar to other instrumentation, such as MySQL, and also current practice
 from existing sites. Like other instrumentation, the policy is intentionally conservative, in efforts to avoid large

diff --git a/instrumentation/mongodb/pom.xml b/instrumentation/mongodb/pom.xml
@@ -32,7 +32,7 @@
     <main.basedir>${project.basedir}/../..</main.basedir>
 
     <mongodb-driver.version>5.0.1</mongodb-driver.version>
-    <old-mongodb-driver.version>3.11.0</old-mongodb-driver.version>
+    <floor-mongodb-driver.version>3.11.0</floor-mongodb-driver.version>
   </properties>
 
   <dependencies>
@@ -84,7 +84,7 @@
             <DynamicDependency>
               <groupId>org.mongodb</groupId>
               <artifactId>mongodb-driver</artifactId>
-              <version>${old-mongodb-driver.version}</version>
+              <version>${floor-mongodb-driver.version}</version>
               <repositoryType>MAIN</repositoryType>
               <type>jar</type>
             </DynamicDependency>
@@ -98,10 +98,10 @@
     <profile>
       <id>release</id>
       <properties>
-        <!-- mongodb-driver 3.x requires Java 6 -->
-        <maven.compiler.source>1.6</maven.compiler.source>
-        <maven.compiler.target>1.6</maven.compiler.target>
-        <maven.compiler.release>6</maven.compiler.release>
+        <!-- mongodb-driver 3.x requires Java 6, but we only support 7 -->
+        <maven.compiler.source>1.7</maven.compiler.source>
+        <maven.compiler.target>1.7</maven.compiler.target>
+        <maven.compiler.release>7</maven.compiler.release>
       </properties>
       <build>
         <plugins>

diff --git a/...ation/mongodb/src/it/mongodb_v3/README.md → ...on/mongodb/src/it/mongodb_floor/README.md b/...ation/mongodb/src/it/mongodb_v3/README.md → ...on/mongodb/src/it/mongodb_floor/README.md
@@ -1,2 +1,2 @@
-# mongodb_v3
+# mongodb_floor
 This tests that MongoDBTracing can be used with mongodb <5
diff --git a/...ntation/mongodb/src/it/mongodb_v3/pom.xml → ...tion/mongodb/src/it/mongodb_floor/pom.xml b/...ntation/mongodb/src/it/mongodb_v3/pom.xml → ...tion/mongodb/src/it/mongodb_floor/pom.xml
@@ -20,9 +20,9 @@
   <modelVersion>4.0.0</modelVersion>
 
   <groupId>@project.groupId@</groupId>
-  <artifactId>mongodb_v3</artifactId>
+  <artifactId>mongodb_floor</artifactId>
   <version>@project.version@</version>
-  <name>mongodb_v3</name>
+  <name>mongodb_floor</name>
 
   <properties>
     <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
@@ -36,7 +36,7 @@
     <dependency>
       <groupId>org.mongodb</groupId>
       <artifactId>mongodb-driver</artifactId>
-      <version>@old-mongodb-driver.version@</version>
+      <version>@floor-mongodb-driver.version@</version>
       <scope>provided</scope>
     </dependency>
 

diff --git a/...va/brave/mongodb_v3/ITMongoDBTracing.java → ...va/brave/mongodb_v3/ITMongoDBTracing.java b/...va/brave/mongodb_v3/ITMongoDBTracing.java → ...va/brave/mongodb_v3/ITMongoDBTracing.java
@@ -11,7 +11,7 @@
  * or implied. See the License for the specific language governing permissions and limitations under
  * the License.
  */
-package brave.mongodb_v3;
+package brave.mongodb_floor;
 
 class ITMongoDBTracing extends brave.mongodb.ITMongoDBTracing {
 }
diff --git a/instrumentation/mongodb/src/main/java/brave/mongodb/MongoDBDriver.java b/instrumentation/mongodb/src/main/java/brave/mongodb/MongoDBDriver.java
@@ -15,7 +15,9 @@
 
 import brave.Span;
 import com.mongodb.ServerAddress;
-import java.lang.reflect.Method;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
 import java.net.InetSocketAddress;
 
 import static brave.internal.Throwables.propagateIfFatal;
@@ -49,16 +51,21 @@ public static MongoDBDriver get() {
    * </ol>
    */
   private static MongoDBDriver findMongoDBDriver() {
+    MethodHandles.Lookup lookup = MethodHandles.lookup();
     try {
-      Method getHost = ServerAddress.class.getMethod("getHost");
-      Method getPort = ServerAddress.class.getMethod("getPort");
+      MethodHandle getHost =
+        lookup.findVirtual(ServerAddress.class, "getHost", MethodType.methodType(String.class));
+      MethodHandle getPort =
+        lookup.findVirtual(ServerAddress.class, "getPort", MethodType.methodType(int.class));
       return new Driver5x(getHost, getPort);
-    } catch (NoSuchMethodException e) {
+    } catch (NoSuchMethodException | IllegalAccessException e) {
       // not 5.x
     }
     try {
-      return new Driver3x(ServerAddress.class.getMethod("getSocketAddress"));
-    } catch (NoSuchMethodException e) {
+      MethodHandle getSocketAddress = lookup.findVirtual(ServerAddress.class, "getSocketAddress",
+        MethodType.methodType(InetSocketAddress.class));
+      return new Driver3x(getSocketAddress);
+    } catch (NoSuchMethodException | IllegalAccessException e) {
       // unknown version
     }
 
@@ -67,16 +74,16 @@ private static MongoDBDriver findMongoDBDriver() {
   }
 
   static final class Driver3x extends MongoDBDriver {
-    final Method getSocketAddress;
+    final MethodHandle getSocketAddress;
 
-    Driver3x(Method getSocketAddress) {
+    Driver3x(MethodHandle getSocketAddress) {
       this.getSocketAddress = getSocketAddress;
     }
 
     @Override void setRemoteIpAndPort(Span span, ServerAddress serverAddress) {
       try {
         InetSocketAddress socketAddress =
-          (InetSocketAddress) getSocketAddress.invoke(serverAddress);
+          (InetSocketAddress) getSocketAddress.invokeExact(serverAddress);
         span.remoteIpAndPort(socketAddress.getAddress().getHostAddress(), socketAddress.getPort());
       } catch (Throwable t) {
         propagateIfFatal(t);
@@ -85,17 +92,18 @@ static final class Driver3x extends MongoDBDriver {
   }
 
   static final class Driver5x extends MongoDBDriver {
-    final Method getHost, getPort;
+    final MethodHandle getHost, getPort;
 
-    Driver5x(Method getHost, Method getPort) {
+    Driver5x(MethodHandle getHost, MethodHandle getPort) {
       this.getHost = getHost;
       this.getPort = getPort;
     }
 
     @Override void setRemoteIpAndPort(Span span, ServerAddress serverAddress) {
       try {
-        span.remoteIpAndPort((String) getHost.invoke(serverAddress),
-          (int) getPort.invoke(serverAddress));
+        String host = (String) getHost.invokeExact(serverAddress);
+        int port = (int) getPort.invokeExact(serverAddress);
+        span.remoteIpAndPort(host, port);
       } catch (Throwable t) {
         propagateIfFatal(t);
       }

diff --git a/instrumentation/mongodb/src/test/java/brave/mongodb/ITMongoDBTracing.java b/instrumentation/mongodb/src/test/java/brave/mongodb/ITMongoDBTracing.java
@@ -28,7 +28,7 @@
 import com.mongodb.client.internal.MongoClientImpl;
 import com.mongodb.connection.ClusterId;
 import com.mongodb.event.CommandListener;
-import java.lang.reflect.Field;
+import java.lang.invoke.VarHandle;
 import org.bson.BsonDocument;
 import org.bson.BsonString;
 import org.bson.Document;
@@ -41,6 +41,9 @@
 import org.testcontainers.junit.jupiter.Testcontainers;
 
 import static brave.Span.Kind.CLIENT;
+import static java.lang.invoke.MethodHandles.Lookup;
+import static java.lang.invoke.MethodHandles.lookup;
+import static java.lang.invoke.MethodHandles.privateLookupIn;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.entry;
@@ -72,13 +75,13 @@ public class ITMongoDBTracing extends ITRemote { // public for invoker test
   }
 
   @BeforeEach void getClusterId() throws Exception {
-    // TODO: Figure out an easier way to get this!
-    Field clusterIdField =
-      Class.forName("com.mongodb.internal.connection.BaseCluster").getDeclaredField("clusterId");
-
-    clusterIdField.setAccessible(true);
-    ClusterId clusterId =
-      (ClusterId) clusterIdField.get(((MongoClientImpl) mongoClient).getCluster());
+    // Object because the type changed between 3.x and 5.x
+    Object cluster = ((MongoClientImpl) mongoClient).getCluster();
+    // Before 5.x, there is no public API to get the clusterId.
+    Class<?> clazz = Class.forName("com.mongodb.internal.connection.BaseCluster");
+    Lookup lookup = privateLookupIn(clazz, lookup());
+    VarHandle clusterIdHandle = lookup.findVarHandle(clazz, "clusterId", ClusterId.class);
+    ClusterId clusterId = (ClusterId) clusterIdHandle.get(cluster);
     this.clusterId = clusterId.getValue();
   }
 
@@ -108,11 +111,12 @@ public class ITMongoDBTracing extends ITRemote { // public for invoker test
     mongoCursor.next();
 
     // Name extracted from {"find": "myCollection"}
-    assertThat(testSpanHandler.takeRemoteSpan(CLIENT).name()).isEqualTo("find " + COLLECTION_NAME);
+    assertThat(testSpanHandler.takeRemoteSpan(CLIENT).name())
+      .isEqualTo("find " + COLLECTION_NAME);
 
     // Name extracted from {"getMore": <cursorId>, "collection": "myCollection"}
-    assertThat(testSpanHandler.takeRemoteSpan(CLIENT).name()).isEqualTo(
-      "getMore " + COLLECTION_NAME);
+    assertThat(testSpanHandler.takeRemoteSpan(CLIENT).name())
+      .isEqualTo("getMore " + COLLECTION_NAME);
   }
 
   /**
@@ -143,8 +147,10 @@ public class ITMongoDBTracing extends ITRemote { // public for invoker test
     executeFind(COLLECTION_NAME);
 
     assertThat(testSpanHandler.takeRemoteSpan(CLIENT).tags()).containsOnly(
-      entry("mongodb.collection", COLLECTION_NAME), entry("mongodb.command", "find"),
-      entry("mongodb.cluster_id", clusterId));
+      entry("mongodb.collection", COLLECTION_NAME),
+      entry("mongodb.command", "find"),
+      entry("mongodb.cluster_id", clusterId)
+    );
   }
 
   @Test void addsTagsForLargePayloadCommand() {
@@ -155,20 +161,24 @@ public class ITMongoDBTracing extends ITRemote { // public for invoker test
     database.getCollection("largeCollection").insertOne(largeDocument);
 
     assertThat(testSpanHandler.takeRemoteSpan(CLIENT).tags()).containsOnly(
-      entry("mongodb.collection", "largeCollection"), entry("mongodb.command", "insert"),
-      entry("mongodb.cluster_id", clusterId));
+      entry("mongodb.collection", "largeCollection"),
+      entry("mongodb.command", "insert"),
+      entry("mongodb.cluster_id", clusterId)
+    );
   }
 
   @Test void reportsServerAddress() {
     executeFind(COLLECTION_NAME);
 
-    assertThat(testSpanHandler.takeRemoteSpan(CLIENT).remoteServiceName()).isEqualTo(
-      "mongodb-" + DATABASE_NAME);
+    MutableSpan span = testSpanHandler.takeRemoteSpan(CLIENT);
+    assertThat(span.remoteServiceName()).isEqualTo("mongodb-" + DATABASE_NAME);
+    assertThat(span.remoteIp()).isEqualTo(mongo.host());
+    assertThat(span.remotePort()).isEqualTo(mongo.port());
   }
 
   @Test void setsError() {
-    assertThatThrownBy(() -> executeFind(INVALID_COLLECTION_NAME)).isInstanceOf(
-      MongoQueryException.class);
+    assertThatThrownBy(() -> executeFind(INVALID_COLLECTION_NAME))
+      .isInstanceOf(MongoQueryException.class);
 
     // the error the interceptor receives is a MongoCommandException!
     testSpanHandler.takeRemoteSpanWithErrorMessage(CLIENT, ".*InvalidNamespace.*");

diff --git a/instrumentation/mongodb/src/test/java/brave/mongodb/MongoDBDriverTest.java b/instrumentation/mongodb/src/test/java/brave/mongodb/MongoDBDriverTest.java
@@ -0,0 +1,39 @@
+/*
+ * Copyright 2013-2024 The OpenZipkin Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package brave.mongodb;
+
+import brave.Span;
+import com.mongodb.ServerAddress;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+public class MongoDBDriverTest {
+  @Mock ServerAddress serverAddress;
+  @Mock Span span;
+
+  @Test void setRemoteIpAndPort() {
+    when(serverAddress.getHost()).thenReturn("127.0.0.1");
+    when(serverAddress.getPort()).thenReturn(27017);
+
+    MongoDBDriver.get().setRemoteIpAndPort(span, serverAddress);
+
+    verify(span).remoteIpAndPort("127.0.0.1", 27017);
+  }
+}