From 24486c35b5685d56a5da6fcea2dab0576f8e5244 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 2 Mar 2021 19:25:01 +0100 Subject: [PATCH] Use low cardinality span names in Couchbase instrumentation (#2449) Also move unit tests to a separate module, previously they passed purely by accident (necessary classes were injected by the javaagent) --- .../api/tracer/DatabaseClientTracer.java | 13 +++++++++++++ .../couchbase-2.0-javaagent-unittests.gradle | 10 ++++++++++ .../v2_0/CouchbaseQuerySanitizerTest.groovy | 2 +- .../couchbase/v2_0/CouchbaseOnSubscribe.java | 17 ++++++++++++----- .../couchbase/v2_0/CouchbaseQuerySanitizer.java | 11 ++++++----- .../groovy/CouchbaseAsyncClient26Test.groovy | 4 ++-- .../test/groovy/CouchbaseClient26Test.groovy | 4 ++-- .../src/test/groovy/CouchbaseSpanUtil.groovy | 6 +++--- .../CouchbaseSpringRepository26Test.groovy | 4 ++-- .../CouchbaseSpringTemplate26Test.groovy | 4 ++-- .../AbstractCouchbaseAsyncClientTest.groovy | 4 +++- .../groovy/AbstractCouchbaseClientTest.groovy | 3 ++- ...AbstractCouchbaseSpringRepositoryTest.groovy | 11 +++++++---- .../groovy/util/AbstractCouchbaseTest.groovy | 4 ++-- settings.gradle | 1 + 15 files changed, 68 insertions(+), 30 deletions(-) create mode 100644 instrumentation/couchbase/couchbase-2.0/javaagent-unittests/couchbase-2.0-javaagent-unittests.gradle rename instrumentation/couchbase/couchbase-2.0/{javaagent => javaagent-unittests}/src/test/groovy/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizerTest.groovy (95%) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/DatabaseClientTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/DatabaseClientTracer.java index fc25f8ff30..802b3956ff 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/DatabaseClientTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/DatabaseClientTracer.java @@ -69,6 +69,19 @@ public abstract class DatabaseClientTracer }. If {@code dbName} and {@code operation} are not + * provided then {@code defaultValue} is returned. + */ + public static String conventionSpanName( + @Nullable String dbName, + @Nullable String operation, + @Nullable String table, + String defaultValue) { if (operation == null) { return dbName == null ? DB_QUERY : dbName; } diff --git a/instrumentation/couchbase/couchbase-2.0/javaagent-unittests/couchbase-2.0-javaagent-unittests.gradle b/instrumentation/couchbase/couchbase-2.0/javaagent-unittests/couchbase-2.0-javaagent-unittests.gradle new file mode 100644 index 0000000000..0502a3b75a --- /dev/null +++ b/instrumentation/couchbase/couchbase-2.0/javaagent-unittests/couchbase-2.0-javaagent-unittests.gradle @@ -0,0 +1,10 @@ +apply from: "$rootDir/gradle/java.gradle" + +dependencies { + testImplementation deps.groovy + testImplementation deps.spock + + testImplementation project(':javaagent-api') + testImplementation project(':instrumentation:couchbase:couchbase-2.0:javaagent') + testImplementation group: 'com.couchbase.client', name: 'java-client', version: '2.5.0' +} diff --git a/instrumentation/couchbase/couchbase-2.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizerTest.groovy b/instrumentation/couchbase/couchbase-2.0/javaagent-unittests/src/test/groovy/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizerTest.groovy similarity index 95% rename from instrumentation/couchbase/couchbase-2.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizerTest.groovy rename to instrumentation/couchbase/couchbase-2.0/javaagent-unittests/src/test/groovy/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizerTest.groovy index 808bb14158..ec3bcd9ab0 100644 --- a/instrumentation/couchbase/couchbase-2.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizerTest.groovy +++ b/instrumentation/couchbase/couchbase-2.0/javaagent-unittests/src/test/groovy/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizerTest.groovy @@ -18,7 +18,7 @@ class CouchbaseQuerySanitizerTest extends Specification { @Unroll def "should normalize #desc query"() { when: - def normalized = CouchbaseQuerySanitizer.sanitize(query) + def normalized = CouchbaseQuerySanitizer.sanitize(query).getFullStatement() then: // the analytics query ends up with trailing ';' in earlier couchbase version, but no trailing ';' in later couchbase version diff --git a/instrumentation/couchbase/couchbase-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseOnSubscribe.java b/instrumentation/couchbase/couchbase-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseOnSubscribe.java index dd8cc34f00..218339cdb9 100644 --- a/instrumentation/couchbase/couchbase-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseOnSubscribe.java +++ b/instrumentation/couchbase/couchbase-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseOnSubscribe.java @@ -6,10 +6,12 @@ package io.opentelemetry.javaagent.instrumentation.couchbase.v2_0; import static io.opentelemetry.api.trace.SpanKind.CLIENT; +import static io.opentelemetry.instrumentation.api.tracer.DatabaseClientTracer.conventionSpanName; import static io.opentelemetry.javaagent.instrumentation.couchbase.v2_0.CouchbaseClientTracer.tracer; import io.opentelemetry.api.trace.Span; import io.opentelemetry.instrumentation.rxjava.TracedOnSubscribe; +import io.opentelemetry.javaagent.instrumentation.api.db.SqlStatementInfo; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes.DbSystemValues; import java.lang.reflect.Method; @@ -24,18 +26,23 @@ public class CouchbaseOnSubscribe extends TracedOnSubscribe { Class declaringClass = method.getDeclaringClass(); String className = declaringClass.getSimpleName().replace("CouchbaseAsync", "").replace("DefaultAsync", ""); - return new CouchbaseOnSubscribe<>( - originalObservable, bucket, className + "." + method.getName()); + String operation = className + "." + method.getName(); + return new CouchbaseOnSubscribe<>(originalObservable, operation, bucket, operation); } public static CouchbaseOnSubscribe create( Observable originalObservable, String bucket, Object query) { + SqlStatementInfo statement = CouchbaseQuerySanitizer.sanitize(query); + String spanName = + conventionSpanName( + bucket, statement.getOperation(), statement.getTable(), statement.getFullStatement()); return new CouchbaseOnSubscribe<>( - originalObservable, bucket, CouchbaseQuerySanitizer.sanitize(query)); + originalObservable, spanName, bucket, statement.getFullStatement()); } - private CouchbaseOnSubscribe(Observable originalObservable, String bucket, String query) { - super(originalObservable, query, tracer(), CLIENT); + private CouchbaseOnSubscribe( + Observable originalObservable, String spanName, String bucket, String query) { + super(originalObservable, spanName, tracer(), CLIENT); this.bucket = bucket; this.query = query; diff --git a/instrumentation/couchbase/couchbase-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizer.java b/instrumentation/couchbase/couchbase-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizer.java index 9a53ddbadd..fc032d8d68 100644 --- a/instrumentation/couchbase/couchbase-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizer.java +++ b/instrumentation/couchbase/couchbase-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v2_0/CouchbaseQuerySanitizer.java @@ -5,6 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.couchbase.v2_0; +import io.opentelemetry.javaagent.instrumentation.api.db.SqlStatementInfo; import io.opentelemetry.javaagent.instrumentation.api.db.SqlStatementSanitizer; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; @@ -68,7 +69,7 @@ public final class CouchbaseQuerySanitizer { ANALYTICS_GET_STATEMENT = analyticsGetStatement; } - public static String sanitize(Object query) { + public static SqlStatementInfo sanitize(Object query) { if (query instanceof String) { return sanitizeString((String) query); } @@ -82,7 +83,7 @@ public final class CouchbaseQuerySanitizer { String queryClassName = query.getClass().getName(); if (queryClassName.equals("com.couchbase.client.java.view.ViewQuery") || queryClassName.equals("com.couchbase.client.java.view.SpatialViewQuery")) { - return query.toString(); + return new SqlStatementInfo(query.toString(), null, null); } // N1qlQuery is present starting from Couchbase 2.2.0 if (N1QL_QUERY_CLASS != null && N1QL_QUERY_CLASS.isAssignableFrom(query.getClass())) { @@ -98,7 +99,7 @@ public final class CouchbaseQuerySanitizer { return sanitizeString(statement); } } - return query.getClass().getSimpleName(); + return new SqlStatementInfo(query.getClass().getSimpleName(), null, null); } private static String getStatementString(MethodHandle handle, Object query) { @@ -112,8 +113,8 @@ public final class CouchbaseQuerySanitizer { } } - private static String sanitizeString(String query) { - return SqlStatementSanitizer.sanitize(query).getFullStatement(); + private static SqlStatementInfo sanitizeString(String query) { + return SqlStatementSanitizer.sanitize(query); } private CouchbaseQuerySanitizer() {} diff --git a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseAsyncClient26Test.groovy b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseAsyncClient26Test.groovy index f8e7b89158..182bf07593 100644 --- a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseAsyncClient26Test.groovy +++ b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseAsyncClient26Test.groovy @@ -8,7 +8,7 @@ import io.opentelemetry.instrumentation.test.asserts.TraceAssert class CouchbaseAsyncClient26Test extends AbstractCouchbaseAsyncClientTest { @Override - void assertCouchbaseCall(TraceAssert trace, int index, Object name, String bucketName = null, Object parentSpan = null) { - CouchbaseSpanUtil.assertCouchbaseCall(trace, index, name, bucketName, parentSpan) + void assertCouchbaseCall(TraceAssert trace, int index, Object name, String bucketName = null, Object parentSpan = null, Object statement = null) { + CouchbaseSpanUtil.assertCouchbaseCall(trace, index, name, bucketName, parentSpan, statement) } } diff --git a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseClient26Test.groovy b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseClient26Test.groovy index c9e86e1a96..2baea95d7d 100644 --- a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseClient26Test.groovy +++ b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseClient26Test.groovy @@ -7,7 +7,7 @@ import io.opentelemetry.instrumentation.test.asserts.TraceAssert class CouchbaseClient26Test extends AbstractCouchbaseClientTest { @Override - void assertCouchbaseCall(TraceAssert trace, int index, Object name, String bucketName = null, Object parentSpan = null) { - CouchbaseSpanUtil.assertCouchbaseCall(trace, index, name, bucketName, parentSpan) + void assertCouchbaseCall(TraceAssert trace, int index, Object name, String bucketName = null, Object parentSpan = null, Object statement = null) { + CouchbaseSpanUtil.assertCouchbaseCall(trace, index, name, bucketName, parentSpan, statement) } } diff --git a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseSpanUtil.groovy b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseSpanUtil.groovy index 3592b576ab..e8ee78ae9a 100644 --- a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseSpanUtil.groovy +++ b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/CouchbaseSpanUtil.groovy @@ -5,14 +5,14 @@ import static io.opentelemetry.api.trace.SpanKind.CLIENT -import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import io.opentelemetry.instrumentation.test.asserts.TraceAssert import io.opentelemetry.sdk.trace.data.SpanData +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes class CouchbaseSpanUtil { // Reusable span assertion method. Cannot directly override AbstractCouchbaseTest.assertCouchbaseSpan because // Of the class hierarchy of these tests - static void assertCouchbaseCall(TraceAssert trace, int index, Object spanName, String bucketName = null, Object parentSpan = null) { + static void assertCouchbaseCall(TraceAssert trace, int index, Object spanName, String bucketName = null, Object parentSpan = null, Object statement = null) { trace.span(index) { name spanName kind CLIENT @@ -41,7 +41,7 @@ class CouchbaseSpanUtil { // that do have operation ids "couchbase.operation_id" { it == null || String } - "${SemanticAttributes.DB_STATEMENT.key}" spanName + "${SemanticAttributes.DB_STATEMENT.key}" (statement ?: spanName) } } } diff --git a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/springdata/CouchbaseSpringRepository26Test.groovy b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/springdata/CouchbaseSpringRepository26Test.groovy index 3807862877..62b5596809 100644 --- a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/springdata/CouchbaseSpringRepository26Test.groovy +++ b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/springdata/CouchbaseSpringRepository26Test.groovy @@ -10,7 +10,7 @@ import io.opentelemetry.instrumentation.test.asserts.TraceAssert class CouchbaseSpringRepository26Test extends AbstractCouchbaseSpringRepositoryTest { @Override - void assertCouchbaseCall(TraceAssert trace, int index, Object name, String bucketName = null, Object parentSpan = null) { - CouchbaseSpanUtil.assertCouchbaseCall(trace, index, name, bucketName, parentSpan) + void assertCouchbaseCall(TraceAssert trace, int index, Object name, String bucketName = null, Object parentSpan = null, Object statement = null) { + CouchbaseSpanUtil.assertCouchbaseCall(trace, index, name, bucketName, parentSpan, statement) } } diff --git a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/springdata/CouchbaseSpringTemplate26Test.groovy b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/springdata/CouchbaseSpringTemplate26Test.groovy index 409aad36a9..80a12e9ab8 100644 --- a/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/springdata/CouchbaseSpringTemplate26Test.groovy +++ b/instrumentation/couchbase/couchbase-2.6/javaagent/src/test/groovy/springdata/CouchbaseSpringTemplate26Test.groovy @@ -9,7 +9,7 @@ import io.opentelemetry.instrumentation.test.asserts.TraceAssert class CouchbaseSpringTemplate26Test extends AbstractCouchbaseSpringTemplateTest { @Override - void assertCouchbaseCall(TraceAssert trace, int index, Object name, String bucketName = null, Object parentSpan = null) { - CouchbaseSpanUtil.assertCouchbaseCall(trace, index, name, bucketName, parentSpan) + void assertCouchbaseCall(TraceAssert trace, int index, Object name, String bucketName = null, Object parentSpan = null, Object statement = null) { + CouchbaseSpanUtil.assertCouchbaseCall(trace, index, name, bucketName, parentSpan, statement) } } diff --git a/instrumentation/couchbase/couchbase-testing/src/main/groovy/AbstractCouchbaseAsyncClientTest.groovy b/instrumentation/couchbase/couchbase-testing/src/main/groovy/AbstractCouchbaseAsyncClientTest.groovy index 82dc38c785..d0f90dd8fb 100644 --- a/instrumentation/couchbase/couchbase-testing/src/main/groovy/AbstractCouchbaseAsyncClientTest.groovy +++ b/instrumentation/couchbase/couchbase-testing/src/main/groovy/AbstractCouchbaseAsyncClientTest.groovy @@ -163,7 +163,9 @@ abstract class AbstractCouchbaseAsyncClientTest extends AbstractCouchbaseTest { basicSpan(it, 0, "someTrace") assertCouchbaseCall(it, 1, "Cluster.openBucket", null, span(0)) - assertCouchbaseCall(it, 2, 'SELECT mockrow', bucketCouchbase.name(), span(1)) + + def dbName = bucketCouchbase.name() + assertCouchbaseCall(it, 2, "SELECT $dbName", dbName, span(1), 'SELECT mockrow') } } diff --git a/instrumentation/couchbase/couchbase-testing/src/main/groovy/AbstractCouchbaseClientTest.groovy b/instrumentation/couchbase/couchbase-testing/src/main/groovy/AbstractCouchbaseClientTest.groovy index 63978f064f..54b551cd32 100644 --- a/instrumentation/couchbase/couchbase-testing/src/main/groovy/AbstractCouchbaseClientTest.groovy +++ b/instrumentation/couchbase/couchbase-testing/src/main/groovy/AbstractCouchbaseClientTest.groovy @@ -109,7 +109,8 @@ abstract class AbstractCouchbaseClientTest extends AbstractCouchbaseTest { assertCouchbaseCall(it, 0, "Cluster.openBucket") } trace(1, 1) { - assertCouchbaseCall(it, 0, 'SELECT mockrow', bucketCouchbase.name()) + def dbName = bucketCouchbase.name() + assertCouchbaseCall(it, 0, "SELECT $dbName", dbName, null, 'SELECT mockrow') } } diff --git a/instrumentation/couchbase/couchbase-testing/src/main/groovy/springdata/AbstractCouchbaseSpringRepositoryTest.groovy b/instrumentation/couchbase/couchbase-testing/src/main/groovy/springdata/AbstractCouchbaseSpringRepositoryTest.groovy index 60de3919a8..b1e1a800fd 100644 --- a/instrumentation/couchbase/couchbase-testing/src/main/groovy/springdata/AbstractCouchbaseSpringRepositoryTest.groovy +++ b/instrumentation/couchbase/couchbase-testing/src/main/groovy/springdata/AbstractCouchbaseSpringRepositoryTest.groovy @@ -82,7 +82,8 @@ abstract class AbstractCouchbaseSpringRepositoryTest extends AbstractCouchbaseTe and: assertTraces(1) { trace(0, 1) { - assertCouchbaseCall(it, 0, ~/^ViewQuery\(doc\/all\).*/, bucketCouchbase.name()) + def dbName = bucketCouchbase.name() + assertCouchbaseCall(it, 0, dbName, dbName, null, ~/^ViewQuery\(doc\/all\).*/) } } } @@ -179,9 +180,11 @@ abstract class AbstractCouchbaseSpringRepositoryTest extends AbstractCouchbaseTe assertTraces(1) { trace(0, 4) { basicSpan(it, 0, "someTrace") - assertCouchbaseCall(it, 1, "Bucket.upsert", bucketCouchbase.name(), span(0)) - assertCouchbaseCall(it, 2, "Bucket.remove", bucketCouchbase.name(), span(0)) - assertCouchbaseCall(it, 3, ~/^ViewQuery\(doc\/all\).*/, bucketCouchbase.name(), span(0)) + + def dbName = bucketCouchbase.name() + assertCouchbaseCall(it, 1, "Bucket.upsert", dbName, span(0)) + assertCouchbaseCall(it, 2, "Bucket.remove", dbName, span(0)) + assertCouchbaseCall(it, 3, dbName, dbName, span(0), ~/^ViewQuery\(doc\/all\).*/) } } } diff --git a/instrumentation/couchbase/couchbase-testing/src/main/groovy/util/AbstractCouchbaseTest.groovy b/instrumentation/couchbase/couchbase-testing/src/main/groovy/util/AbstractCouchbaseTest.groovy index 2e96fec91f..6fca2f3d06 100644 --- a/instrumentation/couchbase/couchbase-testing/src/main/groovy/util/AbstractCouchbaseTest.groovy +++ b/instrumentation/couchbase/couchbase-testing/src/main/groovy/util/AbstractCouchbaseTest.groovy @@ -103,7 +103,7 @@ abstract class AbstractCouchbaseTest extends AgentInstrumentationSpecification { .socketConnectTimeout(timeout.intValue()) } - void assertCouchbaseCall(TraceAssert trace, int index, Object spanName, String bucketName = null, Object parentSpan = null) { + void assertCouchbaseCall(TraceAssert trace, int index, Object spanName, String bucketName = null, Object parentSpan = null, Object statement = null) { trace.span(index) { name spanName kind CLIENT @@ -118,7 +118,7 @@ abstract class AbstractCouchbaseTest extends AgentInstrumentationSpecification { if (bucketName != null) { "${SemanticAttributes.DB_NAME.key}" bucketName } - "${SemanticAttributes.DB_STATEMENT.key}" spanName + "${SemanticAttributes.DB_STATEMENT.key}" (statement ?: spanName) } } } diff --git a/settings.gradle b/settings.gradle index 073923f76b..0910f5609e 100644 --- a/settings.gradle +++ b/settings.gradle @@ -82,6 +82,7 @@ include ':instrumentation:classloaders:javaagent:jboss-testing' include ':instrumentation:classloaders:javaagent:osgi-testing' include ':instrumentation:classloaders:javaagent:tomcat-testing' include ':instrumentation:couchbase:couchbase-2.0:javaagent' +include ':instrumentation:couchbase:couchbase-2.0:javaagent-unittests' include ':instrumentation:couchbase:couchbase-2.6:javaagent' include ':instrumentation:couchbase:couchbase-testing' include ':instrumentation:dropwizard-views-0.7:javaagent'