Skip to content

Fix off-by-one error in tests which effectively disabled recycling validation #3357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 12, 2023
Merged

Fix off-by-one error in tests which effectively disabled recycling validation #3357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93ec02f
Fix off-by-one error which effectively disabled recycling validation
JonasKunz Oct 12, 2023
9f21747
Disable recycling validation for profiler tests
JonasKunz Oct 12, 2023
4058159
Merge branch 'main' into recycling-verification-fix
mergify[bot] Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions apm-agent-core/src/test/java/co/elastic/apm/agent/MockTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand All @@ -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();

Expand All @@ -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();

Expand All @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion apm-agent-core/src/test/java/co/elastic/apm/agent/objectpool/TestObjectPoolFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public void checkAllPooledObjectsHaveBeenRecycled() {
// silently ignored
}
}
} while (retry-- > 0 && hasSomethingLeft);
} while (--retry > 0 && hasSomethingLeft);
Copy link
Contributor Author

@JonasKunz JonasKunz Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The postfix decrement caused retry to become -1 at the end of the loop, effectively causing the if condition below to always evaluate to false.


if (retry == 0 && hasSomethingLeft) {
for (BookkeeperObjectPool<?> pool : createdPools) {
Expand Down
2 changes: 1 addition & 1 deletion apm-agent-core/src/test/java/co/elastic/apm/agent/report/PartialTransactionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ public void setup() throws MalformedURLException {

@AfterEach
public void cleanup() {
reporter.reset();
objectPoolFactory.checkAllPooledObjectsHaveBeenRecycled();
reporter.resetWithoutRecycling();
objectPoolFactory.reset();
}

Expand Down
3 changes: 2 additions & 1 deletion ...nt-plugins/apm-api-plugin/src/test/java/co/elastic/apm/api/AnnotationInheritanceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions .../src/test/java/co/elastic/apm/agent/httpclient/AbstractHttpClientInstrumentationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
}

Expand Down
5 changes: 3 additions & 2 deletions .../src/test/java/co/elastic/apm/agent/loginstr/correlation/CorrelationIdMapAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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();
}

Expand Down
3 changes: 1 addition & 2 deletions ...apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeSpanifyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion ...lugins/apm-profiling-plugin/src/test/java/co/elastic/apm/agent/profiler/CallTreeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down