Skip to content

Commit

Permalink
GH-477: Fix MetricsRetryListener.close() for concurrent calls
Browse files Browse the repository at this point in the history
Fixes: #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()`
  • Loading branch information
artembilan committed Nov 22, 2024
1 parent 78bd8d2 commit f92c90a
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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");
}

/**
Expand Down Expand Up @@ -122,7 +119,11 @@ public <T, E extends Throwable> void close(RetryContext context, RetryCallback<T
.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));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -27,6 +29,7 @@
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;
Expand All @@ -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",
Expand All @@ -70,6 +83,8 @@ void metricsAreCollectedForRetryable() {
"3", "exception", "RetryException"))
.timer()
.count()).isEqualTo(1);

executor.destroy();
}

@Configuration(proxyBeanMethods = false)
Expand Down

0 comments on commit f92c90a

Please sign in to comment.