Spring/spring-retry
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

GH-477: Fix MetricsRetryListener.close() for concurrent calls

Browse Source 
Fixes: https://github.com/spring-projects/spring-retry/issues/477

The `Timer.Builder` from Micrometer does not create a new `Builder` instance for its `tags()` call.
So, using shared `Timer.Builder` is not OK when it can be used from concurrent calls.

* Remove shared `retryMeterProvider` property and use fresh `Timer.Builder` instance in the `MetricsRetryListener.close()`
This commit is contained in:
Artem Bilan
2024-11-22 10:17:59 -05:00
parent 78bd8d22ea
commit f92c90a2a5
2 changed files with 24 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
src/main/java/org/springframework/retry/support/MetricsRetryListener.java
View File

@@ -66,8 +66,6 @@ public class MetricsRetryListener implements RetryListener {
private final Map<RetryContext, Timer.Sample> retryContextToSample = Collections
.synchronizedMap(new IdentityHashMap<>());
private final Timer.Builder retryMeterProvider;
private Tags customTags = Tags.empty();
private Function<RetryContext, Iterable<Tag>> customTagsProvider = retryContext -> Tags.empty();
@@ -79,7 +77,6 @@ public class MetricsRetryListener implements RetryListener {
public MetricsRetryListener(MeterRegistry meterRegistry) {
Assert.notNull(meterRegistry, "'meterRegistry' must not be null");
this.meterRegistry = meterRegistry;
this.retryMeterProvider = Timer.builder(TIMER_NAME).description("Metrics for Spring RetryTemplate");
}
/**
@@ -122,7 +119,11 @@ public class MetricsRetryListener implements RetryListener {
.and(this.customTagsProvider.apply(context))
.and("exception", throwable != null ? throwable.getClass().getSimpleName() : "none");
sample.stop(this.retryMeterProvider.tags(retryTags).register(this.meterRegistry));
Timer.Builder timeBuilder = Timer.builder(TIMER_NAME)
.description("Metrics for Spring RetryTemplate")
.tags(retryTags);
sample.stop(timeBuilder.register(this.meterRegistry));
}
}

23
src/test/java/org/springframework/retry/support/RetryMetricsTests.java
View File

@@ -16,6 +16,8 @@
package org.springframework.retry.support;
import java.util.concurrent.CompletableFuture;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Tags;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
@@ -27,6 +29,7 @@ import org.springframework.context.annotation.Configuration;
import org.springframework.retry.RetryException;
import org.springframework.retry.annotation.EnableRetry;
import org.springframework.retry.annotation.Retryable;
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor;
import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
import static org.assertj.core.api.Assertions.assertThat;
@@ -48,10 +51,20 @@ public class RetryMetricsTests {
@Test
void metricsAreCollectedForRetryable() {
assertThatNoException().isThrownBy(this.service::service1);
assertThatNoException().isThrownBy(this.service::service1);
assertThatNoException().isThrownBy(this.service::service2);
assertThatExceptionOfType(RetryException.class).isThrownBy(this.service::service3);
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
executor.setCorePoolSize(4);
executor.afterPropertiesSet();
CompletableFuture<?> future1 = executor
.submitCompletable(() -> assertThatNoException().isThrownBy(this.service::service1));
CompletableFuture<?> future2 = executor
.submitCompletable(() -> assertThatNoException().isThrownBy(this.service::service1));
CompletableFuture<?> future3 = executor
.submitCompletable(() -> assertThatNoException().isThrownBy(this.service::service2));
CompletableFuture<?> future4 = executor.submitCompletable(
() -> assertThatExceptionOfType(RetryException.class).isThrownBy(this.service::service3));
CompletableFuture.allOf(future1, future2, future3, future4).join();
assertThat(this.meterRegistry.get(MetricsRetryListener.TIMER_NAME)
.tags(Tags.of("name", "org.springframework.retry.support.RetryMetricsTests$Service.service1", "retry.count",
@@ -70,6 +83,8 @@ public class RetryMetricsTests {
"3", "exception", "RetryException"))
.timer()
.count()).isEqualTo(1);
executor.destroy();
}
@Configuration(proxyBeanMethods = false)