From 36acd2396025ccd975367cc6bd225c892bd5baa5 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 12 Oct 2023 17:38:30 +0200 Subject: [PATCH] Fix off-by-one error in tests which effectively disabled recycling validation (#3357) * Fix off-by-one error which effectively disabled recycling validation * Disable recycling validation for profiler tests --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- .../java/co/elastic/apm/agent/MockTracer.java | 25 ++++++++++++++----- .../objectpool/TestObjectPoolFactory.java | 2 +- .../agent/report/PartialTransactionTest.java | 2 +- .../apm/api/AnnotationInheritanceTest.java | 3 ++- ...AbstractHttpClientInstrumentationTest.java | 1 + .../CorrelationIdMapAdapterTest.java | 5 ++-- .../agent/profiler/CallTreeSpanifyTest.java | 3 +-- .../apm/agent/profiler/CallTreeTest.java | 2 +- 8 files changed, 29 insertions(+), 14 deletions(-) diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockTracer.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockTracer.java index f4644df753..7a955207f7 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/MockTracer.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/MockTracer.java @@ -26,6 +26,7 @@ import co.elastic.apm.agent.report.ApmServerClient; import co.elastic.apm.agent.report.Reporter; import co.elastic.apm.agent.common.util.Version; +import org.mockito.CheckReturnValue; import org.stagemonitor.configuration.ConfigurationRegistry; import static org.mockito.ArgumentMatchers.any; @@ -51,11 +52,15 @@ public static ElasticApmTracer createRealTracer(Reporter reporter) { return createRealTracer(reporter, SpyConfiguration.createSpyConfig()); } + public static ElasticApmTracer createRealTracer(Reporter reporter, ConfigurationRegistry config) { + return createRealTracer(reporter, config, true); + } + /** * Creates a real tracer with a given reporter and a mock configuration which returns default values which can be customized by mocking * the configuration. */ - public static ElasticApmTracer createRealTracer(Reporter reporter, ConfigurationRegistry config) { + public static ElasticApmTracer createRealTracer(Reporter reporter, ConfigurationRegistry config, boolean checkRecycling) { // use an object pool that does bookkeeping to allow for extra usage checks TestObjectPoolFactory objectPoolFactory = new TestObjectPoolFactory(); @@ -72,8 +77,10 @@ public static ElasticApmTracer createRealTracer(Reporter reporter, Configuration ((MockReporter) reporter).assertRecycledAfterDecrementingReferences(); } - // checking proper object pool usage using tracer lifecycle events - objectPoolFactory.checkAllPooledObjectsHaveBeenRecycled(); + if (checkRecycling) { + // checking proper object pool usage using tracer lifecycle events + objectPoolFactory.checkAllPooledObjectsHaveBeenRecycled(); + } })) .build(); @@ -91,6 +98,10 @@ public static ElasticApmTracer createRealTracer(Reporter reporter, Configuration * values that can be customized by mocking the configuration. */ public static synchronized MockInstrumentationSetup createMockInstrumentationSetup(ConfigurationRegistry configRegistry) { + return createMockInstrumentationSetup(configRegistry, true); + } + + public static synchronized MockInstrumentationSetup createMockInstrumentationSetup(ConfigurationRegistry configRegistry, boolean checkRecycling) { // use an object pool that does bookkeeping to allow for extra usage checks TestObjectPoolFactory objectPoolFactory = new TestObjectPoolFactory(); @@ -106,9 +117,11 @@ public static synchronized MockInstrumentationSetup createMockInstrumentationSet // is left behind .withObjectPoolFactory(objectPoolFactory) .withLifecycleListener(ClosableLifecycleListenerAdapter.of(() -> { - reporter.assertRecycledAfterDecrementingReferences(); - // checking proper object pool usage using tracer lifecycle events - objectPoolFactory.checkAllPooledObjectsHaveBeenRecycled(); + if (checkRecycling) { + reporter.assertRecycledAfterDecrementingReferences(); + // checking proper object pool usage using tracer lifecycle events + objectPoolFactory.checkAllPooledObjectsHaveBeenRecycled(); + } })) .buildAndStart(); return new MockInstrumentationSetup( diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/objectpool/TestObjectPoolFactory.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/objectpool/TestObjectPoolFactory.java index eb5b121e4f..72a2bfc5b3 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/objectpool/TestObjectPoolFactory.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/objectpool/TestObjectPoolFactory.java @@ -79,7 +79,7 @@ public void checkAllPooledObjectsHaveBeenRecycled() { // silently ignored } } - } while (retry-- > 0 && hasSomethingLeft); + } while (--retry > 0 && hasSomethingLeft); if (retry == 0 && hasSomethingLeft) { for (BookkeeperObjectPool pool : createdPools) { diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/PartialTransactionTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/PartialTransactionTest.java index 65420c3f89..1b8a6ec486 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/agent/report/PartialTransactionTest.java +++ b/apm-agent-core/src/test/java/co/elastic/apm/agent/report/PartialTransactionTest.java @@ -88,8 +88,8 @@ public void setup() throws MalformedURLException { @AfterEach public void cleanup() { + reporter.reset(); objectPoolFactory.checkAllPooledObjectsHaveBeenRecycled(); - reporter.resetWithoutRecycling(); objectPoolFactory.reset(); } diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java index 61cb5084c4..902fea1c74 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java @@ -22,6 +22,7 @@ import co.elastic.apm.agent.MockTracer; import co.elastic.apm.agent.bci.ElasticApmAgent; import co.elastic.apm.agent.configuration.CoreConfiguration; +import co.elastic.apm.agent.configuration.SpyConfiguration; import co.elastic.apm.agent.impl.ElasticApmTracer; import net.bytebuddy.agent.ByteBuddyAgent; import org.junit.jupiter.api.AfterAll; @@ -49,7 +50,7 @@ void cleanup() { } private void init(boolean annotationInheritanceEnabled) { - MockTracer.MockInstrumentationSetup mockInstrumentationSetup = MockTracer.createMockInstrumentationSetup(); + MockTracer.MockInstrumentationSetup mockInstrumentationSetup = MockTracer.createMockInstrumentationSetup(SpyConfiguration.createSpyConfig(), false); tracer = mockInstrumentationSetup.getTracer(); doReturn(annotationInheritanceEnabled).when(tracer.getConfig(CoreConfiguration.class)).isEnablePublicApiAnnotationInheritance(); reporter = mockInstrumentationSetup.getReporter(); diff --git a/apm-agent-plugins/apm-httpclient-core/src/test/java/co/elastic/apm/agent/httpclient/AbstractHttpClientInstrumentationTest.java b/apm-agent-plugins/apm-httpclient-core/src/test/java/co/elastic/apm/agent/httpclient/AbstractHttpClientInstrumentationTest.java index bbd3139d37..8bfcd27ecc 100644 --- a/apm-agent-plugins/apm-httpclient-core/src/test/java/co/elastic/apm/agent/httpclient/AbstractHttpClientInstrumentationTest.java +++ b/apm-agent-plugins/apm-httpclient-core/src/test/java/co/elastic/apm/agent/httpclient/AbstractHttpClientInstrumentationTest.java @@ -401,6 +401,7 @@ private void verifyTraceContextHeaders(Span span, String path) { assertThat(transaction).isNotNull(); assertThat(transaction.getTraceContext().getTraceId()).isEqualTo(span.getTraceContext().getTraceId()); assertThat(transaction.getTraceContext().getParentId()).isEqualTo(span.getTraceContext().getId()); + transaction.decrementReferences(); //recycle transaction without reporting }); } diff --git a/apm-agent-plugins/apm-logging-plugin/apm-logging-plugin-common/src/test/java/co/elastic/apm/agent/loginstr/correlation/CorrelationIdMapAdapterTest.java b/apm-agent-plugins/apm-logging-plugin/apm-logging-plugin-common/src/test/java/co/elastic/apm/agent/loginstr/correlation/CorrelationIdMapAdapterTest.java index a456400bac..6b7e3037fe 100644 --- a/apm-agent-plugins/apm-logging-plugin/apm-logging-plugin-common/src/test/java/co/elastic/apm/agent/loginstr/correlation/CorrelationIdMapAdapterTest.java +++ b/apm-agent-plugins/apm-logging-plugin/apm-logging-plugin-common/src/test/java/co/elastic/apm/agent/loginstr/correlation/CorrelationIdMapAdapterTest.java @@ -57,7 +57,7 @@ void testTransactionContext() { try (Scope scope = transaction.activateInScope()) { assertThat(CorrelationIdMapAdapter.get()).containsOnlyKeys("trace.id", "transaction.id"); } finally { - transaction.end(); + transaction.decrementReferences(); //recycle without reporting } assertThat(CorrelationIdMapAdapter.get()).isEmpty(); } @@ -70,8 +70,9 @@ void testSpanContext() { assertThat(CorrelationIdMapAdapter.get()).containsOnlyKeys("trace.id", "transaction.id"); } finally { span.end(); + span.decrementReferences(); } - transaction.end(); + transaction.decrementReferences(); assertThat(CorrelationIdMapAdapter.get()).isEmpty(); } diff --git a/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeSpanifyTest.java b/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeSpanifyTest.java index 5481703f2a..aea204c257 100644 --- a/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeSpanifyTest.java +++ b/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeSpanifyTest.java @@ -53,8 +53,7 @@ void setUp() { ConfigurationRegistry config = SpyConfiguration.createSpyConfig(); // disable scheduled profiling to not interfere with this test doReturn(false).when(config.getConfig(ProfilingConfiguration.class)).isProfilingEnabled(); - tracer = MockTracer.createRealTracer(reporter, config); - tracer = MockTracer.createRealTracer(reporter); + tracer = MockTracer.createRealTracer(reporter, config, false); } @AfterEach diff --git a/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeTest.java b/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeTest.java index b256ecdfdc..b18d86ecdd 100644 --- a/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeTest.java +++ b/apm-agent-plugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeTest.java @@ -73,7 +73,7 @@ void setUp() { // disable scheduled profiling to not interfere with this test profilerConfig = config.getConfig(ProfilingConfiguration.class); doReturn(true).when(profilerConfig).isProfilingEnabled(); - tracer = MockTracer.createRealTracer(reporter, config); + tracer = MockTracer.createRealTracer(reporter, config, false); } @AfterEach