Run tests with javaagent. (#1643)

Co-authored-by: Trask Stalnaker <[email protected]>

Author: Anuraag Agrawal

.github/scripts/deadlock-detector.sh

@@ -0,0 +1,20 @@
+#!/bin/bash
+
+while true
+do
+  sleep 60
+  file="/tmp/deadlock-detector-$(date +"%Y-%m-%d-%H-%M-%S").txt"
+  for pid in $(jps | grep -v Jps | awk '{ print $1 }')
+  do
+    jcmd $pid VM.command_line >> $file
+    jcmd $pid Thread.print >> $file
+    if jcmd $pid Thread.print | grep -q SimpleLogger
+    then
+      # check once more to eliminate most of the sporadic finds
+      if jcmd $pid Thread.print | grep -q SimpleLogger
+      then
+        jcmd $pid GC.heap_dump /tmp/deadlock-detector-$pid.hprof
+      fi
+    fi
+  done
+done &

.github/workflows/pr.yaml

@@ -18,12 +18,19 @@ jobs:
         with:
           job-id: jdk11
 
+      - name: Start deadlock detector
+        run: .github/scripts/deadlock-detector.sh
+
       - name: Build
-        uses: nick-invision/[email protected]
+        run: ./gradlew check --stacktrace -x :smoke-tests:test
+
+      - name: Upload deadlock detector artifacts
+        if: always()
+        uses: actions/upload-artifact@v2
         with:
-          command: ./gradlew check --stacktrace -x :smoke-tests:test
-          timeout_minutes: 90
-          max_attempts: 3
+          name: deadlock-detector-build
+          path: /tmp/deadlock-detector-*
+          if-no-files-found: ignore
 
   test:
     runs-on: ubuntu-latest
@@ -49,12 +56,19 @@ jobs:
         with:
           job-id: jdk${{ matrix.java }}
 
+      - name: Start deadlock detector
+        run: .github/scripts/deadlock-detector.sh
+
       - name: Test
-        uses: nick-invision/[email protected]
+        run: ./gradlew test -PtestJavaVersion=${{ matrix.java }} --stacktrace -x :smoke-tests:test -Porg.gradle.java.installations.paths=${{ steps.setup-test-java.outputs.path }} -Porg.gradle.java.installations.auto-download=false
+
+      - name: Upload deadlock detector artifacts
+        if: always()
+        uses: actions/upload-artifact@v2
         with:
-          command: ./gradlew test -PtestJavaVersion=${{ matrix.java }} --stacktrace -x :smoke-tests:test -Porg.gradle.java.installations.paths=${{ steps.setup-test-java.outputs.path }} -Porg.gradle.java.installations.auto-download=false
-          timeout_minutes: 90
-          max_attempts: 3
+          name: deadlock-detector-test-${{ matrix.java }}
+          path: /tmp/deadlock-detector-*
+          if-no-files-found: ignore
 
   smoke-test:
     runs-on: ubuntu-latest
@@ -72,11 +86,7 @@ jobs:
           job-id: smokeTests
 
       - name: Test
-        uses: nick-invision/[email protected]
-        with:
-          command: ./gradlew :smoke-tests:test
-          timeout_minutes: 90
-          max_attempts: 3
+        run: ./gradlew :smoke-tests:test
 
   setup-muzzle-matrix:
     runs-on: ubuntu-latest

CONTRIBUTING.md

@@ -54,6 +54,10 @@ See [Writing instrumentation](docs/contributing/writing-instrumentation.md)
 
 See [Understanding the javaagent components](docs/contributing/javaagent-jar-components.md)
 
+### Understanding the javaagent instrumentation testing components
+
+See [Understanding the javaagent instrumentation testing components](docs/contributing/javaagent-test-infra.md)
+
 ### Debugging
 
 See [Debugging](docs/contributing/debugging.md)

buildSrc/build.gradle

@@ -21,10 +21,6 @@ gradlePlugin {
       id = "muzzle"
       implementationClass = "MuzzlePlugin"
     }
-    create("javaagent-instrumentation-plugin") {
-      id = "io.opentelemetry.javaagent.instrumentation-instrumentation"
-      implementationClass = "io.opentelemetry.instrumentation.gradle.AutoInstrumentationPlugin"
-    }
   }
 }
 

buildSrc/src/main/java/io/opentelemetry/instrumentation/gradle/AutoInstrumentationPlugin.java

@@ -1,7 +1,6 @@
 ### Understanding the javaagent components
 
-OpenTelemetry Auto Instrumentation java agent's jar can logically be divided
-into 3 parts:
+The javaagent jar can logically be divided into 3 parts:
 
 * Modules that live in the system class loader
 * Modules that live in the bootstrap class loader

docs/contributing/javaagent-jar-components.md

