Binary execution support

CHANGELOG.asciidoc

@@ -28,8 +28,9 @@ endif::[]
 
 * Add support for <<supported-databases, Redis Lettuce client>>
 * Add `context.message.age.ms` field for JMS message receiving spans and transactions - {pull}970[#970]
-* Instrument log4j Logger#error(String, Throwable) (#919)
+* Instrument log4j Logger#error(String, Throwable) ({pull}919[#919])
   Automatically captures exceptions when calling `logger.error("message", exception)`
+* Add instrumentation for external process execution through `java.lang.Process` and Apache `commons-exec` - {pull}903[#903]
 
 [float]
 ===== Bug Fixes

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/ElasticApmAgent.java

@@ -74,6 +74,7 @@
 import static net.bytebuddy.matcher.ElementMatchers.nameContains;
 import static net.bytebuddy.matcher.ElementMatchers.nameEndsWith;
 import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
+import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.not;
 
 public class ElasticApmAgent {
@@ -342,11 +343,16 @@ private static AgentBuilder getAgentBuilder(final ByteBuddy byteBuddy, final Cor
                 : AgentBuilder.PoolStrategy.Default.FAST)
             .ignore(any(), isReflectionClassLoader())
             .or(any(), classLoaderWithName("org.codehaus.groovy.runtime.callsite.CallSiteClassLoader"))
+            // ideally, those bootstrap classpath inclusions should be set at plugin level, see issue #952
             .or(nameStartsWith("java.")
                 .and(
                     not(
                         nameEndsWith("URLConnection")
                             .or(nameStartsWith("java.util.concurrent."))
+                            .or(named("java.lang.ProcessBuilder"))
+                            .or(named("java.lang.ProcessImpl"))
+                            .or(named("java.lang.Process"))
+                            .or(named("java.lang.UNIXProcess"))
                     )
                 )
             )

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java

@@ -391,7 +391,6 @@ public <T extends ConfigurationOptionProvider> T getConfig(Class<T> pluginClass)
         return configurationRegistry.getConfig(pluginClass);
     }
 
-    @SuppressWarnings("ReferenceEquality")
     public void endTransaction(Transaction transaction) {
         if (logger.isDebugEnabled()) {
             logger.debug("} endTransaction {}", transaction);
@@ -408,7 +407,6 @@ public void endTransaction(Transaction transaction) {
         }
     }
 
-    @SuppressWarnings("ReferenceEquality")
     public void endSpan(Span span) {
         if (span.isSampled() && !span.isDiscard()) {
             long spanFramesMinDurationMs = stacktraceConfiguration.getSpanFramesMinDurationMs();

apm-agent-core/src/test/java/co/elastic/apm/agent/AbstractInstrumentationTest.java

@@ -11,9 +11,9 @@
  * 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
@@ -91,6 +91,9 @@ public final void resetReporter() {
     @AfterEach
     public final void cleanUp() {
         tracer.resetServiceNameOverrides();
-        assertThat(tracer.getActive()).isNull();
+
+        assertThat(tracer.getActive())
+            .describedAs("nothing should be left active at end of test, failure will likely indicate a span/transaction still active")
+            .isNull();
     }
 }

apm-agent-plugins/apm-process-plugin/README.md

@@ -0,0 +1,21 @@
+# Process plugin
+
+This plugin creates spans for external processes executed by the JVM, which use the `java.lang.Process` class.
+
+[Apache commons-exec](https://commons.apache.org/proper/commons-exec/) library support is included.
+
+## Limitations
+
+`java.lang.ProcessHandler` and `java.lang.Process.toHandle()` introduced in java 9 are not
+instrumented. As a result, process execution using this API is not yet supported.
+
+## Implementation Notes
+
+Instrumentation of classes in `java.lang.*` that are loaded in the bootstrap classloader can't
+be tested with unit tests due to the fact that agent is loaded in application/system classloader
+for those tests.
+
+Thus, using integration tests is required to test the instrumentation end-to-end.
+Also, in order to provide a good test of this feature without testing everything in integration tests, we
+- delegate most of the advice code to helper classes
+- test extensively those classes with regular unit tests

apm-agent-plugins/apm-process-plugin/pom.xml

@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <parent>
+        <artifactId>apm-agent-plugins</artifactId>
+        <groupId>co.elastic.apm</groupId>
+        <version>1.12.1-SNAPSHOT</version>
+    </parent>
+    <modelVersion>4.0.0</modelVersion>
+
+    <properties>
+        <apm-agent-parent.base.dir>${project.basedir}/../..</apm-agent-parent.base.dir>
+    </properties>
+
+    <artifactId>apm-process-plugin</artifactId>
+    <name>${project.groupId}:${project.artifactId}</name>
+
+    <dependencies>
+        <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-exec</artifactId>
+            <version>1.3</version>
+            <scope>test</scope>
+        </dependency>
+    </dependencies>
+</project>

apm-agent-plugins/apm-process-plugin/src/main/java/co/elastic/apm/agent/process/BaseProcessInstrumentation.java

@@ -0,0 +1,48 @@
+/*-
+ * #%L
+ * Elastic APM Java agent
+ * %%
+ * Copyright (C) 2018 - 2019 Elastic and contributors
+ * %%
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you 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.
+ * #L%
+ */
+package co.elastic.apm.agent.process;
+
+import co.elastic.apm.agent.bci.ElasticApmInstrumentation;
+import net.bytebuddy.matcher.ElementMatcher;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import static net.bytebuddy.matcher.ElementMatchers.isBootstrapClassLoader;
+
+public abstract class BaseProcessInstrumentation extends ElasticApmInstrumentation {
+
+    @Override
+    public final ElementMatcher.Junction<ClassLoader> getClassLoaderMatcher() {
+        // java.lang.* is loaded from bootstrap classloader
+        return isBootstrapClassLoader();
+    }
+
+    @Override
+    public final Collection<String> getInstrumentationGroupNames() {
+        return Arrays.asList("process", "incubating");
+    }
+
+}

apm-agent-plugins/apm-process-plugin/src/main/java/co/elastic/apm/agent/process/CommonsExecAsyncInstrumentation.java

@@ -0,0 +1,101 @@
+/*-
+ * #%L
+ * Elastic APM Java agent
+ * %%
+ * Copyright (C) 2018 - 2019 Elastic and contributors
+ * %%
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you 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.
+ * #L%
+ */
+package co.elastic.apm.agent.process;
+
+import co.elastic.apm.agent.bci.ElasticApmInstrumentation;
+import net.bytebuddy.asm.Advice;
+import net.bytebuddy.description.NamedElement;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import static net.bytebuddy.matcher.ElementMatchers.hasSuperClass;
+import static net.bytebuddy.matcher.ElementMatchers.nameContains;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
+
+/**
+ * Provides context propagation for apache commons-exec library that delegates to a background thread for
+ * asynchronous process execution. Synchronous execution is already covered with {@link Process} instrumentation.
+ * <p>
+ * Instruments {@code org.apache.commons.exec.DefaultExecutor#createThread(Runnable, String)} and any direct subclass
+ * that overrides it.
+ */
+public class CommonsExecAsyncInstrumentation extends ElasticApmInstrumentation {
+
+    private static final String DEFAULT_EXECUTOR_CLASS = "org.apache.commons.exec.DefaultExecutor";
+    // only known subclass of default implementation
+    private static final String DAEMON_EXECUTOR_CLASS = "org.apache.commons.exec.DaemonExecutor";
+
+    @Override
+    public ElementMatcher<? super NamedElement> getTypeMatcherPreFilter() {
+        // Most implementations are likely to have 'Executor' in their name, which will work most of the time
+        // while not perfect this allows to avoid the expensive 'hasSuperClass' in most cases
+        return nameContains("Executor");
+    }
+
+    @Override
+    public ElementMatcher<? super TypeDescription> getTypeMatcher() {
+        // instrument default implementation and direct subclasses
+        return named(DEFAULT_EXECUTOR_CLASS)
+            .or(named(DAEMON_EXECUTOR_CLASS))
+            // this super class check is expensive
+            .or(hasSuperClass(named(DEFAULT_EXECUTOR_CLASS)));
+    }
+
+    @Override
+    public ElementMatcher<? super MethodDescription> getMethodMatcher() {
+        return named("createThread")
+            .and(takesArgument(0, Runnable.class));
+    }
+
+    @Override
+    public Collection<String> getInstrumentationGroupNames() {
+        // part of 'process' group, as usage is not relevant without it, relies on generic Process instrumentation
+        return Arrays.asList("apache-commons-exec", "process", "incubating");
+    }
+
+    @Override
+    public Class<?> getAdviceClass() {
+        return CommonsExecAdvice.class;
+    }
+
+    public static final class CommonsExecAdvice {
+
+        @Advice.OnMethodEnter(suppress = Throwable.class)
+        private static void onEnter(@Advice.Argument(value = 0, readOnly = false) Runnable runnable) {
+            if (tracer == null || tracer.getActive() == null) {
+                return;
+            }
+            // context propagation is done by wrapping existing runnable argument
+
+            //noinspection UnusedAssignment
+            runnable = tracer.getActive().withActive(runnable);
+        }
+    }
+}