improve: Make MicrometerMetrics extendable

Signed-off-by: David Sondermann <[email protected]>

Author: Donnerbart

micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java

@@ -96,14 +96,13 @@ public static MicrometerMetricsBuilder newMicrometerMetricsBuilder(MeterRegistry
 
   /**
    * Creates a micrometer-based Metrics implementation that cleans up {@link Meter}s associated with
-   * deleted resources as specified by the (possibly {@code null}) provided {@link Cleaner}
-   * instance.
+   * deleted resources as specified by the provided {@link Cleaner} instance.
    *
    * @param registry the {@link MeterRegistry} instance to use for metrics recording
    * @param cleaner the {@link Cleaner} to use
    * @param collectingPerResourceMetrics whether to collect per resource metrics
    */
-  private MicrometerMetrics(
+  protected MicrometerMetrics(
       MeterRegistry registry, Cleaner cleaner, boolean collectingPerResourceMetrics) {
     this.registry = registry;
     this.cleaner = cleaner;
@@ -315,11 +314,10 @@ protected Set<Meter.Id> recordedMeterIdsFor(ResourceID resourceID) {
   public static class PerResourceCollectingMicrometerMetricsBuilder
       extends MicrometerMetricsBuilder {
 
-    private int cleaningThreadsNumber;
-    private int cleanUpDelayInSeconds;
-
     private PerResourceCollectingMicrometerMetricsBuilder(MeterRegistry registry) {
       super(registry);
+      cleanerBuilder.withCleanerType(CleanerType.DELAYED);
+      collectingPerResourceMetrics = true;
     }
 
     /**
@@ -329,7 +327,7 @@ private PerResourceCollectingMicrometerMetricsBuilder(MeterRegistry registry) {
      */
     public PerResourceCollectingMicrometerMetricsBuilder withCleaningThreadNumber(
         int cleaningThreadsNumber) {
-      this.cleaningThreadsNumber = cleaningThreadsNumber <= 0 ? 1 : cleaningThreadsNumber;
+      cleanerBuilder.withCleaningThreadNumber(cleaningThreadsNumber);
       return this;
     }
 
@@ -343,30 +341,24 @@ public PerResourceCollectingMicrometerMetricsBuilder withCleaningThreadNumber(
      */
     public PerResourceCollectingMicrometerMetricsBuilder withCleanUpDelayInSeconds(
         int cleanUpDelayInSeconds) {
-      this.cleanUpDelayInSeconds = Math.max(cleanUpDelayInSeconds, 1);
+      cleanerBuilder.withCleanUpDelayInSeconds(cleanUpDelayInSeconds);
       return this;
     }
-
-    @Override
-    public MicrometerMetrics build() {
-      final var cleaner =
-          new DelayedCleaner(registry, cleanUpDelayInSeconds, cleaningThreadsNumber);
-      return new MicrometerMetrics(registry, cleaner, true);
-    }
   }
 
   public static class MicrometerMetricsBuilder {
     protected final MeterRegistry registry;
-    private boolean collectingPerResourceMetrics = true;
+    protected CleanerBuilder cleanerBuilder;
+    protected boolean collectingPerResourceMetrics = true;
 
     private MicrometerMetricsBuilder(MeterRegistry registry) {
       this.registry = registry;
+      this.cleanerBuilder = new CleanerBuilder(registry);
     }
 
     /** Configures the instance to collect metrics on a per-resource basis. */
     @SuppressWarnings("unused")
     public PerResourceCollectingMicrometerMetricsBuilder collectingMetricsPerResource() {
-      collectingPerResourceMetrics = true;
       return new PerResourceCollectingMicrometerMetricsBuilder(registry);
     }
 
@@ -381,11 +373,79 @@ public MicrometerMetricsBuilder notCollectingMetricsPerResource() {
     }
 
     public MicrometerMetrics build() {
-      return new MicrometerMetrics(registry, Cleaner.NOOP, collectingPerResourceMetrics);
+      return new MicrometerMetrics(registry, cleanerBuilder.build(), collectingPerResourceMetrics);
+    }
+  }
+
+  public static class CleanerBuilder {
+
+    private final MeterRegistry registry;
+    private CleanerType cleanerType = CleanerType.NOOP;
+    private int cleaningThreadsNumber = 0;
+    private int cleanUpDelayInSeconds = 0;
+
+    public CleanerBuilder(MeterRegistry registry) {
+      this.registry = registry;
+    }
+
+    /**
+     * @param cleanerType the type of cleaner to use, defaults to {@link CleanerType#NOOP} if not
+     *     specified
+     */
+    public CleanerBuilder withCleanerType(CleanerType cleanerType) {
+      this.cleanerType = cleanerType;
+      return this;
+    }
+
+    /**
+     * @param cleaningThreadsNumber the maximal number of threads that can be assigned to the
+     *     removal of {@link Meter}s associated with deleted resources, defaults to 1 if not
+     *     specified or if the provided number is lesser or equal to 0
+     * @throws IllegalStateException if the cleaner type is not {@link CleanerType#DELAYED}
+     */
+    public CleanerBuilder withCleaningThreadNumber(int cleaningThreadsNumber) {
+      if (cleanerType != CleanerType.DELAYED) {
+        throw new IllegalStateException(
+            "Cleaning threads can only be configured when using the DELAYED cleaner type");
+      }
+      this.cleaningThreadsNumber = cleaningThreadsNumber <= 0 ? 1 : cleaningThreadsNumber;
+      return this;
     }
+
+    /**
+     * @param cleanUpDelayInSeconds the number of seconds to wait before {@link Meter}s are removed
+     *     for deleted resources, defaults to 1 (meaning meters will be removed one second after the
+     *     associated resource is deleted) if not specified or if the provided number is lesser than
+     *     0. Threading and the general interaction model of interacting with the API server means
+     *     that it's not possible to ensure that meters are immediately deleted in all cases so a
+     *     minimal delay of one second is always enforced
+     * @throws IllegalStateException if the cleaner type is not {@link CleanerType#DELAYED}
+     */
+    public CleanerBuilder withCleanUpDelayInSeconds(int cleanUpDelayInSeconds) {
+      if (cleanerType != CleanerType.DELAYED) {
+        throw new IllegalStateException(
+            "Clean up delay can only be configured when using the DELAYED cleaner type");
+      }
+      this.cleanUpDelayInSeconds = Math.max(cleanUpDelayInSeconds, 1);
+      return this;
+    }
+
+    public Cleaner build() {
+      return switch (cleanerType) {
+        case NOOP -> Cleaner.NOOP;
+        case DEFAULT -> new DefaultCleaner(registry);
+        case DELAYED -> new DelayedCleaner(registry, cleanUpDelayInSeconds, cleaningThreadsNumber);
+      };
+    }
+  }
+
+  public enum CleanerType {
+    NOOP,
+    DEFAULT,
+    DELAYED
   }
 
-  interface Cleaner {
+  public interface Cleaner {
     Cleaner NOOP = new Cleaner() {};
 
     default void removeMetersFor(ResourceID resourceID) {}
@@ -444,5 +504,10 @@ public void removeMetersFor(ResourceID resourceID) {
       metersCleaner.schedule(
           () -> super.removeMetersFor(resourceID), cleanUpDelayInSeconds, TimeUnit.SECONDS);
     }
+
+    // for testing purposes only
+    int getCleanUpDelayInSeconds() {
+      return cleanUpDelayInSeconds;
+    }
   }
 }

micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java

@@ -87,7 +87,7 @@ public DeleteControl cleanup(ConfigMap resource, Context<ConfigMap> context) {
     }
   }
 
-  static class TestSimpleMeterRegistry extends SimpleMeterRegistry {
+  protected static class TestSimpleMeterRegistry extends SimpleMeterRegistry {
     private final Set<Meter.Id> removed = new HashSet<>();
 
     @Override

micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/CleanerBuilderTest.java

@@ -0,0 +1,70 @@
+package io.javaoperatorsdk.operator.monitoring.micrometer;
+
+import org.junit.jupiter.api.Test;
+
+import io.javaoperatorsdk.operator.monitoring.micrometer.MicrometerMetrics.CleanerBuilder;
+import io.javaoperatorsdk.operator.monitoring.micrometer.MicrometerMetrics.CleanerType;
+import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
+
+import static org.assertj.core.api.Assertions.*;
+
+class CleanerBuilderTest {
+
+  private final SimpleMeterRegistry registry = new SimpleMeterRegistry();
+  private final CleanerBuilder cleanerBuilder = new CleanerBuilder(registry);
+
+  @Test
+  void test_withCleanerTypeNoop() {
+    final var cleaner = cleanerBuilder.withCleanerType(CleanerType.NOOP).build();
+    assertThat(cleaner).isSameAs(MicrometerMetrics.Cleaner.NOOP);
+  }
+
+  @Test
+  void test_withCleanerTypeDefault() {
+    final var cleaner = cleanerBuilder.withCleanerType(CleanerType.DEFAULT).build();
+    assertThat(cleaner).isInstanceOf(MicrometerMetrics.DefaultCleaner.class);
+  }
+
+  @Test
+  void test_withCleanerTypeDelayed() {
+    final var cleaner = cleanerBuilder.withCleanerType(CleanerType.DELAYED).build();
+    assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
+    if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
+      assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(0);
+    }
+  }
+
+  @Test
+  void test_withCleaningThreadNumber() {
+    final var cleaner =
+        cleanerBuilder.withCleanerType(CleanerType.DELAYED).withCleaningThreadNumber(42).build();
+    assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
+    if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
+      assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(0);
+    }
+  }
+
+  @Test
+  void test_withCleaningThreadNumber_whenTypeIsNoDelayed_thenThrowException() {
+    assertThatThrownBy(() -> cleanerBuilder.withCleaningThreadNumber(23))
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessage("Cleaning threads can only be configured when using the DELAYED cleaner type");
+  }
+
+  @Test
+  void test_withCleanUpDelayInSeconds() {
+    final var cleaner =
+        cleanerBuilder.withCleanerType(CleanerType.DELAYED).withCleanUpDelayInSeconds(23).build();
+    assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
+    if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
+      assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(23);
+    }
+  }
+
+  @Test
+  void test_withCleanUpDelayInSeconds_whenTypeIsNoDelayed_thenThrowException() {
+    assertThatThrownBy(() -> cleanerBuilder.withCleanUpDelayInSeconds(1))
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessage("Clean up delay can only be configured when using the DELAYED cleaner type");
+  }
+}

micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java

@@ -9,6 +9,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 public class DefaultBehaviorIT extends AbstractMicrometerMetricsTestFixture {
+
   @Override
   protected MicrometerMetrics getMetrics() {
     return MicrometerMetrics.newMicrometerMetricsBuilder(registry).build();

micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorWithCustomImplementationIT.java

@@ -0,0 +1,18 @@
+package io.javaoperatorsdk.operator.monitoring.micrometer;
+
+import io.micrometer.core.instrument.MeterRegistry;
+
+public class DefaultBehaviorWithCustomImplementationIT extends DefaultBehaviorIT {
+
+  @Override
+  protected MicrometerMetrics getMetrics() {
+    return new TestMetrics(registry);
+  }
+
+  private static class TestMetrics extends MicrometerMetrics {
+
+    private TestMetrics(MeterRegistry registry) {
+      super(registry, new CleanerBuilder(registry).build(), true);
+    }
+  }
+}

micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java

@@ -10,7 +10,7 @@
 
 public class DelayedMetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsTestFixture {
 
-  private static final int testDelay = 1;
+  protected static final int testDelay = 1;
 
   @Override
   protected MicrometerMetrics getMetrics() {

micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteWithCustomImplementationIT.java

@@ -0,0 +1,26 @@
+package io.javaoperatorsdk.operator.monitoring.micrometer;
+
+import io.micrometer.core.instrument.MeterRegistry;
+
+public class DelayedMetricsCleaningOnDeleteWithCustomImplementationIT
+    extends DelayedMetricsCleaningOnDeleteIT {
+
+  @Override
+  protected MicrometerMetrics getMetrics() {
+    return new TestMetrics(registry);
+  }
+
+  private static class TestMetrics extends MicrometerMetrics {
+
+    private TestMetrics(MeterRegistry registry) {
+      super(
+          registry,
+          new CleanerBuilder(registry)
+              .withCleanerType(CleanerType.DELAYED)
+              .withCleanUpDelayInSeconds(testDelay)
+              .withCleaningThreadNumber(2)
+              .build(),
+          true);
+    }
+  }
+}

micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java

@@ -9,6 +9,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 public class NoPerResourceCollectionIT extends AbstractMicrometerMetricsTestFixture {
+
   @Override
   protected MicrometerMetrics getMetrics() {
     return MicrometerMetrics.withoutPerResourceMetrics(registry);

micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionWithCustomImplementationIT.java

@@ -0,0 +1,18 @@
+package io.javaoperatorsdk.operator.monitoring.micrometer;
+
+import io.micrometer.core.instrument.MeterRegistry;
+
+public class NoPerResourceCollectionWithCustomImplementationIT extends NoPerResourceCollectionIT {
+
+  @Override
+  protected MicrometerMetrics getMetrics() {
+    return new TestMetrics(registry);
+  }
+
+  private static class TestMetrics extends MicrometerMetrics {
+
+    private TestMetrics(MeterRegistry registry) {
+      super(registry, new CleanerBuilder(registry).build(), false);
+    }
+  }
+}