Introduce priorities for transaction name (#748)

Now uses a transaction name according to use_path_as_transaction_name
rather than ServletClass#doGet.
But if a name can be determined from a high level framework,
like Spring MVC, that takes precedence.
User-supplied names from the API always take precedence over any others.

fixes #673

Author: felixbarny

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

@@ -40,6 +40,11 @@
 import java.util.concurrent.atomic.AtomicLong;
 
 public abstract class AbstractSpan<T extends AbstractSpan> extends TraceContextHolder<T> {
+    public static final int PRIO_USER_SUPPLIED = 1000;
+    public static final int PRIO_HIGH_LEVEL_FRAMEWORK = 100;
+    public static final int PRIO_METHOD_SIGNATURE = 100;
+    public static final int PRIO_LOW_LEVEL_FRAMEWORK = 10;
+    public static final int PRIO_DEFAULT = 0;
     private static final Logger logger = LoggerFactory.getLogger(AbstractSpan.class);
     protected static final double MS_IN_MICROS = TimeUnit.MILLISECONDS.toMicros(1);
     protected final TraceContext traceContext;
@@ -56,6 +61,7 @@ public abstract class AbstractSpan<T extends AbstractSpan> extends TraceContextH
     private ChildDurationTimer childDurations = new ChildDurationTimer();
     protected AtomicInteger references = new AtomicInteger();
     protected volatile boolean finished = true;
+    private int namePriority = PRIO_DEFAULT;
 
     public int getReferenceCount() {
         return references.get();
@@ -143,18 +149,36 @@ public double getDurationMs() {
     }
 
     /**
-     * Generic designation of a transaction in the scope of a single service (eg: 'GET /users/:id')
+     * Only intended to be used by {@link co.elastic.apm.agent.report.serialize.DslJsonSerializer}
      */
-    public StringBuilder getName() {
+    public StringBuilder getNameForSerialization() {
         return name;
     }
 
     /**
-     * Generic designation of a transaction in the scope of a single service (eg: 'GET /users/:id')
+     * Resets and returns the name {@link StringBuilder} if the provided priority is {@code >=} {@link #namePriority} one.
+     * Otherwise, returns {@code null}
+     *
+     * @param namePriority the priority for the name. See also the {@code AbstractSpan#PRIO_*} constants.
+     * @return the name {@link StringBuilder} if the provided priority is {@code >=} {@link #namePriority}, {@code null} otherwise.
+     */
+    @Nullable
+    public StringBuilder getAndOverrideName(int namePriority) {
+        if (namePriority >= this.namePriority) {
+            this.namePriority = namePriority;
+            this.name.setLength(0);
+            return name;
+        } else {
+            return null;
+        }
+    }
+
+    /**
+     * Only intended for testing purposes as this allocates a {@link String}
+     * @return
      */
-    public void setName(@Nullable String name) {
-        this.name.setLength(0);
-        this.name.append(name);
+    public String getNameAsString() {
+        return name.toString();
     }
 
     /**
@@ -168,7 +192,27 @@ public void setName(@Nullable String name) {
      * @return {@code this}, for chaining
      */
     public T appendToName(String s) {
-        name.append(s);
+        return appendToName(s, PRIO_DEFAULT);
+    }
+
+    public T appendToName(String s, int priority) {
+        if (priority >= namePriority) {
+            this.name.append(s);
+            this.namePriority = priority;
+        }
+        return (T) this;
+    }
+
+    public T withName(@Nullable String name) {
+        return withName(name, PRIO_DEFAULT);
+    }
+
+    public T withName(@Nullable String name, int priority) {
+        if (priority >= namePriority && name != null && !name.isEmpty()) {
+            this.name.setLength(0);
+            this.name.append(name);
+            this.namePriority = priority;
+        }
         return (T) this;
     }
 
@@ -194,6 +238,7 @@ public void resetState() {
         traceContext.resetState();
         childDurations.resetState();
         references.set(0);
+        namePriority = PRIO_DEFAULT;
     }
 
     public boolean isChildOf(AbstractSpan<?> parent) {

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

@@ -118,11 +118,6 @@ public SpanContext getContext() {
         return context;
     }
 
-    public Span withName(@Nullable String name) {
-        setName(name);
-        return this;
-    }
-
     /**
      * Keywords of specific relevance in the span's domain (eg: 'db', 'template', 'ext', etc)
      */

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

@@ -138,11 +138,6 @@ public TransactionContext getContextEnsureVisibility() {
         }
     }
 
-    public Transaction withName(@Nullable String name) {
-        setName(name);
-        return this;
-    }
-
     /**
      * Keyword of specific relevance in the service's domain (eg:  'request', 'backgroundjob')
      */

apm-agent-core/src/main/java/co/elastic/apm/agent/report/serialize/DslJsonSerializer.java

@@ -526,7 +526,7 @@ private void serializeTransactions(final List<Transaction> transactions) {
     private void serializeTransaction(final Transaction transaction) {
         jw.writeByte(OBJECT_START);
         writeTimestamp(transaction.getTimestamp());
-        writeField("name", transaction.getName());
+        writeField("name", transaction.getNameForSerialization());
         serializeTraceContext(transaction.getTraceContext(), false);
         writeField("type", transaction.getType());
         writeField("duration", transaction.getDurationMs());
@@ -567,7 +567,7 @@ private void serializeSpans(final List<Span> spans) {
 
     private void serializeSpan(final Span span) {
         jw.writeByte(OBJECT_START);
-        writeField("name", span.getName());
+        writeField("name", span.getNameForSerialization());
         writeTimestamp(span.getTimestamp());
         serializeTraceContext(span.getTraceContext(), true);
         writeField("duration", span.getDurationMs());

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

@@ -38,7 +38,7 @@ public class TransactionUtils {
 
     public static void fillTransaction(Transaction t) {
         t.start(TraceContext.asRoot(), null, (long) 0, ConstantSampler.of(true), TransactionUtils.class.getClassLoader());
-        t.setName("GET /api/types");
+        t.withName("GET /api/types");
         t.withType("request");
         t.withResult("success");
 

apm-agent-core/src/test/java/co/elastic/apm/agent/bci/methodmatching/TraceMethodInstrumentationTest.java

@@ -88,18 +88,18 @@ void tearDown() {
     void testTraceMethod() {
         TestClass.traceMe();
         assertThat(reporter.getTransactions()).hasSize(1);
-        assertThat(reporter.getFirstTransaction().getName().toString()).isEqualTo("TestClass#traceMe");
+        assertThat(reporter.getFirstTransaction().getNameAsString()).isEqualTo("TestClass#traceMe");
         assertThat(reporter.getSpans()).hasSize(1);
-        assertThat(reporter.getFirstSpan().getName().toString()).isEqualTo("TestClass#traceMeToo");
+        assertThat(reporter.getFirstSpan().getNameAsString()).isEqualTo("TestClass#traceMeToo");
     }
 
     @Test
     void testExcludedMethod() {
         TestClass.traceMeAsWell();
         assertThat(reporter.getTransactions()).hasSize(1);
-        assertThat(reporter.getFirstTransaction().getName().toString()).isEqualTo("TestClass#traceMeAsWell");
+        assertThat(reporter.getFirstTransaction().getNameAsString()).isEqualTo("TestClass#traceMeAsWell");
         assertThat(reporter.getSpans()).hasSize(1);
-        assertThat(reporter.getFirstSpan().getName().toString()).isEqualTo("TestClass#traceMeToo");
+        assertThat(reporter.getFirstSpan().getNameAsString()).isEqualTo("TestClass#traceMeToo");
     }
 
     @Test
@@ -122,7 +122,7 @@ void testNotMatched_Constructor() {
         assertThat(reporter.getTransactions()).isEmpty();
         testClass.traceMe();
         assertThat(reporter.getTransactions()).hasSize(1);
-        assertThat(reporter.getFirstTransaction().getName().toString()).isEqualTo("TestExcludeConstructor#traceMe");
+        assertThat(reporter.getFirstTransaction().getNameAsString()).isEqualTo("TestExcludeConstructor#traceMe");
     }
 
     @Test

apm-agent-core/src/test/java/co/elastic/apm/agent/impl/ElasticApmTracerTest.java

@@ -263,7 +263,7 @@ void testDisableMidTransaction() {
             try (Scope spanScope = span.activateInScope()) {
                 when(config.getConfig(CoreConfiguration.class).isActive()).thenReturn(false);
                 span.withName("test");
-                assertThat(span.getName().toString()).isEqualTo("test");
+                assertThat(span.getNameAsString()).isEqualTo("test");
                 assertThat(tracerImpl.getActive()).isSameAs(span);
                 assertThat(span.isChildOf(transaction)).isTrue();
                 span.end();
@@ -272,7 +272,7 @@ void testDisableMidTransaction() {
             try (Scope spanScope = span2.activateInScope()) {
                 when(config.getConfig(CoreConfiguration.class).isActive()).thenReturn(false);
                 span2.withName("test2");
-                assertThat(span2.getName().toString()).isEqualTo("test2");
+                assertThat(span2.getNameAsString()).isEqualTo("test2");
                 assertThat(tracerImpl.getActive()).isSameAs(span2);
                 assertThat(span2.isChildOf(transaction)).isTrue();
                 span2.end();

apm-agent-core/src/test/java/co/elastic/apm/agent/impl/transaction/SpanTest.java

@@ -47,7 +47,7 @@ void resetState() {
             .withUser("readonly_user");
         span.resetState();
         assertThat(span.getContext().hasContent()).isFalse();
-        assertThat((CharSequence) span.getName()).isNullOrEmpty();
+        assertThat(span.getNameAsString()).isNullOrEmpty();
         assertThat(span.getType()).isNull();
         assertThat(span.getSubtype()).isNull();
         assertThat(span.getAction()).isNull();

apm-agent-core/src/test/java/co/elastic/apm/agent/metrics/builtin/SystemMetricsTest.java

@@ -28,6 +28,8 @@
 import co.elastic.apm.agent.metrics.MetricRegistry;
 import co.elastic.apm.agent.report.ReporterConfiguration;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledOnOs;
+import org.junit.jupiter.api.condition.OS;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.CsvSource;
 
@@ -36,6 +38,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 
+@DisabledOnOs(OS.MAC)
 class SystemMetricsTest {
 
     private MetricRegistry metricRegistry = new MetricRegistry(mock(ReporterConfiguration.class));

apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/AbstractSpanInstrumentation.java

@@ -39,6 +39,7 @@
 import javax.annotation.Nullable;
 import java.lang.invoke.MethodHandle;
 
+import static co.elastic.apm.agent.impl.transaction.AbstractSpan.PRIO_USER_SUPPLIED;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
 
@@ -73,7 +74,7 @@ public SetNameInstrumentation() {
         public static void setName(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) TraceContextHolder<?> context,
                                    @Advice.Argument(0) String name) {
             if (context instanceof AbstractSpan) {
-                ((AbstractSpan) context).setName(name);
+                ((AbstractSpan) context).withName(name, PRIO_USER_SUPPLIED);
             }
         }
     }