From 63091c9350299bef2ed7623047fa1a8c2b7d8126 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 29 Aug 2018 09:39:09 +1000 Subject: [PATCH] Review fixes --- .../CouchbaseClientInstrumentation.java | 11 ++- .../test/groovy/CouchbaseClientTest.groovy | 2 +- .../groovy/springdata/CouchbaseConfig.groovy | 6 +- .../CouchbaseSpringRepositoryTest.groovy | 2 +- .../CouchbaseSpringTemplateTest.groovy | 6 +- .../groovy/util/AbstractCouchbaseTest.groovy | 92 +++++++++---------- .../trace/agent/test/AgentTestRunner.java | 3 +- 7 files changed, 61 insertions(+), 61 deletions(-) diff --git a/dd-java-agent/instrumentation/couchbase-2.0/src/main/java/datadog/trace/instrumentation/couchbase/client/CouchbaseClientInstrumentation.java b/dd-java-agent/instrumentation/couchbase-2.0/src/main/java/datadog/trace/instrumentation/couchbase/client/CouchbaseClientInstrumentation.java index dc18106dae..8ce69ae5b0 100644 --- a/dd-java-agent/instrumentation/couchbase-2.0/src/main/java/datadog/trace/instrumentation/couchbase/client/CouchbaseClientInstrumentation.java +++ b/dd-java-agent/instrumentation/couchbase-2.0/src/main/java/datadog/trace/instrumentation/couchbase/client/CouchbaseClientInstrumentation.java @@ -15,6 +15,7 @@ import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.api.DDTags; import datadog.trace.bootstrap.CallDepthThreadLocalMap; import io.opentracing.Span; +import io.opentracing.noop.NoopSpan; import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; import java.lang.reflect.Method; @@ -35,11 +36,6 @@ public class CouchbaseClientInstrumentation extends Instrumenter.Default { super("couchbase"); } - @Override - protected boolean defaultEnabled() { - return false; - } - @Override public ElementMatcher typeMatcher() { return not(isInterface()) @@ -103,11 +99,16 @@ public class CouchbaseClientInstrumentation extends Instrumenter.Default { @Override public void call() { + // This is called each time an observer has a new subscriber, but we should only time it once. + if (!spanRef.compareAndSet(null, NoopSpan.INSTANCE)) { + return; + } final Class declaringClass = method.getDeclaringClass(); final String className = declaringClass.getSimpleName().replace("CouchbaseAsync", "").replace("DefaultAsync", ""); final String resourceName = className + "." + method.getName(); + // just replace the no-op span. spanRef.set( GlobalTracer.get() .buildSpan("couchbase.call") diff --git a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/CouchbaseClientTest.groovy b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/CouchbaseClientTest.groovy index 6246a87b2d..ad57b22561 100644 --- a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/CouchbaseClientTest.groovy +++ b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/CouchbaseClientTest.groovy @@ -52,7 +52,7 @@ class CouchbaseClientTest extends AbstractCouchbaseTest { } where: - bucketSettings << [BUCKET_COUCHBASE, BUCKET_MEMCACHE, BUCKET_EPHEMERAL] + bucketSettings << [bucketCouchbase, bucketMemcache, bucketEphemeral] type = bucketSettings.type().name() } } diff --git a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseConfig.groovy b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseConfig.groovy index a0b7ffdd6f..9ab5794827 100644 --- a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseConfig.groovy +++ b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseConfig.groovy @@ -1,11 +1,9 @@ package springdata - import org.springframework.context.annotation.ComponentScan import org.springframework.context.annotation.Configuration import org.springframework.data.couchbase.config.AbstractCouchbaseConfiguration import org.springframework.data.couchbase.repository.config.EnableCouchbaseRepositories -import util.AbstractCouchbaseTest @Configuration @EnableCouchbaseRepositories(basePackages = "springdata") @@ -19,11 +17,11 @@ class CouchbaseConfig extends AbstractCouchbaseConfiguration { @Override protected String getBucketName() { - return AbstractCouchbaseTest.BUCKET_COUCHBASE.name() + return "CouchbaseSpringRepositoryTest-cb" } @Override protected String getBucketPassword() { - return AbstractCouchbaseTest.BUCKET_COUCHBASE.password() + return "test-pass" } } diff --git a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseSpringRepositoryTest.groovy b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseSpringRepositoryTest.groovy index 8231f86a9a..ac753864de 100644 --- a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseSpringRepositoryTest.groovy +++ b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseSpringRepositoryTest.groovy @@ -38,7 +38,7 @@ class CouchbaseSpringRepositoryTest extends AbstractCouchbaseTest { def setup() { repo.deleteAll() -// TEST_WRITER.waitForTraces(4) + TEST_WRITER.waitForTraces(1) // There might be more if there were documents to delete TEST_WRITER.clear() } diff --git a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseSpringTemplateTest.groovy b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseSpringTemplateTest.groovy index 64fa661dfe..a4059103a5 100644 --- a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseSpringTemplateTest.groovy +++ b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/springdata/CouchbaseSpringTemplateTest.groovy @@ -13,9 +13,9 @@ class CouchbaseSpringTemplateTest extends AbstractCouchbaseTest { List templates def setupSpec() { - Bucket bucketCouchbase = cluster.openBucket(BUCKET_COUCHBASE.name()) - Bucket bucketMemcache = cluster.openBucket(BUCKET_MEMCACHE.name()) - Bucket bucketEphemeral = cluster.openBucket(BUCKET_EPHEMERAL.name()) + Bucket bucketCouchbase = cluster.openBucket(bucketCouchbase.name()) + Bucket bucketMemcache = cluster.openBucket(bucketMemcache.name()) + Bucket bucketEphemeral = cluster.openBucket(bucketEphemeral.name()) def info = manager.info() templates = [new CouchbaseTemplate(info, bucketCouchbase), diff --git a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/util/AbstractCouchbaseTest.groovy b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/util/AbstractCouchbaseTest.groovy index 118f3a90fe..d94d6457d9 100644 --- a/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/util/AbstractCouchbaseTest.groovy +++ b/dd-java-agent/instrumentation/couchbase-2.0/src/test/groovy/util/AbstractCouchbaseTest.groovy @@ -22,6 +22,11 @@ import com.couchbase.client.java.query.Index import com.couchbase.client.java.view.DefaultView import com.couchbase.client.java.view.DesignDocument import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.utils.OkHttpUtils +import okhttp3.FormBody +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.RequestBody import org.testcontainers.couchbase.CouchbaseContainer import spock.lang.Requires import spock.lang.Shared @@ -33,31 +38,35 @@ import java.util.concurrent.TimeUnit @Requires({ "true" == System.getenv("CI") || jvm.java8Compatible }) class AbstractCouchbaseTest extends AgentTestRunner { - static { - System.setProperty("dd.integration.couchbase.enabled", "true") - } private static final USERNAME = "Administrator" private static final PASSWORD = "password" + private static final OkHttpClient HTTP_CLIENT = OkHttpUtils.client() - static final BUCKET_COUCHBASE = DefaultBucketSettings.builder() + @Shared + private String testBucketName = this.getClass().simpleName + + @Shared + protected bucketCouchbase = DefaultBucketSettings.builder() .enableFlush(true) - .name("test-bucket-cb") + .name("$testBucketName-cb") .password("test-pass") .type(BucketType.COUCHBASE) .quota(100) .build() - static final BUCKET_MEMCACHE = DefaultBucketSettings.builder() + @Shared + protected bucketMemcache = DefaultBucketSettings.builder() .enableFlush(true) - .name("test-bucket-mem") + .name("$testBucketName-mem") .password("test-pass") .type(BucketType.MEMCACHED) .quota(100) .build() - static final BUCKET_EPHEMERAL = DefaultBucketSettings.builder() + @Shared + protected bucketEphemeral = DefaultBucketSettings.builder() .enableFlush(true) - .name("test-bucket-emp") + .name("$testBucketName-emp") .password("test-pass") .type(BucketType.EPHEMERAL) .quota(100) @@ -91,13 +100,15 @@ class AbstractCouchbaseTest extends AgentTestRunner { } else { initCluster() cluster = CouchbaseCluster.create(envBuilder().build()) - println "Using local couchbase" + println "Using provided couchbase" } manager = cluster.clusterManager(USERNAME, PASSWORD) - resetBucket(cluster, BUCKET_COUCHBASE) - resetBucket(cluster, BUCKET_MEMCACHE) - resetBucket(cluster, BUCKET_EPHEMERAL) + if (!testBucketName.contains(AbstractCouchbaseTest.simpleName)) { + resetBucket(cluster, bucketCouchbase) + resetBucket(cluster, bucketMemcache) + resetBucket(cluster, bucketEphemeral) + } } def cleanupSpec() { @@ -131,41 +142,30 @@ class AbstractCouchbaseTest extends AgentTestRunner { protected void initCluster() { - assert callCouchbaseRestAPI("/pools/default", "memoryQuota=600&indexMemoryQuota=300") == 200 + assert callCouchbaseRestAPI("/pools/default", "memoryQuota=1000&indexMemoryQuota=300") == 200 // This one fails if already initialized, so don't assert. - callCouchbaseRestAPI("/node/controller/setupServices", "services=kv%2Cn1ql%2Cindex%2Cfts") + callCouchbaseRestAPI("/node/controller/setupServices", "services=kv%2Cindex%2Cn1ql%2Cfts") assert callCouchbaseRestAPI("/settings/web", "username=$USERNAME&password=$PASSWORD&port=8091") == 200 -// callCouchbaseRestAPI(bucketURL, sampleBucketPayloadBuilder.toString()) assert callCouchbaseRestAPI("/settings/indexes", "indexerThreads=0&logLevel=info&maxRollbackPoints=5&storageMode=memory_optimized") == 200 } + /** + * Adapted from CouchbaseContainer.callCouchbaseRestAPI() + */ protected int callCouchbaseRestAPI(String url, String payload) throws IOException { - String fullUrl = "http://localhost:8091" + url - HttpURLConnection httpConnection = (HttpURLConnection) ((new URL(fullUrl).openConnection())) - try { - httpConnection.setDoOutput(true) - httpConnection.setRequestMethod("POST") - httpConnection.setRequestProperty("Content-Type", "application/x-www-form-urlencoded") - String encoded = Base64.encode((USERNAME + ":" + PASSWORD).getBytes("UTF-8")) - httpConnection.setRequestProperty("Authorization", "Basic " + encoded) - DataOutputStream out = new DataOutputStream(httpConnection.getOutputStream()) - try { - out.writeBytes(payload) - out.flush() - def code = httpConnection.getResponseCode() - return code - } finally { - if (Collections.singletonList(out).get(0) != null) { - out.close() - } - } - } finally { - if (Collections.singletonList(httpConnection).get(0) != null) { - httpConnection.disconnect() - } - } + String authToken = Base64.encode((USERNAME + ":" + PASSWORD).getBytes("UTF-8")) + def request = new Request.Builder() + .url("http://localhost:8091$url") + .header("Authorization", "Basic " + authToken) + .post(RequestBody.create(FormBody.CONTENT_TYPE, payload)) + .build() + def response = HTTP_CLIENT.newCall(request).execute() + return response.code() } + /** + * Copied from CouchbaseContainer.callCouchbaseRestAPI() + */ protected void resetBucket(CouchbaseCluster cluster, BucketSettings bucketSetting) { ClusterManager clusterManager = cluster.clusterManager(USERNAME, PASSWORD) @@ -177,22 +177,22 @@ class AbstractCouchbaseTest extends AgentTestRunner { // Insert Bucket... This generates a LOT of traces BucketSettings bucketSettings = clusterManager.insertBucket(bucketSetting) + // Insert Bucket admin user UserSettings userSettings = UserSettings.build().password(bucketSetting.password()) .roles([new UserRole("bucket_full_access", bucketSetting.name())]) - clusterManager.upsertUser(AuthDomain.LOCAL, bucketSetting.name(), userSettings) Bucket bucket = cluster.openBucket(bucketSettings.name(), bucketSettings.password()) -// boolean queryServiceEnabled = false -// while (!queryServiceEnabled) { -// GetClusterConfigResponse clusterConfig = bucket.core(). send(new GetClusterConfigRequest()).toBlocking().single() -// queryServiceEnabled = clusterConfig.config().bucketConfig(bucket.name()).serviceEnabled(ServiceType.QUERY) -// } bucket.query(Index.createPrimaryIndex().on(bucketSetting.name())) + // We don't have a good way to tell that all traces are reported + // since we don't know how many there will be. + Thread.sleep(150) TEST_WRITER.clear() // remove traces generated by insertBucket + + // Create view for SpringRepository's findAll() if (BucketType.COUCHBASE.equals(bucketSettings.type())) { bucket.bucketManager().insertDesignDocument( DesignDocument.create("doc", Collections.singletonList(DefaultView.create("all", diff --git a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java index f71cb77ea0..551c52b7a6 100644 --- a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java +++ b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/AgentTestRunner.java @@ -14,6 +14,7 @@ import io.opentracing.Tracer; import java.lang.instrument.ClassFileTransformer; import java.lang.instrument.Instrumentation; import java.util.List; +import java.util.ServiceLoader; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicInteger; @@ -123,7 +124,7 @@ public abstract class AgentTestRunner extends Specification { final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader(); try { Thread.currentThread().setContextClassLoader(AgentTestRunner.class.getClassLoader()); - // assert ServiceLoader.load(Instrumenter.class).iterator().hasNext(); + assert ServiceLoader.load(Instrumenter.class).iterator().hasNext(); activeTransformer = AgentInstaller.installBytebuddyAgent(instrumentation, ERROR_LISTENER); } finally { Thread.currentThread().setContextClassLoader(contextLoader);