Automatic instrumentation should not override manual results (#752)

Author: wolframhaussig

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

@@ -73,7 +73,8 @@ protected Labels.Mutable initialValue() {
     private final WriterReaderPhaser phaser = new WriterReaderPhaser();
 
     /**
-     * The result of the transaction. HTTP status code for HTTP-related transactions.
+     * The result of the transaction. HTTP status code for HTTP-related
+     * transactions.
      */
     @Nullable
     private String result;
@@ -155,7 +156,21 @@ public String getResult() {
     }
 
     /**
-     * The result of the transaction. HTTP status code for HTTP-related transactions.
+     * The result of the transaction. HTTP status code for HTTP-related
+     * transactions. This sets the result only if it is not already set. should be
+     * used for instrumentations
+     */
+    public Transaction withResultIfUnset(@Nullable String result) {
+        if (this.result == null) {
+            this.result = result;
+        }
+        return this;
+    }
+
+    /**
+     * The result of the transaction. HTTP status code for HTTP-related
+     * transactions. This sets the result regardless of an already existing value.
+     * should be used for user defined results
      */
     public Transaction withResult(@Nullable String result) {
         this.result = result;

apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/plugin/api/TransactionInstrumentationTest.java

@@ -81,6 +81,22 @@ void testResult() {
         assertThat(reporter.getFirstTransaction().getResult()).isEqualTo("foo");
     }
 
+    @Test
+    void testInstrumentationDoesNotOverrideUserResult() {
+        transaction.setResult("foo");
+        endTransaction();
+        reporter.getFirstTransaction().withResultIfUnset("200");
+        assertThat(reporter.getFirstTransaction().getResult()).isEqualTo("foo");
+    }
+
+    @Test
+    void testUserCanOverrideResult() {
+        transaction.setResult("foo");
+        transaction.setResult("bar");
+        endTransaction();
+        assertThat(reporter.getFirstTransaction().getResult()).isEqualTo("bar");
+    }
+
     @Test
     void testChaining() {
         transaction.setType("foo").setName("foo").addLabel("foo", "bar").setUser("foo", "bar", "baz").setResult("foo");

apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/test/java/co/elastic/apm/agent/es/restclient/AbstractEsClientInstrumentationTest.java

@@ -71,7 +71,7 @@ public void startTransaction() {
         Transaction transaction = tracer.startTransaction(TraceContext.asRoot(), null, null).activate();
         transaction.withName("ES Transaction");
         transaction.withType("request");
-        transaction.withResult("success");
+        transaction.withResultIfUnset("success");
     }
 
     @After

apm-agent-plugins/apm-jdbc-plugin/src/test/java/co/elastic/apm/agent/jdbc/AbstractJdbcInstrumentationTest.java

@@ -73,7 +73,7 @@ public abstract class AbstractJdbcInstrumentationTest extends AbstractInstrument
         transaction = tracer.startTransaction(TraceContext.asRoot(), null, null).activate();
         transaction.withName("transaction");
         transaction.withType("request");
-        transaction.withResult("success");
+        transaction.withResultIfUnset("success");
         signatureParser = new SignatureParser();
     }
 

apm-agent-plugins/apm-opentracing-plugin/src/main/java/co/elastic/apm/agent/opentracing/impl/ApmSpanInstrumentation.java

@@ -213,15 +213,13 @@ private static boolean handleSpecialTransactionTag(Transaction transaction, Stri
                 transaction.withResult(value.toString());
                 return true;
             } else if ("error".equals(key)) {
-                if (transaction.getResult() == null && Boolean.FALSE.equals(value)) {
-                    transaction.withResult("error");
+                if (Boolean.FALSE.equals(value)) {
+                    transaction.withResultIfUnset("error");
                 }
                 return true;
             } else if ("http.status_code".equals(key) && value instanceof Number) {
                 transaction.getContext().getResponse().withStatusCode(((Number) value).intValue());
-                if (transaction.getResult() == null) {
-                    transaction.withResult(ResultUtil.getResultByHttpStatus(((Number) value).intValue()));
-                }
+                transaction.withResultIfUnset(ResultUtil.getResultByHttpStatus(((Number) value).intValue()));
                 transaction.withType(Transaction.TYPE_REQUEST);
                 return true;
             } else if ("http.method".equals(key)) {

apm-agent-plugins/apm-quartz-job-plugin/src/main/java/co/elastic/apm/agent/quartz/job/JobTransactionNameAdvice.java

@@ -67,8 +67,8 @@ private static void setTransactionName(@Advice.Argument(value = 0) @Nullable Job
     public static void onMethodExitException(@Advice.Argument(value = 0) @Nullable JobExecutionContext context,
                                              @Advice.Local("transaction") @Nullable Transaction transaction, @Advice.Thrown Throwable t) {
         if (transaction != null) {
-            if (transaction.getResult() == null && context != null && context.getResult() != null) {
-                transaction.withResult(context.getResult().toString());
+            if (context != null && context.getResult() != null) {
+                transaction.withResultIfUnset(context.getResult().toString());
             }
             transaction.captureException(t)
                 .deactivate()

apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletTransactionHelper.java

@@ -189,7 +189,7 @@ private void doOnAfter(Transaction transaction, @Nullable Throwable exception, b
             status = 500;
         }
         fillResponse(transaction.getContext().getResponse(), committed, status);
-        transaction.withResult(ResultUtil.getResultByHttpStatus(status));
+        transaction.withResultIfUnset(ResultUtil.getResultByHttpStatus(status));
         transaction.withType("request");
         applyDefaultTransactionName(method, servletPath, pathInfo, transaction);
         if (exception != null) {