Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -315,11 +314,9 @@ 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);
collectingPerResourceMetrics = true;
}

/**
Expand All @@ -329,7 +326,7 @@ private PerResourceCollectingMicrometerMetricsBuilder(MeterRegistry registry) {
*/
public PerResourceCollectingMicrometerMetricsBuilder withCleaningThreadNumber(
int cleaningThreadsNumber) {
this.cleaningThreadsNumber = cleaningThreadsNumber <= 0 ? 1 : cleaningThreadsNumber;
cleanerBuilder.withCleaningThreadNumber(cleaningThreadsNumber);
return this;
}

Expand All @@ -343,30 +340,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 final 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);
}

Expand All @@ -381,11 +372,52 @@ public MicrometerMetricsBuilder notCollectingMetricsPerResource() {
}

public MicrometerMetrics build() {
return new MicrometerMetrics(registry, Cleaner.NOOP, collectingPerResourceMetrics);
return new MicrometerMetrics(registry, cleanerBuilder.build(), collectingPerResourceMetrics);
}
}

public static class CleanerBuilder {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private final MeterRegistry registry;
private int cleaningThreadsNumber = 0;
private int cleanUpDelayInSeconds = 0;

public CleanerBuilder(MeterRegistry registry) {
this.registry = registry;
}

/**
* @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
*/
public CleanerBuilder withCleaningThreadNumber(int cleaningThreadsNumber) {
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
*/
public CleanerBuilder withCleanUpDelayInSeconds(int cleanUpDelayInSeconds) {
this.cleanUpDelayInSeconds = Math.max(cleanUpDelayInSeconds, 1);
return this;
}

public Cleaner build() {
if (cleanUpDelayInSeconds > 0 || cleaningThreadsNumber > 0) {
return new DelayedCleaner(registry, cleanUpDelayInSeconds, cleaningThreadsNumber);
}
return Cleaner.NOOP;
}
}

interface Cleaner {
public interface Cleaner {
Cleaner NOOP = new Cleaner() {};

default void removeMetersFor(ResourceID resourceID) {}
Expand Down Expand Up @@ -444,5 +476,10 @@ public void removeMetersFor(ResourceID resourceID) {
metersCleaner.schedule(
() -> super.removeMetersFor(resourceID), cleanUpDelayInSeconds, TimeUnit.SECONDS);
}

// for testing purposes only
int getCleanUpDelayInSeconds() {
return cleanUpDelayInSeconds;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package io.javaoperatorsdk.operator.monitoring.micrometer;

import org.junit.jupiter.api.Test;

import io.javaoperatorsdk.operator.monitoring.micrometer.MicrometerMetrics.CleanerBuilder;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;

import static org.assertj.core.api.Assertions.assertThat;

class CleanerBuilderTest {

private final SimpleMeterRegistry registry = new SimpleMeterRegistry();

@Test
void test_defaultsToNoopCleaner() {
final var cleaner = new CleanerBuilder(registry).build();
assertThat(cleaner).isSameAs(MicrometerMetrics.Cleaner.NOOP);
}

@Test
void test_withCleaningThreadNumber() {
final var cleaner = new CleanerBuilder(registry).withCleaningThreadNumber(42).build();
assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(0);
}
}

@Test
void test_withCleaningThreadNumber_whenNegative_thenApplyDefault() {
final var cleaner = new CleanerBuilder(registry).withCleaningThreadNumber(42).build();
assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(0);
}
}

@Test
void test_withCleaningThreadNumber_whenZero_thenApplyDefault() {
final var cleaner = new CleanerBuilder(registry).withCleaningThreadNumber(0).build();
assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(0);
}
}

@Test
void test_withCleanUpDelayInSeconds() {
final var cleaner = new CleanerBuilder(registry).withCleanUpDelayInSeconds(23).build();
assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(23);
}
}

@Test
void test_withCleanUpDelayInSeconds_whenNegative_thenApplyDefault() {
final var cleaner = new CleanerBuilder(registry).withCleanUpDelayInSeconds(-23).build();
assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(1);
}
}

@Test
void test_withCleanUpDelayInSeconds_whenZero_thenApplyDefault() {
final var cleaner = new CleanerBuilder(registry).withCleanUpDelayInSeconds(0).build();
assertThat(cleaner).isInstanceOf(MicrometerMetrics.DelayedCleaner.class);
if (cleaner instanceof MicrometerMetrics.DelayedCleaner delayedCleaner) {
assertThat(delayedCleaner.getCleanUpDelayInSeconds()).isEqualTo(1);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
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)
.withCleanUpDelayInSeconds(testDelay)
.withCleaningThreadNumber(2)
.build(),
true);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
}