@@ -0,0 +1,38 @@
+### Understanding the javaagent instrumentation testing components
+
+Javaagent instrumentation tests are run using a fully shaded `-javaagent` in order to perform
+the same bytecode instrumentation as when the agent is run against a normal app.
+
+There are a few key components that make this possible, described below.
+
+### gradle/instrumentation.gradle
+
+* shades the instrumentation
+* adds jvm args to the test configuration
+  * -javaagent:[agent for testing]
+  * -Dotel.initializer.jar=[shaded instrumentation jar]
+
+The `otel.initializer.jar` property is used to load the shaded instrumentation jar into the
+`AgentClassLoader`, so that the javaagent jar doesn't need to be re-built each time.
+
+### :testing:agent-exporter
+
+This contains the span and metric exporters that are used.
+
+These are in-memory exporters, so that the tests can verify the spans and metrics being exported.
+
+These exporters and the in-memory data live in the `AgentClassLoader`, so tests must access them
+using reflection. To simplify this, they store the in-memory data using the OTLP protobuf objects,
+so that they can be serialized into byte arrays inside the `AgentClassLoader`, then passed back
+to the tests and deserialized inside their class loader where they can be verified. The
+`:testing-common` module (described below) hides this complexity from instrumentation test authors.
+
+### :agent-for-testing
+
+This is a custom distro of the javaagent that embeds the `:testing:agent-exporter`.
+
+### :testing-common
+
+This module provides methods to help verify the span and metric data produced by the
+instrumentation, hiding the complexity of accessing the in-memory exporters that live in the
+`AgentClassLoader`.

docs/contributing/javaagent-test-infra.md

@@ -77,12 +77,12 @@ only depend on the OpenTelemetry API, `instrumentation-common`, and the instrume
 [instrumentation-library.gradle](../../gradle/instrumentation-library.gradle) needs to be applied to
 configure build tooling for the library.
 
-## Writing unit tests
+## Writing instrumentation tests
 
-Once the instrumentation is completed, we add unit tests to the `testing` module. Tests will
+Once the instrumentation is completed, we add tests to the `testing` module. Tests will
 generally apply to both library and agent instrumentation, with the only difference being how a client
 or server is initialized. In a library test, there will be code calling into the instrumentation API,
-while in an agent test, it will generally just use the underlying library's API as is. Create unit tests in an
+while in an agent test, it will generally just use the underlying library's API as is. Create tests in an
 abstract class with an abstract method that returns an instrumented object like a client. The class
 should itself extend from `InstrumentationSpecification` to be recognized by Spock and include helper
 methods for assertions.
@@ -100,11 +100,11 @@ Now that we have working instrumentation, we can implement agent instrumentation
 do not have to modify their apps to use it. Make sure the `javaagent` submodule has a dependency on the
 `library` submodule and a test dependency on the `testing` submodule. Agent instrumentation defines
 classes to match against to generate bytecode for. You will often match against the class you used
-in the unit test for library instrumentation, for example the builder of a client. And then you could
+in the test for library instrumentation, for example the builder of a client. And then you could
 match against the method that creates the builder, for example its constructor. Agent instrumentation
 can inject byte code to be run after the constructor returns, which would invoke e.g.,
 `registerInterceptor` and initialize the instrumentation. Often, the code inside the byte code
-decorator will be identical to the one in the unit test you wrote above - the agent does the work for
+decorator will be identical to the one in the test you wrote above - the agent does the work for
 initializing the instrumentation library, so a user doesn't have to.
 
 With that written, let's add tests for the agent instrumentation. We basically want to ensure that
@@ -113,6 +113,34 @@ the base class you wrote earlier, but in this, create a client using none of the
 only the ones offered by the library. Implement the `AgentTestRunner` trait for common setup logic,
 and try running. All the tests should pass for agent instrumentation too.
 
+Note that all the tests inside the `javaagent` module will be run using the shaded `-javaagent`
+in order to perform the same bytecode instrumentation as when the agent is run against a normal app.
+This means that the javaagent instrumentation will be inside the javaagent (inside of the
+`AgentClassLoader`) and will not be directly accessible to your test code. See the next section in
+case you need to write unit tests that directly access the javaagent instrumentation.
+
+## Writing Java agent unit tests
+
+As mentioned above, tests in the `javaagent` module cannot access the javaagent instrumentation
+classes directly.
+
+Ideally javaagent instrumentation is just a thin wrapper over library instrumentation, and so there
+is no need to write unit tests that directly access the javaagent instrumentation classes.
+
+If you still want to write a unit test against javaagent instrumentation, add another module
+named `javaagent-unittests`. Continuing with the example above:
+
+```
+instrumentation ->
+    ...
+    yarpc-1.0 ->
+        javaagent
+            yarpc-1.0-javaagent.gradle
+        javaagent-unittest
+            yarpc-1.0-javaagent-unittest.gradle
+        ...
+```
+
 ### Java agent instrumentation gotchas
 
 #### Calling Java 8 default methods from advice

docs/contributing/writing-instrumentation.md

@@ -6,20 +6,15 @@ affecting it negatively, for example introducing crashes.
 
 ## Instrumentation tests
 
