Feedback, switch to ScheduledExecutorService for registrar

micrometer-meter-provider/README.md

@@ -40,8 +40,7 @@ MeterRegistry meterRegistry = ...;
 
 // create the meter provider
 MeterProvider meterProvider = MicrometerMeterProvider.builder(meterRegistry)
-    .setCallbackRegistrar(TimerCallbackRegistrar.builder()
-        .setDelay(Duration.ofSeconds(10L))
+    .setCallbackRegistrar(ScheduledCallbackRegistrar.builder()
         .setPeriod(Duration.ofSeconds(10L))
         .build())
     .build();

micrometer-meter-provider/src/main/java/io/opentelemetry/contrib/metrics/micrometer/ScheduledCallbackRegistrar.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.contrib.metrics.micrometer;
+
+import java.util.Objects;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * An implementation of {@link CallbackRegistrar} that uses a {@link ScheduledExecutorService} to
+ * execute the registered callbacks on the specified schedule.
+ */
+public final class ScheduledCallbackRegistrar implements CallbackRegistrar {
+  private final ScheduledExecutorService scheduledExecutorService;
+  private final long period;
+  private final TimeUnit timeUnit;
+
+  ScheduledCallbackRegistrar(
+      ScheduledExecutorService scheduledExecutorService, long period, TimeUnit timeUnit) {
+    this.scheduledExecutorService = scheduledExecutorService;
+    this.period = period;
+    this.timeUnit = timeUnit;
+  }
+
+  public static ScheduledCallbackRegistrarBuilder builder(
+      ScheduledExecutorService scheduledExecutorService) {
+    Objects.requireNonNull(scheduledExecutorService, "scheduledExecutorService");
+    return new ScheduledCallbackRegistrarBuilder(scheduledExecutorService);
+  }
+
+  @Override
+  public CallbackRegistration registerCallback(Runnable callback) {
+    if (callback != null) {
+      ScheduledFuture<?> future =
+          scheduledExecutorService.scheduleAtFixedRate(callback, period, period, timeUnit);
+      return () -> future.cancel(false);
+    } else {
+      return () -> {};
+    }
+  }
+
+  @Override
+  public void close() {
+    scheduledExecutorService.shutdown();
+  }
+}

micrometer-meter-provider/src/main/java/io/opentelemetry/contrib/metrics/micrometer/ScheduledCallbackRegistrarBuilder.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.contrib.metrics.micrometer;
+
+import java.time.Duration;
+import java.util.Objects;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+/** Builder utility class for creating instances of {@link ScheduledCallbackRegistrar}. */
+public final class ScheduledCallbackRegistrarBuilder {
+  private final ScheduledExecutorService scheduledExecutorService;
+  private long period;
+  private TimeUnit timeUnit;
+
+  ScheduledCallbackRegistrarBuilder(ScheduledExecutorService scheduledExecutorService) {
+    this.scheduledExecutorService = scheduledExecutorService;
+    this.period = 1L;
+    this.timeUnit = TimeUnit.SECONDS;
+  }
+
+  /** Sets the period between successive executions of each registered callback */
+  public ScheduledCallbackRegistrarBuilder setPeriod(long period, TimeUnit unit) {
+    Objects.requireNonNull(unit, "unit");
+    this.period = period;
+    this.timeUnit = unit;
+    return this;
+  }
+
+  /** Sets the period between successive executions of each registered callback */
+  public ScheduledCallbackRegistrarBuilder setPeriod(Duration period) {
+    Objects.requireNonNull(period, "period");
+    this.period = period.toMillis();
+    this.timeUnit = TimeUnit.MILLISECONDS;
+    return this;
+  }
+
+  /**
+   * Constructs a new instance of the {@link CallbackRegistrar} based on the builder's values.
+   *
+   * @return a new instance.
+   */
+  public CallbackRegistrar build() {
+    return new ScheduledCallbackRegistrar(scheduledExecutorService, period, timeUnit);
+  }
+}

micrometer-meter-provider/src/test/java/io/opentelemetry/contrib/metrics/micrometer/ScheduledCallbackRegistrarTest.java

@@ -0,0 +1,119 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.contrib.metrics.micrometer;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+
+import java.time.Duration;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class ScheduledCallbackRegistrarTest {
+
+  @Mock ScheduledExecutorService scheduledExecutorService;
+
+  @Mock Runnable callback;
+
+  @Mock ScheduledFuture<?> scheduledFuture;
+
+  @Test
+  void schedulesCallback() {
+    doReturn(scheduledFuture)
+        .when(scheduledExecutorService)
+        .scheduleAtFixedRate(any(), anyLong(), anyLong(), any());
+
+    try (CallbackRegistrar underTest =
+        ScheduledCallbackRegistrar.builder(scheduledExecutorService).build()) {
+
+      underTest.registerCallback(callback);
+
+      verify(scheduledExecutorService).scheduleAtFixedRate(callback, 1L, 1L, TimeUnit.SECONDS);
+    }
+  }
+
+  @Test
+  @SuppressWarnings("PreferJavaTimeOverload")
+  void schedulesCallbackWithPeriod() {
+    doReturn(scheduledFuture)
+        .when(scheduledExecutorService)
+        .scheduleAtFixedRate(any(), anyLong(), anyLong(), any());
+
+    try (CallbackRegistrar underTest =
+        ScheduledCallbackRegistrar.builder(scheduledExecutorService)
+            .setPeriod(10L, TimeUnit.SECONDS)
+            .build()) {
+
+      underTest.registerCallback(callback);
+
+      verify(scheduledExecutorService).scheduleAtFixedRate(callback, 10L, 10L, TimeUnit.SECONDS);
+    }
+  }
+
+  @Test
+  void schedulesCallbackWithPeriodDuration() {
+    doReturn(scheduledFuture)
+        .when(scheduledExecutorService)
+        .scheduleAtFixedRate(any(), anyLong(), anyLong(), any());
+
+    try (CallbackRegistrar underTest =
+        ScheduledCallbackRegistrar.builder(scheduledExecutorService)
+            .setPeriod(Duration.ofSeconds(10L))
+            .build()) {
+
+      underTest.registerCallback(callback);
+
+      verify(scheduledExecutorService)
+          .scheduleAtFixedRate(callback, 10_000L, 10_000L, TimeUnit.MILLISECONDS);
+    }
+  }
+
+  @Test
+  void handlesNullCallback() {
+    try (CallbackRegistrar underTest =
+        ScheduledCallbackRegistrar.builder(scheduledExecutorService).build()) {
+
+      underTest.registerCallback(null);
+
+      verifyNoInteractions(scheduledExecutorService);
+    }
+  }
+
+  @Test
+  void closeCancelsScheduledFuture() {
+    doReturn(scheduledFuture)
+        .when(scheduledExecutorService)
+        .scheduleAtFixedRate(any(), anyLong(), anyLong(), any());
+
+    try (CallbackRegistrar underTest =
+        ScheduledCallbackRegistrar.builder(scheduledExecutorService).build()) {
+
+      CallbackRegistration callbackRegistration = underTest.registerCallback(callback);
+      verify(scheduledExecutorService).scheduleAtFixedRate(callback, 1L, 1L, TimeUnit.SECONDS);
+
+      callbackRegistration.close();
+      verify(scheduledFuture).cancel(false);
+    }
+  }
+
+  @Test
+  void closeShutsDownScheduledExecutorService() {
+    CallbackRegistrar underTest =
+        ScheduledCallbackRegistrar.builder(scheduledExecutorService).build();
+
+    underTest.close();
+    verify(scheduledExecutorService).shutdown();
+  }
+}