improve: Make MicrometerMetrics extendable

[Donnerbart]

Partially fixes #2884

As mentioned in the issue:

I think implementing a custom metrics interface, or extending the current implementation with micrometer, should do the job

The latter is not possible due to the private constructor and private Cleaner implementations. This is an attempt to make the MicrometerMetrics class extendable without exposing all Cleaner implementations.

[Copilot]

Pull Request Overview

This PR makes the MicrometerMetrics class extendable by exposing previously private constructor and cleaner implementations. It introduces a builder pattern for cleaners and allows custom implementations to extend the base functionality.

• Exposes the MicrometerMetrics constructor as protected instead of private
• Introduces public CleanerBuilder and CleanerType enum for configurable cleaner creation
• Refactors existing builder classes to use the new CleanerBuilder internally

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
MicrometerMetrics.java	Main implementation changes - exposes constructor, adds CleanerBuilder and CleanerType enum, refactors existing builders
CleanerBuilderTest.java	New test file for the CleanerBuilder functionality
*WithCustomImplementationIT.java	New test files demonstrating custom extension capability
AbstractMicrometerMetricsTestFixture.java	Changed TestSimpleMeterRegistry visibility to protected
DelayedMetricsCleaningOnDeleteIT.java	Changed testDelay field visibility to protected
DefaultBehaviorIT.java, NoPerResourceCollectionIT.java	Added blank lines for formatting

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[metacosm]

Does this need to be exposed?

[Donnerbart]

The problem is that the constructor needs a Cleaner implementation:

protected MicrometerMetrics(
      MeterRegistry registry, Cleaner cleaner, boolean collectingPerResourceMetrics) {

So how can we re-use the existing implementations (beside the static Cleaner.NOOP)?

I didn't want to make them all public, so I tried to hide them behind that new CleanerBuilder.

[metacosm]

I would rather not expose the type if possible but rather switch the type internally based on whether the user sets the number of cleaning threads or the delay, which would switch the implementation rather than throwing an exception if the type is not amenable to set these values.

[Donnerbart]

That might actually work, yes! With that we might not need the types at all. The build() method could just check if a delay parameter is set, otherwise use Cleaner.NOOP. The DefaultCleaner is actually not directly used and exposed right now at all, so no need to change that.

[Donnerbart]

I've updated the PR to get rid of the CleanerType and derive the type dynamically from the set builder parameters.

[Donnerbart]

My main motivation to extend the MicrometerMetrics class is to avoid using a delegate pattern. This is how we've implemented this right now:

public class CustomResourceMetrics implements Metrics {

  private final @NotNull Metrics delegate;

  @Override
  public void controllerRegistered(final @NotNull Controller<? extends HasMetadata> controller) {
    delegate.controllerRegistered(controller);
    ...
  }

  @Override
  public void reconcileCustomResource(
          final @NotNull HasMetadata resource,
          final @NotNull RetryInfo retryInfo,
          final @NotNull Map<String, Object> metadata) {
      delegate.reconcileCustomResource(resource, retryInfo, metadata);
  }
}

This works fine, but we need to implement all methods of Metrics to preserve the JOSDK provided metrics. And if there would ever be a new default method in Metrics that is implemented in MicrometerMetrics, it would silently break something.

An alternative approach would be to allow multiple Metrics implementations. Then we could just configure the MicrometerMetrics as they are and our custom metrics together. But this might actually break public APIs, especially the getter signatures:

public class ConfigurationServiceOverrider {

  private List<Metrics> metrics;

  public ConfigurationServiceOverrider withMetrics(Metrics metrics) {
    return withMetrics(List.of(metrics));
  }
  
  public ConfigurationServiceOverrider withMetrics(List<Metrics> metrics) {
    this.metrics = metrics;
    return this;
  }
  
  public ConfigurationService build() {
    return new BaseConfigurationService(original.getVersion(), cloner, client) {
      @Override
      public List<Metrics> getMetrics() {
        return metrics != null ? metrics : original.getMetrics();
      }
   }
}

public interface ConfigurationService {

  default List<Metrics> getMetrics() {
    return List.of(Metrics.NOOP);
  }
}

[metacosm]

An alternative approach would be to allow multiple Metrics implementations. Then we could just configure the MicrometerMetrics as they are and our custom metrics together. But this might actually break public APIs, especially the getter signatures:

public class ConfigurationServiceOverrider {

  private List<Metrics> metrics;

  public ConfigurationServiceOverrider withMetrics(Metrics metrics) {
    return withMetrics(List.of(metrics));
  }
  
  public ConfigurationServiceOverrider withMetrics(List<Metrics> metrics) {
    this.metrics = metrics;
    return this;
  }
  
  public ConfigurationService build() {
    return new BaseConfigurationService(original.getVersion(), cloner, client) {
      @Override
      public List<Metrics> getMetrics() {
        return metrics != null ? metrics : original.getMetrics();
      }
   }
}

public interface ConfigurationService {

  default List<Metrics> getMetrics() {
    return List.of(Metrics.NOOP);
  }
}

Alternatively, one could implement an aggregate Metrics implementation that would allow to register multiple Metrics implementations and would then dispatch to all registered implementations for each method call. This would be similar to the delegate pattern I guess but a little bit more flexible.

For that matter, such an aggregate implementation could even be provided by JOSDK.

[Donnerbart]

Alternatively, one could implement an aggregate Metrics implementation that would allow to register multiple Metrics implementations and would then dispatch to all registered implementations for each method call. This would be similar to the delegate pattern I guess but a little bit more flexible.

I think that might actually be the smartest approach with the least impact to the existing classes and APIs! I'm happy to rewrite this PR or create an alternative one with that approach.

[metacosm]

Alternatively, one could implement an aggregate Metrics implementation that would allow to register multiple Metrics implementations and would then dispatch to all registered implementations for each method call. This would be similar to the delegate pattern I guess but a little bit more flexible.

I think that might actually be the smartest approach with the least impact to the existing classes and APIs! I'm happy to rewrite this PR or create an alternative one with that approach.

Whatever you think is more appropriate but I do think that this approach would be better, indeed. What do you think @csviri, @xstefank?