-All instrumentation are written with instrumentation tests - these can be considered
-the unit tests of this project. Instrumentation tests invoke bytecode manipulation to
-actually rewrite classes similar to how the agent would in a normal app. By then
-exercising the instrumented library in a way a user would, for example by issuing
-requests from an HTTP client, we can assert on the spans that should be generated, including
-their semantic attributes. A problem in the instrumentation will generally cause
-spans to be reported incorrectly or not reported at all, and we can find these situations
-with the instrumentation tests.
-
-Note: the instrumentation tests have significant differences from when run against a
-normal app with `-javaagent`, in particular, our usual shading is not applied which
-can cause false positives. We have work in progress to actually run tests with `-javaagent`
-after applying our shading, in which case these tests will run almost identically to
-a normal app and eliminate false positives. https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/1643
+All instrumentation are written with instrumentation tests - these can be considered the unit tests
+of this project.
+
+Instrumentation tests are run using a fully shaded `-javaagent` in order to perform the same bytecode
+instrumentation as when the agent is run against a normal app.
+By then exercising the instrumented library in a way a user would, for example by issuing requests
+from an HTTP client, we can assert on the spans that should be generated, including their semantic
+attributes. A problem in the instrumentation will generally cause spans to be reported incorrectly
+or not reported at all, and we can find these situations with the instrumentation tests.
 
 ## Latest dep tests
 

docs/safety-mechanisms.md

@@ -53,4 +53,7 @@ shadowJar {
   relocate "io.opentelemetry.spi", "io.opentelemetry.javaagent.shaded.io.opentelemetry.spi"
   relocate "io.opentelemetry.context", "io.opentelemetry.javaagent.shaded.io.opentelemetry.context"
 
+  // relocate OpenTelemetry extensions
+  relocate "io.opentelemetry.extension.kotlin", "io.opentelemetry.javaagent.shaded.io.opentelemetry.extension.kotlin"
+  relocate "io.opentelemetry.extension.trace.propagation", "io.opentelemetry.javaagent.shaded.io.opentelemetry.extension.trace.propagation"
 }

examples/distro/instrumentation/build.gradle

@@ -8,28 +8,28 @@ configurations.all {
 
 ext {
   versions = [
-    opentelemetry       : '0.13.1',
-    opentelemetryAlpha  : "0.13.1-alpha",
+    opentelemetry     : '0.13.1',
+    opentelemetryAlpha: "0.13.1-alpha",
 
-    slf4j               : "1.7.30",
-    guava               : "20.0", // Last version to support Java 7
+    slf4j             : "1.7.30",
+    guava             : "20.0", // Last version to support Java 7
 
-    spock               : "1.3-groovy-$spockGroovyVer",
-    groovy              : groovyVer,
-    logback             : "1.2.3",
-    bytebuddy           : "1.10.18", // Also explicitly specified in buildSrc
-    scala               : "2.11.12",  // Last version to support Java 7 (2.12+ require Java 8+)
-    kotlin              : "1.4.0",
-    coroutines          : "1.3.0",
-    springboot          : "2.3.1.RELEASE",
+    spock             : "1.3-groovy-$spockGroovyVer",
+    groovy            : groovyVer,
+    logback           : "1.2.3",
+    bytebuddy         : "1.10.18", // Also explicitly specified in buildSrc
+    scala             : "2.11.12",  // Last version to support Java 7 (2.12+ require Java 8+)
+    kotlin            : "1.4.0",
+    coroutines        : "1.3.0",
+    springboot        : "2.3.1.RELEASE",
     // TODO(anuraaga): Switch off of milestones, this version fixes compatibility with Spock Unroll
-    junit5              : "5.7.0-M1",
-    checkerFramework    : "3.6.1",
-    errorprone          : "2.4.0",
-    nullaway            : "0.8.0",
-    autoValue           : "1.7.4",
-    systemLambda        : "1.1.0",
-    prometheus          : "0.9.0"
+    junit5            : "5.7.0-M1",
+    checkerFramework  : "3.6.1",
+    errorprone        : "2.4.0",
+    nullaway          : "0.8.0",
+    autoValue         : "1.7.4",
+    systemLambda      : "1.1.0",
+    prometheus        : "0.9.0"
   ]
 
   deps = [
@@ -40,6 +40,7 @@ ext {
     opentelemetryKotlin        : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-extension-kotlin', version: versions.opentelemetry),
     opentelemetryTraceProps    : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-extension-trace-propagators', version: versions.opentelemetry),
     opentelemetrySdk           : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-sdk', version: versions.opentelemetry),
+    opentelemetrySdkMetrics    : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-sdk-metrics', version: versions.opentelemetryAlpha),
     opentelemetryJaeger        : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporter-jaeger', version: versions.opentelemetry),
     opentelemetryJaegerThrift  : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporter-jaeger-thrift', version: versions.opentelemetry),
     opentelemetryOtlp          : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporter-otlp', version: versions.opentelemetry),
@@ -49,6 +50,7 @@ ext {
     opentelemetryLogging       : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-exporter-logging', version: versions.opentelemetry),
     opentelemetryProto         : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-proto', version: versions.opentelemetry),
     opentelemetryResources     : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-sdk-extension-resources', version: versions.opentelemetry),
+    opentelemetrySdkTesting    : dependencies.create(group: 'io.opentelemetry', name: 'opentelemetry-sdk-testing', version: versions.opentelemetry),
 
     // General
     slf4j                      : "org.slf4j:slf4j-api:${versions.slf4j}",