Fix too small activation stack for low transaction_max_spans values (#…

…3643)
elastic · May 22, 2024 · a28d978 · a28d978
1 parent a65bf2e
commit a28d978
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 33 deletions.
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
@@ -39,6 +39,7 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
 ===== Bug fixes
 * Fixed edge case where inferred spans could cause cycles in the trace parent-child relationships, subsequently resulting in the UI crashing - {pull}3588[#3588]
 * Fix NPE in dropped spans statistics - {pull}3590[#3590]
+* Fix too small activation stack size for small `transaction_max_spans` values - {pull}3643[#3643]
 
 [[release-notes-1.x]]
 === Java Agent version 1.x

diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java
@@ -112,6 +112,7 @@ public class ElasticApmTracer implements Tracer {
     private static final Map<Class<?>, Class<? extends ConfigurationOptionProvider>> configs = new HashMap<>();
 
     public static final Set<String> TRACE_HEADER_NAMES;
+    public static final int ACTIVATION_STACK_BASE_SIZE = 16;
 
     static {
         Set<String> headerNames = new HashSet<>();
@@ -138,7 +139,10 @@ public class ElasticApmTracer implements Tracer {
     private final ThreadLocal<ActiveStack> activeStack = new ThreadLocal<ActiveStack>() {
         @Override
         protected ActiveStack initialValue() {
-            return new ActiveStack(transactionMaxSpans, emptyContext);
+            //We allow transactionMaxSpan activation plus a constant minimum of 16 to account for
+            // * the activation of the transaction itself
+            // * account for baggage updates, which also count towards the depth
+            return new ActiveStack(ACTIVATION_STACK_BASE_SIZE + transactionMaxSpans, emptyContext);
         }
     };
 
@@ -673,7 +677,7 @@ public synchronized void stop() {
     public Reporter getReporter() {
         return reporter;
     }
-    
+
     public UniversalProfilingIntegration getProfilingIntegration() {
         return profilingIntegration;
     }

diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/ElasticApmTracerTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/impl/ElasticApmTracerTest.java
@@ -22,6 +22,7 @@
 import co.elastic.apm.agent.common.util.WildcardMatcher;
 import co.elastic.apm.agent.configuration.AutoDetectedServiceInfo;
 import co.elastic.apm.agent.configuration.CoreConfiguration;
+import co.elastic.apm.agent.impl.baggage.BaggageContext;
 import co.elastic.apm.agent.tracer.service.ServiceInfo;
 import co.elastic.apm.agent.configuration.SpyConfiguration;
 import co.elastic.apm.agent.configuration.source.ConfigSources;
@@ -52,6 +53,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.function.Consumer;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -73,9 +75,15 @@ class ElasticApmTracerTest {
 
     @BeforeEach
     void setUp() {
+        doSetup(conf -> {
+        });
+    }
+
+    void doSetup(Consumer<ConfigurationRegistry> configCustomizer) {
         objectPoolFactory = new TestObjectPoolFactory();
         reporter = new MockReporter();
         config = SpyConfiguration.createSpyConfig();
+        configCustomizer.accept(config);
 
         apmServerClient = new ApmServerClient(config);
         apmServerClient = mock(ApmServerClient.class, delegatesTo(apmServerClient));
@@ -88,6 +96,11 @@ void setUp() {
             .buildAndStart();
     }
 
+    void setupWithCustomConfig(Consumer<ConfigurationRegistry> configCustomizer) {
+        cleanupAndCheck(); //cleanup @BeforeEach
+        doSetup(configCustomizer);
+    }
+
     @AfterEach
     void cleanupAndCheck() {
         reporter.assertRecycledAfterDecrementingReferences();
@@ -326,11 +339,14 @@ private ErrorCapture validateError(ErrorCapture error, AbstractSpan<?> span, boo
 
     @Test
     void testEnableDropSpans() {
-        doReturn(1).when(tracerImpl.getConfig(CoreConfiguration.class)).getTransactionMaxSpans();
+        setupWithCustomConfig(conf -> {
+            doReturn(1).when(conf.getConfig(CoreConfiguration.class)).getTransactionMaxSpans();
+        });
         Transaction transaction = startTestRootTransaction();
         try (Scope scope = transaction.activateInScope()) {
             Span span = tracerImpl.getActive().createSpan();
             try (Scope spanScope = span.activateInScope()) {
+                assertThat(tracerImpl.getActive()).isSameAs(span); //ensure ActiveStack limit is not reached
                 assertThat(span.isSampled()).isTrue();
                 span.end();
             }
@@ -359,48 +375,61 @@ void testActivationStackOverflow() {
             .withObjectPoolFactory(objectPoolFactory)
             .buildAndStart();
 
-        Transaction transaction = tracer.startRootTransaction(getClass().getClassLoader());
-        assertThat(tracer.getActive()).isNull();
-        try (Scope scope = transaction.activateInScope()) {
-            assertThat(tracer.getActive()).isEqualTo(transaction);
-            Span child1 = transaction.createSpan();
-            try (Scope childScope = child1.activateInScope()) {
-                assertThat(tracer.getActive()).isEqualTo(child1);
-                Span grandchild1 = child1.createSpan();
-                try (Scope grandchildScope = grandchild1.activateInScope()) {
-                    // latter activation should not be applied due to activation stack overflow
+        doWithNestedBaggageActivations(() -> {
+            Transaction transaction = tracer.startRootTransaction(getClass().getClassLoader());
+            assertThat(tracer.getActive()).isNull();
+            try (Scope scope = transaction.activateInScope()) {
+                assertThat(tracer.getActive()).isEqualTo(transaction);
+                Span child1 = transaction.createSpan();
+                try (Scope childScope = child1.activateInScope()) {
                     assertThat(tracer.getActive()).isEqualTo(child1);
-                    Span ggc = grandchild1.createSpan();
-                    try (Scope ggcScope = ggc.activateInScope()) {
+                    Span grandchild1 = child1.createSpan();
+                    try (Scope grandchildScope = grandchild1.activateInScope()) {
+                        // latter activation should not be applied due to activation stack overflow
                         assertThat(tracer.getActive()).isEqualTo(child1);
-                        ggc.end();
+                        Span ggc = grandchild1.createSpan();
+                        try (Scope ggcScope = ggc.activateInScope()) {
+                            assertThat(tracer.getActive()).isEqualTo(child1);
+                            ggc.end();
+                        }
+                        grandchild1.end();
                     }
-                    grandchild1.end();
+                    assertThat(tracer.getActive()).isEqualTo(child1);
+                    child1.end();
                 }
-                assertThat(tracer.getActive()).isEqualTo(child1);
-                child1.end();
-            }
-            assertThat(tracer.getActive()).isEqualTo(transaction);
-            Span child2 = transaction.createSpan();
-            try (Scope childScope = child2.activateInScope()) {
-                assertThat(tracer.getActive()).isEqualTo(child2);
-                Span grandchild2 = child2.createSpan();
-                try (Scope grandchildScope = grandchild2.activateInScope()) {
-                    // latter activation should not be applied due to activation stack overflow
+                assertThat(tracer.getActive()).isEqualTo(transaction);
+                Span child2 = transaction.createSpan();
+                try (Scope childScope = child2.activateInScope()) {
+                    assertThat(tracer.getActive()).isEqualTo(child2);
+                    Span grandchild2 = child2.createSpan();
+                    try (Scope grandchildScope = grandchild2.activateInScope()) {
+                        // latter activation should not be applied due to activation stack overflow
+                        assertThat(tracer.getActive()).isEqualTo(child2);
+                        grandchild2.end();
+                    }
                     assertThat(tracer.getActive()).isEqualTo(child2);
-                    grandchild2.end();
+                    child2.end();
                 }
-                assertThat(tracer.getActive()).isEqualTo(child2);
-                child2.end();
+                assertThat(tracer.getActive()).isEqualTo(transaction);
+                transaction.end();
             }
-            assertThat(tracer.getActive()).isEqualTo(transaction);
-            transaction.end();
-        }
+        }, tracer, ElasticApmTracer.ACTIVATION_STACK_BASE_SIZE);
         assertThat(tracer.getActive()).isNull();
         assertThat(reporter.getTransactions()).hasSize(1);
         assertThat(reporter.getSpans()).hasSize(2);
     }
 
+    private void doWithNestedBaggageActivations(Runnable r, Tracer tracer, int nestedCount) {
+        if (nestedCount == 0) {
+            r.run();
+            return;
+        }
+        BaggageContext baggageContext = tracer.currentContext().withUpdatedBaggage().buildContext();
+        try (Scope scope = baggageContext.activateInScope()) {
+            doWithNestedBaggageActivations(r, tracer, nestedCount - 1);
+        }
+    }
+
     @Test
     void testPause() {
         tracerImpl.pause();