From 4e5794f86017d7a7fdba47f8201fabb765443fa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 21 Feb 2023 16:31:13 +0100 Subject: [PATCH 01/29] feat: bounded cache for informers (#1718) --- pom.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pom.xml b/pom.xml index a0e56f1a55..7714d91476 100644 --- a/pom.xml +++ b/pom.xml @@ -196,6 +196,11 @@ mockwebserver ${okhttp.version} + + com.github.ben-manes.caffeine + caffeine + ${caffein.version} + com.github.ben-manes.caffeine caffeine From 6c5fafa8818ed02dd1a4f7c33b29fe63bbae95a6 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 3 Mar 2023 13:20:19 +0100 Subject: [PATCH 02/29] fix: typo caffein -> caffeine (#1795) --- pom.xml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pom.xml b/pom.xml index 7714d91476..a0e56f1a55 100644 --- a/pom.xml +++ b/pom.xml @@ -196,11 +196,6 @@ mockwebserver ${okhttp.version} - - com.github.ben-manes.caffeine - caffeine - ${caffein.version} - com.github.ben-manes.caffeine caffeine From 0ae01d1d227225dbcf9d1a88ab1f7fca8766de63 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 15 Mar 2023 17:30:37 +0100 Subject: [PATCH 03/29] feat: now possible to only output non-resource related metrics Fixes #1812. --- .../micrometer/MicrometerMetrics.java | 212 +++++++++++------- .../micrometer/MetricsCleaningOnDeleteIT.java | 3 +- 2 files changed, 137 insertions(+), 78 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 5b3a83a1c6..f28dea274e 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -30,9 +30,9 @@ public class MicrometerMetrics implements Metrics { private static final String RECONCILIATIONS = "reconciliations."; private static final String RECONCILIATIONS_EXECUTIONS = PREFIX + RECONCILIATIONS + "executions."; private static final String RECONCILIATIONS_QUEUE_SIZE = PREFIX + RECONCILIATIONS + "queue.size."; + private final boolean collectPerResourceMetrics; private final MeterRegistry registry; private final Map gauges = new ConcurrentHashMap<>(); - private final Map> metersPerResource = new ConcurrentHashMap<>(); private final Cleaner cleaner; /** @@ -42,43 +42,30 @@ public class MicrometerMetrics implements Metrics { * @param registry the {@link MeterRegistry} instance to use for metrics recording */ public MicrometerMetrics(MeterRegistry registry) { - this(registry, 0); + this(registry, Cleaner.NOOP); } - /** - * Creates a micrometer-based Metrics implementation that delays cleaning up {@link Meter}s - * associated with deleted resources by the specified amount of seconds, using a single thread for - * that process. - * - * @param registry the {@link MeterRegistry} instance to use for metrics recording - * @param cleanUpDelayInSeconds the number of seconds to wait before meters are removed for - * deleted resources - */ - public MicrometerMetrics(MeterRegistry registry, int cleanUpDelayInSeconds) { - this(registry, cleanUpDelayInSeconds, 1); + @SuppressWarnings("unused") + public static MicrometerMetrics withoutPerResourceMetrics(MeterRegistry registry) { + return new MicrometerMetrics(registry); + } + + public static MicrometerMetricsBuilder newMicrometerMetrics(MeterRegistry registry) { + return new MicrometerMetricsBuilder(registry); } /** - * Creates a micrometer-based Metrics implementation that delays cleaning up {@link Meter}s - * associated with deleted resources by the specified amount of seconds, using the specified - * (maximally) number of threads for that process. + * 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. * * @param registry the {@link MeterRegistry} instance to use for metrics recording - * @param cleanUpDelayInSeconds the number of seconds to wait before meters are removed for - * deleted resources - * @param cleaningThreadsNumber the number of threads to use for the cleaning process + * @param cleaner the {@link Cleaner} to use */ - public MicrometerMetrics(MeterRegistry registry, int cleanUpDelayInSeconds, - int cleaningThreadsNumber) { + private MicrometerMetrics(MeterRegistry registry, Cleaner cleaner) { this.registry = registry; - if (cleanUpDelayInSeconds < 0) { - cleaner = new NoDelayCleaner(); - } else { - cleaningThreadsNumber = - cleaningThreadsNumber <= 0 ? Runtime.getRuntime().availableProcessors() - : cleaningThreadsNumber; - cleaner = new DelayedCleaner(cleanUpDelayInSeconds, cleaningThreadsNumber); - } + this.cleaner = cleaner; + this.collectPerResourceMetrics = Cleaner.NOOP != cleaner; } @Override @@ -153,45 +140,53 @@ private static String getScope(ResourceID resourceID) { @Override public void receivedEvent(Event event, Map metadata) { - final String[] tags; - if (event instanceof ResourceEvent) { - tags = new String[] {"event", event.getClass().getSimpleName(), "action", - ((ResourceEvent) event).getAction().toString()}; - } else { - tags = new String[] {"event", event.getClass().getSimpleName()}; + if (collectPerResourceMetrics) { + final String[] tags; + if (event instanceof ResourceEvent) { + tags = new String[] {"event", event.getClass().getSimpleName(), "action", + ((ResourceEvent) event).getAction().toString()}; + } else { + tags = new String[] {"event", event.getClass().getSimpleName()}; + } + + incrementCounter(event.getRelatedCustomResourceID(), "events.received", + metadata, + tags); } - - incrementCounter(event.getRelatedCustomResourceID(), "events.received", - metadata, - tags); } @Override public void cleanupDoneFor(ResourceID resourceID, Map metadata) { - incrementCounter(resourceID, "events.delete", metadata); + if (collectPerResourceMetrics) { + incrementCounter(resourceID, "events.delete", metadata); - cleaner.removeMetersFor(resourceID); + cleaner.removeMetersFor(resourceID); + } } @Override public void reconcileCustomResource(HasMetadata resource, RetryInfo retryInfoNullable, Map metadata) { - Optional retryInfo = Optional.ofNullable(retryInfoNullable); - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "started", - metadata, - RECONCILIATIONS + "retries.number", - String.valueOf(retryInfo.map(RetryInfo::getAttemptCount).orElse(0)), - RECONCILIATIONS + "retries.last", - String.valueOf(retryInfo.map(RetryInfo::isLastAttempt).orElse(true))); - - var controllerQueueSize = - gauges.get(RECONCILIATIONS_QUEUE_SIZE + metadata.get(CONTROLLER_NAME)); - controllerQueueSize.incrementAndGet(); + if (collectPerResourceMetrics) { + Optional retryInfo = Optional.ofNullable(retryInfoNullable); + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "started", + metadata, + RECONCILIATIONS + "retries.number", + String.valueOf(retryInfo.map(RetryInfo::getAttemptCount).orElse(0)), + RECONCILIATIONS + "retries.last", + String.valueOf(retryInfo.map(RetryInfo::isLastAttempt).orElse(true))); + + var controllerQueueSize = + gauges.get(RECONCILIATIONS_QUEUE_SIZE + metadata.get(CONTROLLER_NAME)); + controllerQueueSize.incrementAndGet(); + } } @Override public void finishedReconciliation(HasMetadata resource, Map metadata) { - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "success", metadata); + if (collectPerResourceMetrics) { + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "success", metadata); + } } @Override @@ -215,15 +210,17 @@ public void reconciliationExecutionFinished(HasMetadata resource, Map metadata) { - var cause = exception.getCause(); - if (cause == null) { - cause = exception; - } else if (cause instanceof RuntimeException) { - cause = cause.getCause() != null ? cause.getCause() : cause; + if (collectPerResourceMetrics) { + var cause = exception.getCause(); + if (cause == null) { + cause = exception; + } else if (cause instanceof RuntimeException) { + cause = cause.getCause() != null ? cause.getCause() : cause; + } + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "failed", metadata, + "exception", + cause.getClass().getSimpleName()); } - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "failed", metadata, - "exception", - cause.getClass().getSimpleName()); } @Override @@ -258,40 +255,101 @@ private void incrementCounter(ResourceID id, String counterName, Map new HashSet<>()).add(counter.getId()); + cleaner.recordAssociation(id, counter); counter.increment(); } protected Set recordedMeterIdsFor(ResourceID resourceID) { - return metersPerResource.get(resourceID); + return cleaner.recordedMeterIdsFor(resourceID); } - private interface Cleaner { - void removeMetersFor(ResourceID resourceID); + public static class MicrometerMetricsBuilder { + private final MeterRegistry registry; + private int cleaningThreadsNumber; + private int cleanUpDelayInSeconds; + + private MicrometerMetricsBuilder(MeterRegistry registry) { + this.registry = registry; + } + + public MicrometerMetricsBuilder withCleaningThreadNumber(int cleaningThreadsNumber) { + this.cleaningThreadsNumber = cleaningThreadsNumber; + return this; + } + + /** + * @param cleanUpDelayInSeconds the number of seconds to wait before meters are removed for + * deleted resources + */ + public MicrometerMetricsBuilder withCleanUpDelayInSeconds(int cleanUpDelayInSeconds) { + this.cleanUpDelayInSeconds = cleanUpDelayInSeconds; + return this; + } + + public MicrometerMetrics build() { + MicrometerMetrics.Cleaner cleaner; + if (cleanUpDelayInSeconds < 0) { + cleaner = new MicrometerMetrics.DefaultCleaner(registry); + } else { + cleaningThreadsNumber = + cleaningThreadsNumber <= 0 ? Runtime.getRuntime().availableProcessors() + : cleaningThreadsNumber; + cleaner = new DelayedCleaner(registry, cleanUpDelayInSeconds, cleaningThreadsNumber); + } + + return new MicrometerMetrics(registry, cleaner); + } } - private void removeMetersFor(ResourceID resourceID) { - // remove each meter - final var toClean = metersPerResource.get(resourceID); - if (toClean != null) { - toClean.forEach(registry::remove); + private interface Cleaner { + Cleaner NOOP = new Cleaner() {}; + + default void removeMetersFor(ResourceID resourceID) {} + + default void recordAssociation(ResourceID resourceID, Meter meter) {} + + default Set recordedMeterIdsFor(ResourceID resourceID) { + return Collections.emptySet(); } - // then clean-up local recording of associations - metersPerResource.remove(resourceID); } - private class NoDelayCleaner implements Cleaner { + private static class DefaultCleaner implements Cleaner { + private final Map> metersPerResource = new ConcurrentHashMap<>(); + private final MeterRegistry registry; + + private DefaultCleaner(MeterRegistry registry) { + this.registry = registry; + } + @Override public void removeMetersFor(ResourceID resourceID) { - MicrometerMetrics.this.removeMetersFor(resourceID); + // remove each meter + final var toClean = metersPerResource.get(resourceID); + if (toClean != null) { + toClean.forEach(registry::remove); + } + // then clean-up local recording of associations + metersPerResource.remove(resourceID); + } + + @Override + public void recordAssociation(ResourceID resourceID, Meter meter) { + metersPerResource.computeIfAbsent(resourceID, id -> new HashSet<>()).add(meter.getId()); + } + + @Override + public Set recordedMeterIdsFor(ResourceID resourceID) { + return metersPerResource.get(resourceID); } } - private class DelayedCleaner implements Cleaner { + private static class DelayedCleaner extends MicrometerMetrics.DefaultCleaner { private final ScheduledExecutorService metersCleaner; private final int cleanUpDelayInSeconds; - private DelayedCleaner(int cleanUpDelayInSeconds, int cleaningThreadsNumber) { + private DelayedCleaner(MeterRegistry registry, int cleanUpDelayInSeconds, + int cleaningThreadsNumber) { + super(registry); this.cleanUpDelayInSeconds = cleanUpDelayInSeconds; this.metersCleaner = Executors.newScheduledThreadPool(cleaningThreadsNumber); } @@ -299,7 +357,7 @@ private DelayedCleaner(int cleanUpDelayInSeconds, int cleaningThreadsNumber) { @Override public void removeMetersFor(ResourceID resourceID) { // schedule deletion of meters associated with ResourceID - metersCleaner.schedule(() -> MicrometerMetrics.this.removeMetersFor(resourceID), + metersCleaner.schedule(() -> super.removeMetersFor(resourceID), cleanUpDelayInSeconds, TimeUnit.SECONDS); } } diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java index 717f3a11f9..af8ba73b87 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java @@ -29,7 +29,8 @@ public class MetricsCleaningOnDeleteIT { private static final TestSimpleMeterRegistry registry = new TestSimpleMeterRegistry(); private static final int testDelay = 1; - private static final MicrometerMetrics metrics = new MicrometerMetrics(registry, testDelay, 2); + private static final MicrometerMetrics metrics = MicrometerMetrics.newMicrometerMetrics(registry) + .withCleanUpDelayInSeconds(testDelay).withCleaningThreadNumber(2).build(); private static final String testResourceName = "cleaning-metrics-cr"; @BeforeAll From f761dbc34effb871c8dfeb960c3309700484a256 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 16 Mar 2023 09:46:52 +0100 Subject: [PATCH 04/29] refactor: extract abstract test fixture to add tests with variations --- .../AbstractMicrometerMetricsTestFixture.java | 103 ++++++++++++++++++ .../micrometer/MetricsCleaningOnDeleteIT.java | 89 +++------------ 2 files changed, 117 insertions(+), 75 deletions(-) create mode 100644 micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java new file mode 100644 index 0000000000..49c2575500 --- /dev/null +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -0,0 +1,103 @@ +package io.javaoperatorsdk.operator.monitoring.micrometer; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; +import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; +import io.javaoperatorsdk.operator.api.reconciler.*; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.processing.event.ResourceID; +import io.micrometer.core.instrument.Meter; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; + +import static org.awaitility.Awaitility.await; + +public abstract class AbstractMicrometerMetricsTestFixture { + @RegisterExtension + static LocallyRunOperatorExtension operator = + LocallyRunOperatorExtension.builder().withReconciler(new MetricsCleaningTestReconciler()) + .build(); + + protected static final TestSimpleMeterRegistry registry = new TestSimpleMeterRegistry(); + protected final MicrometerMetrics metrics = getMetrics(); + protected static final String testResourceName = "micrometer-metrics-cr"; + + protected abstract MicrometerMetrics getMetrics(); + + @BeforeAll + void setup() { + ConfigurationServiceProvider.overrideCurrent(overrider -> overrider.withMetrics(metrics)); + } + + @AfterAll + void reset() { + ConfigurationServiceProvider.reset(); + } + + @Test + void properlyHandlesResourceDeletion() throws Exception { + var testResource = new ConfigMapBuilder() + .withNewMetadata() + .withName(testResourceName) + .endMetadata() + .build(); + final var created = operator.create(testResource); + + // make sure the resource is created + await().until(() -> !operator.get(ConfigMap.class, testResourceName) + .getMetadata().getFinalizers().isEmpty()); + + final var resourceID = ResourceID.fromResource(created); + final var meters = preDeleteChecks(resourceID); + + // delete the resource and wait for it to be deleted + operator.delete(testResource); + await().until(() -> operator.get(ConfigMap.class, testResourceName) == null); + + postDeleteChecks(resourceID, meters); + } + + protected Set preDeleteChecks(ResourceID resourceID) { + return Collections + .emptySet(); + } + + protected void postDeleteChecks(ResourceID resourceID, Set meters) throws Exception {} + + @ControllerConfiguration + private static class MetricsCleaningTestReconciler + implements Reconciler, Cleaner { + @Override + public UpdateControl reconcile(ConfigMap resource, Context context) { + return UpdateControl.noUpdate(); + } + + @Override + public DeleteControl cleanup(ConfigMap resource, Context context) { + return DeleteControl.defaultDelete(); + } + } + + static class TestSimpleMeterRegistry extends SimpleMeterRegistry { + private final Set removed = new HashSet<>(); + + @Override + public Meter remove(Meter.Id mappedId) { + final var removed = super.remove(mappedId); + this.removed.add(removed.getId()); + return removed; + } + + public Set getRemoved() { + return removed; + } + } +} diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java index af8ba73b87..fe91338ea9 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java @@ -1,98 +1,37 @@ package io.javaoperatorsdk.operator.monitoring.micrometer; import java.time.Duration; -import java.util.HashSet; import java.util.Set; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; - -import io.fabric8.kubernetes.api.model.ConfigMap; -import io.fabric8.kubernetes.api.model.ConfigMapBuilder; -import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; -import io.javaoperatorsdk.operator.api.reconciler.*; -import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.micrometer.core.instrument.Meter; -import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import static org.assertj.core.api.Assertions.assertThat; -import static org.awaitility.Awaitility.await; -public class MetricsCleaningOnDeleteIT { - @RegisterExtension - static LocallyRunOperatorExtension operator = - LocallyRunOperatorExtension.builder().withReconciler(new MetricsCleaningTestReconciler()) - .build(); +public class MetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsTestFixture { - private static final TestSimpleMeterRegistry registry = new TestSimpleMeterRegistry(); private static final int testDelay = 1; - private static final MicrometerMetrics metrics = MicrometerMetrics.newMicrometerMetrics(registry) - .withCleanUpDelayInSeconds(testDelay).withCleaningThreadNumber(2).build(); - private static final String testResourceName = "cleaning-metrics-cr"; - - @BeforeAll - static void setup() { - ConfigurationServiceProvider.overrideCurrent(overrider -> overrider.withMetrics(metrics)); - } - @AfterAll - static void reset() { - ConfigurationServiceProvider.reset(); + @Override + protected MicrometerMetrics getMetrics() { + return MicrometerMetrics.newMicrometerMetrics(registry) + .withCleanUpDelayInSeconds(testDelay).withCleaningThreadNumber(2).build(); } - @Test - void removesMetersAssociatedWithResourceAfterItsDeletion() throws InterruptedException { - var testResource = new ConfigMapBuilder() - .withNewMetadata() - .withName(testResourceName) - .endMetadata() - .build(); - final var created = operator.create(testResource); - - // make sure the resource is created - await().until(() -> !operator.get(ConfigMap.class, testResourceName) - .getMetadata().getFinalizers().isEmpty()); - + @Override + protected Set preDeleteChecks(ResourceID resourceID) { // check that we properly recorded meters associated with the resource - final var meters = metrics.recordedMeterIdsFor(ResourceID.fromResource(created)); + final var meters = metrics.recordedMeterIdsFor(resourceID); assertThat(meters).isNotNull(); assertThat(meters).isNotEmpty(); + return meters; + } - // delete the resource and wait for it to be deleted - operator.delete(testResource); - await().until(() -> operator.get(ConfigMap.class, testResourceName) == null); - + @Override + protected void postDeleteChecks(ResourceID resourceID, Set meters) throws Exception { // check that the meters are properly removed after the specified delay Thread.sleep(Duration.ofSeconds(testDelay).toMillis()); - assertThat(registry.removed).isEqualTo(meters); - assertThat(metrics.recordedMeterIdsFor(ResourceID.fromResource(created))).isNull(); - } - - @ControllerConfiguration - private static class MetricsCleaningTestReconciler - implements Reconciler, Cleaner { - @Override - public UpdateControl reconcile(ConfigMap resource, Context context) { - return UpdateControl.noUpdate(); - } - - @Override - public DeleteControl cleanup(ConfigMap resource, Context context) { - return DeleteControl.defaultDelete(); - } - } - - private static class TestSimpleMeterRegistry extends SimpleMeterRegistry { - private final Set removed = new HashSet<>(); - - @Override - public Meter remove(Meter.Id mappedId) { - final var removed = super.remove(mappedId); - this.removed.add(removed.getId()); - return removed; - } + assertThat(registry.getRemoved()).isEqualTo(meters); + assertThat(metrics.recordedMeterIdsFor(resourceID)).isNull(); } } From 0d75b8709b50fb65d96625e0aa8a9fe332d627f0 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 16 Mar 2023 10:12:03 +0100 Subject: [PATCH 05/29] fix: add missing annotation --- .../micrometer/AbstractMicrometerMetricsTestFixture.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index 49c2575500..7f2059b29d 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -7,6 +7,7 @@ import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.extension.RegisterExtension; import io.fabric8.kubernetes.api.model.ConfigMap; @@ -20,6 +21,7 @@ import static org.awaitility.Awaitility.await; +@TestInstance(TestInstance.Lifecycle.PER_CLASS) public abstract class AbstractMicrometerMetricsTestFixture { @RegisterExtension static LocallyRunOperatorExtension operator = From 805fbe08bbca72147a532f686fe5c55d473d5258 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 16 Mar 2023 10:12:32 +0100 Subject: [PATCH 06/29] tests: add more test variations --- .../AbstractMicrometerMetricsTestFixture.java | 12 ++++++---- .../micrometer/DefaultBehaviorIT.java | 22 ++++++++++++++++++ ... => DelayedMetricsCleaningOnDeleteIT.java} | 16 ++++--------- .../NoDelayMetricsCleaningOnDeleteIT.java | 23 +++++++++++++++++++ .../micrometer/NoPerResourceCollectionIT.java | 22 ++++++++++++++++++ 5 files changed, 79 insertions(+), 16 deletions(-) create mode 100644 micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java rename micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/{MetricsCleaningOnDeleteIT.java => DelayedMetricsCleaningOnDeleteIT.java} (62%) create mode 100644 micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java create mode 100644 micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index 7f2059b29d..000875736b 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -1,6 +1,5 @@ package io.javaoperatorsdk.operator.monitoring.micrometer; -import java.util.Collections; import java.util.HashSet; import java.util.Set; @@ -19,6 +18,7 @@ import io.micrometer.core.instrument.Meter; import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; @TestInstance(TestInstance.Lifecycle.PER_CLASS) @@ -68,11 +68,15 @@ void properlyHandlesResourceDeletion() throws Exception { } protected Set preDeleteChecks(ResourceID resourceID) { - return Collections - .emptySet(); + // check that we properly recorded meters associated with the resource + final var meters = metrics.recordedMeterIdsFor(resourceID); + assertThat(meters).isNotNull(); + assertThat(meters).isNotEmpty(); + return meters; } - protected void postDeleteChecks(ResourceID resourceID, Set meters) throws Exception {} + protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) + throws Exception {} @ControllerConfiguration private static class MetricsCleaningTestReconciler diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java new file mode 100644 index 0000000000..288d178c7f --- /dev/null +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java @@ -0,0 +1,22 @@ +package io.javaoperatorsdk.operator.monitoring.micrometer; + +import java.util.Set; + +import io.javaoperatorsdk.operator.processing.event.ResourceID; +import io.micrometer.core.instrument.Meter; + +import static org.assertj.core.api.Assertions.assertThat; + +public class DefaultBehaviorIT extends AbstractMicrometerMetricsTestFixture { + @Override + protected MicrometerMetrics getMetrics() { + return MicrometerMetrics.newMicrometerMetrics(registry).build(); + } + + @Override + protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) + throws Exception { + assertThat(metrics.recordedMeterIdsFor(resourceID)).isNotNull(); + assertThat(registry.getRemoved()).isEmpty(); + } +} diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java similarity index 62% rename from micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java rename to micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java index fe91338ea9..d17f4ba5af 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/MetricsCleaningOnDeleteIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java @@ -8,7 +8,7 @@ import static org.assertj.core.api.Assertions.assertThat; -public class MetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsTestFixture { +public class DelayedMetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsTestFixture { private static final int testDelay = 1; @@ -19,19 +19,11 @@ protected MicrometerMetrics getMetrics() { } @Override - protected Set preDeleteChecks(ResourceID resourceID) { - // check that we properly recorded meters associated with the resource - final var meters = metrics.recordedMeterIdsFor(resourceID); - assertThat(meters).isNotNull(); - assertThat(meters).isNotEmpty(); - return meters; - } - - @Override - protected void postDeleteChecks(ResourceID resourceID, Set meters) throws Exception { + protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) + throws Exception { // check that the meters are properly removed after the specified delay Thread.sleep(Duration.ofSeconds(testDelay).toMillis()); - assertThat(registry.getRemoved()).isEqualTo(meters); + assertThat(registry.getRemoved()).isEqualTo(recordedMeters); assertThat(metrics.recordedMeterIdsFor(resourceID)).isNull(); } } diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java new file mode 100644 index 0000000000..d349da764d --- /dev/null +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java @@ -0,0 +1,23 @@ +package io.javaoperatorsdk.operator.monitoring.micrometer; + +import java.util.Set; + +import io.javaoperatorsdk.operator.processing.event.ResourceID; +import io.micrometer.core.instrument.Meter; + +import static org.assertj.core.api.Assertions.assertThat; + +public class NoDelayMetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsTestFixture { + @Override + protected MicrometerMetrics getMetrics() { + return MicrometerMetrics.newMicrometerMetrics(registry).withCleanUpDelayInSeconds(0).build(); + } + + @Override + protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) + throws Exception { + // check that the meters are properly immediately removed + assertThat(registry.getRemoved()).isEqualTo(recordedMeters); + assertThat(metrics.recordedMeterIdsFor(resourceID)).isNull(); + } +} diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java new file mode 100644 index 0000000000..d02e4a0e79 --- /dev/null +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java @@ -0,0 +1,22 @@ +package io.javaoperatorsdk.operator.monitoring.micrometer; + +import java.util.Collections; +import java.util.Set; + +import io.javaoperatorsdk.operator.processing.event.ResourceID; +import io.micrometer.core.instrument.Meter; + +import static org.assertj.core.api.Assertions.assertThat; + +public class NoPerResourceCollectionIT extends AbstractMicrometerMetricsTestFixture { + @Override + protected MicrometerMetrics getMetrics() { + return MicrometerMetrics.withoutPerResourceMetrics(registry); + } + + @Override + protected Set preDeleteChecks(ResourceID resourceID) { + assertThat(metrics.recordedMeterIdsFor(resourceID)).isEmpty(); + return Collections.emptySet(); + } +} From 76d25f909e9c8ae76af16e4a5bd55083a5a41337 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 16 Mar 2023 10:40:30 +0100 Subject: [PATCH 07/29] fix: make operator non-static so it's registered once per test subclass --- .../micrometer/AbstractMicrometerMetricsTestFixture.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index 000875736b..3bbbdbfd72 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -24,7 +24,7 @@ @TestInstance(TestInstance.Lifecycle.PER_CLASS) public abstract class AbstractMicrometerMetricsTestFixture { @RegisterExtension - static LocallyRunOperatorExtension operator = + LocallyRunOperatorExtension operator = LocallyRunOperatorExtension.builder().withReconciler(new MetricsCleaningTestReconciler()) .build(); From dd2d36069f26ab311b17b9d4d0d71b597a11fa4f Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 16 Mar 2023 17:02:38 +0100 Subject: [PATCH 08/29] feat: introduce builder for MicrometerMetrics, fix test --- .../micrometer/MicrometerMetrics.java | 90 +++++++++++++------ .../AbstractMicrometerMetricsTestFixture.java | 2 + .../micrometer/DefaultBehaviorIT.java | 12 ++- .../DelayedMetricsCleaningOnDeleteIT.java | 2 +- .../NoDelayMetricsCleaningOnDeleteIT.java | 5 +- .../micrometer/NoPerResourceCollectionIT.java | 1 + .../operator/sample/MySQLSchemaOperator.java | 6 +- 7 files changed, 86 insertions(+), 32 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index f28dea274e..87723ae9e7 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -36,24 +36,32 @@ public class MicrometerMetrics implements Metrics { private final Cleaner cleaner; /** - * Creates a non-delayed, micrometer-based Metrics implementation. The non-delayed part refers to - * the cleaning of meters associated with deleted resources. + * Creates a default micrometer-based Metrics implementation, collecting metrics on a per resource + * basis and not dealing with cleaning these after these resources are deleted. Note that this + * probably will change in a future release. If you want more control over what the implementation + * actually does, please use the static factory methods instead. * * @param registry the {@link MeterRegistry} instance to use for metrics recording + * @deprecated Use the factory methods / builders instead */ + @Deprecated public MicrometerMetrics(MeterRegistry registry) { - this(registry, Cleaner.NOOP); + this(registry, Cleaner.NOOP, true); } - @SuppressWarnings("unused") public static MicrometerMetrics withoutPerResourceMetrics(MeterRegistry registry) { - return new MicrometerMetrics(registry); + return new MicrometerMetrics(registry, Cleaner.NOOP, false); } public static MicrometerMetricsBuilder newMicrometerMetrics(MeterRegistry registry) { return new MicrometerMetricsBuilder(registry); } + public static PerResourceCollectingMicrometerMetricsBuilder newPerResourceCollectingMicrometerMetrics( + MeterRegistry registry) { + return new PerResourceCollectingMicrometerMetricsBuilder(registry); + } + /** * 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} @@ -61,11 +69,13 @@ public static MicrometerMetricsBuilder newMicrometerMetrics(MeterRegistry regist * * @param registry the {@link MeterRegistry} instance to use for metrics recording * @param cleaner the {@link Cleaner} to use + * @param collectingPerResourceMetrics whether or not to collect per resource metrics */ - private MicrometerMetrics(MeterRegistry registry, Cleaner cleaner) { + private MicrometerMetrics(MeterRegistry registry, Cleaner cleaner, + boolean collectingPerResourceMetrics) { this.registry = registry; this.cleaner = cleaner; - this.collectPerResourceMetrics = Cleaner.NOOP != cleaner; + this.collectPerResourceMetrics = collectingPerResourceMetrics; } @Override @@ -263,41 +273,67 @@ protected Set recordedMeterIdsFor(ResourceID resourceID) { return cleaner.recordedMeterIdsFor(resourceID); } - public static class MicrometerMetricsBuilder { - private final MeterRegistry registry; + + public static class PerResourceCollectingMicrometerMetricsBuilder + extends MicrometerMetricsBuilder { + private int cleaningThreadsNumber; private int cleanUpDelayInSeconds; - private MicrometerMetricsBuilder(MeterRegistry registry) { - this.registry = registry; + private PerResourceCollectingMicrometerMetricsBuilder(MeterRegistry registry) { + super(registry); } - public MicrometerMetricsBuilder withCleaningThreadNumber(int cleaningThreadsNumber) { - this.cleaningThreadsNumber = cleaningThreadsNumber; + /** + * @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 PerResourceCollectingMicrometerMetricsBuilder withCleaningThreadNumber( + int cleaningThreadsNumber) { + this.cleaningThreadsNumber = cleaningThreadsNumber <= 0 ? 1 : cleaningThreadsNumber; return this; } /** - * @param cleanUpDelayInSeconds the number of seconds to wait before meters are removed for - * deleted resources + * @param cleanUpDelayInSeconds the number of seconds to wait before {@link Meter}s are removed + * for deleted resources, defaults to 0 (meaning meters will be removed immediately after + * the associated resource is deleted) if not specified or if the provided number is + * lesser or equal to 0 */ - public MicrometerMetricsBuilder withCleanUpDelayInSeconds(int cleanUpDelayInSeconds) { - this.cleanUpDelayInSeconds = cleanUpDelayInSeconds; + public PerResourceCollectingMicrometerMetricsBuilder withCleanUpDelayInSeconds( + int cleanUpDelayInSeconds) { + this.cleanUpDelayInSeconds = Math.max(cleanUpDelayInSeconds, 0); return this; } + @Override public MicrometerMetrics build() { - MicrometerMetrics.Cleaner cleaner; - if (cleanUpDelayInSeconds < 0) { - cleaner = new MicrometerMetrics.DefaultCleaner(registry); - } else { - cleaningThreadsNumber = - cleaningThreadsNumber <= 0 ? Runtime.getRuntime().availableProcessors() - : cleaningThreadsNumber; - cleaner = new DelayedCleaner(registry, cleanUpDelayInSeconds, cleaningThreadsNumber); - } + final var cleaner = cleanUpDelayInSeconds == 0 ? new DefaultCleaner(registry) + : new DelayedCleaner(registry, cleanUpDelayInSeconds, cleaningThreadsNumber); + return new MicrometerMetrics(registry, cleaner, true); + } + } + public static class MicrometerMetricsBuilder { + protected final MeterRegistry registry; + private boolean collectingPerResourceMetrics = true; + + private MicrometerMetricsBuilder(MeterRegistry registry) { + this.registry = registry; + } + + public PerResourceCollectingMicrometerMetricsBuilder collectingMetricsPerResource() { + collectingPerResourceMetrics = true; + return new PerResourceCollectingMicrometerMetricsBuilder(registry); + } - return new MicrometerMetrics(registry, cleaner); + public MicrometerMetricsBuilder notCollectingMetricsPerResource() { + collectingPerResourceMetrics = false; + return this; + } + + public MicrometerMetrics build() { + return new MicrometerMetrics(registry, Cleaner.NOOP, collectingPerResourceMetrics); } } diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index 3bbbdbfd72..932dc905d5 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -70,6 +70,8 @@ void properlyHandlesResourceDeletion() throws Exception { protected Set preDeleteChecks(ResourceID resourceID) { // check that we properly recorded meters associated with the resource final var meters = metrics.recordedMeterIdsFor(resourceID); + // metrics are collected per resource + assertThat(registry.getMetersAsString()).contains(resourceID.getName()); assertThat(meters).isNotNull(); assertThat(meters).isNotEmpty(); return meters; diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java index 288d178c7f..753b1db4b0 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java @@ -1,5 +1,6 @@ package io.javaoperatorsdk.operator.monitoring.micrometer; +import java.util.Collections; import java.util.Set; import io.javaoperatorsdk.operator.processing.event.ResourceID; @@ -13,10 +14,19 @@ protected MicrometerMetrics getMetrics() { return MicrometerMetrics.newMicrometerMetrics(registry).build(); } + @Override + protected Set preDeleteChecks(ResourceID resourceID) { + // no meter should be recorded because we're not tracking anything to be deleted later + assertThat(metrics.recordedMeterIdsFor(resourceID)).isEmpty(); + // metrics are collected per resource by default for now, this will change in a future release + assertThat(registry.getMetersAsString()).contains(resourceID.getName()); + return Collections.emptySet(); + } + @Override protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) throws Exception { - assertThat(metrics.recordedMeterIdsFor(resourceID)).isNotNull(); + // meters should be neither recorded, nor removed by default assertThat(registry.getRemoved()).isEmpty(); } } diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java index d17f4ba5af..af798a9521 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java @@ -14,7 +14,7 @@ public class DelayedMetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsT @Override protected MicrometerMetrics getMetrics() { - return MicrometerMetrics.newMicrometerMetrics(registry) + return MicrometerMetrics.newPerResourceCollectingMicrometerMetrics(registry) .withCleanUpDelayInSeconds(testDelay).withCleaningThreadNumber(2).build(); } diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java index d349da764d..5eb7804eb0 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java @@ -10,7 +10,8 @@ public class NoDelayMetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsTestFixture { @Override protected MicrometerMetrics getMetrics() { - return MicrometerMetrics.newMicrometerMetrics(registry).withCleanUpDelayInSeconds(0).build(); + return MicrometerMetrics.newPerResourceCollectingMicrometerMetrics(registry) + .withCleanUpDelayInSeconds(0).build(); } @Override @@ -20,4 +21,6 @@ protected void postDeleteChecks(ResourceID resourceID, Set recordedMet assertThat(registry.getRemoved()).isEqualTo(recordedMeters); assertThat(metrics.recordedMeterIdsFor(resourceID)).isNull(); } + + } diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java index d02e4a0e79..ac35347697 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoPerResourceCollectionIT.java @@ -17,6 +17,7 @@ protected MicrometerMetrics getMetrics() { @Override protected Set preDeleteChecks(ResourceID resourceID) { assertThat(metrics.recordedMeterIdsFor(resourceID)).isEmpty(); + assertThat(registry.getMetersAsString()).doesNotContain(resourceID.getName()); return Collections.emptySet(); } } diff --git a/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaOperator.java b/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaOperator.java index 4128dd0ea8..f312716f44 100644 --- a/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaOperator.java +++ b/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaOperator.java @@ -9,7 +9,8 @@ import org.takes.http.Exit; import org.takes.http.FtBasic; -import io.fabric8.kubernetes.client.*; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientBuilder; import io.javaoperatorsdk.operator.Operator; import io.javaoperatorsdk.operator.monitoring.micrometer.MicrometerMetrics; import io.javaoperatorsdk.operator.sample.dependent.ResourcePollerConfig; @@ -25,7 +26,8 @@ public static void main(String[] args) throws IOException { KubernetesClient client = new KubernetesClientBuilder().build(); Operator operator = new Operator(client, - overrider -> overrider.withMetrics(new MicrometerMetrics(new LoggingMeterRegistry()))); + overrider -> overrider + .withMetrics(MicrometerMetrics.withoutPerResourceMetrics(new LoggingMeterRegistry()))); MySQLSchemaReconciler schemaReconciler = new MySQLSchemaReconciler(); From 57cf2462ecb5fa2be5943b8db176bfa299cca643 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 16 Mar 2023 17:46:27 +0100 Subject: [PATCH 09/29] fix: exclude more tags when not collecting per resource --- .../monitoring/micrometer/MicrometerMetrics.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 87723ae9e7..b5d5e6455f 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -104,11 +104,16 @@ public T timeControllerExecution(ControllerExecution execution) { final var resourceID = execution.resourceID(); final var metadata = execution.metadata(); final var tags = new ArrayList(metadata.size() + 4); - tags.addAll(List.of( - "controller", name, - "resource.name", resourceID.getName(), - "resource.namespace", resourceID.getNamespace().orElse(""), - "resource.scope", getScope(resourceID))); + if (collectPerResourceMetrics) { + tags.addAll(List.of( + "controller", name, + "resource.name", resourceID.getName(), + "resource.namespace", resourceID.getNamespace().orElse(""), + "resource.scope", getScope(resourceID))); + } else { + tags.add("controller"); + tags.add(name); + } final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY); if (gvk != null) { tags.addAll(List.of( From b51ea8435a52d6d5ef8304cfd2dab22280967cd8 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 17 Mar 2023 09:18:29 +0100 Subject: [PATCH 10/29] fix: registry should be per-instance to ensure test independence --- .../micrometer/AbstractMicrometerMetricsTestFixture.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index 932dc905d5..aa67c75f76 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -28,7 +28,7 @@ public abstract class AbstractMicrometerMetricsTestFixture { LocallyRunOperatorExtension.builder().withReconciler(new MetricsCleaningTestReconciler()) .build(); - protected static final TestSimpleMeterRegistry registry = new TestSimpleMeterRegistry(); + protected final TestSimpleMeterRegistry registry = new TestSimpleMeterRegistry(); protected final MicrometerMetrics metrics = getMetrics(); protected static final String testResourceName = "micrometer-metrics-cr"; From 9b141515d51834b57c74e7c6993051d1e2652e57 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 17 Mar 2023 10:37:10 +0100 Subject: [PATCH 11/29] fix: make sure we wait a little to ensure event is properly processed --- .../micrometer/AbstractMicrometerMetricsTestFixture.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index aa67c75f76..830d28b938 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -1,5 +1,6 @@ package io.javaoperatorsdk.operator.monitoring.micrometer; +import java.time.Duration; import java.util.HashSet; import java.util.Set; @@ -63,6 +64,7 @@ void properlyHandlesResourceDeletion() throws Exception { // delete the resource and wait for it to be deleted operator.delete(testResource); await().until(() -> operator.get(ConfigMap.class, testResourceName) == null); + Thread.sleep(Duration.ofSeconds(1)); // make sure delete event is propagated and handled by the SDK postDeleteChecks(resourceID, meters); } From 54ddeec139457ffc8902fdc8a24c7abd001b088b Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 17 Mar 2023 11:02:45 +0100 Subject: [PATCH 12/29] fix: make things work on Java 11, format --- .../micrometer/AbstractMicrometerMetricsTestFixture.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index 830d28b938..3b5b111b7d 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -64,7 +64,8 @@ void properlyHandlesResourceDeletion() throws Exception { // delete the resource and wait for it to be deleted operator.delete(testResource); await().until(() -> operator.get(ConfigMap.class, testResourceName) == null); - Thread.sleep(Duration.ofSeconds(1)); // make sure delete event is propagated and handled by the SDK + // make sure delete event is propagated and handled by the SDK + Thread.sleep(Duration.ofSeconds(1).toMillis()); postDeleteChecks(resourceID, meters); } From feb8b06a0dcc98d09a924bf6687103f4abbaddce Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 17 Mar 2023 17:23:42 +0100 Subject: [PATCH 13/29] fix: also clean metrics on finalizer removal This is needed because the finalizer will trigger a reconciliation that adds a resource-specific metric. --- .../monitoring/micrometer/MicrometerMetrics.java | 9 ++++++--- .../micrometer/AbstractMicrometerMetricsTestFixture.java | 5 +---- .../micrometer/NoDelayMetricsCleaningOnDeleteIT.java | 6 ++++++ .../operator/processing/event/EventProcessor.java | 1 + 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index b5d5e6455f..562214c5e1 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -278,6 +278,9 @@ protected Set recordedMeterIdsFor(ResourceID resourceID) { return cleaner.recordedMeterIdsFor(resourceID); } + protected Cleaner getCleaner() { + return cleaner; + } public static class PerResourceCollectingMicrometerMetricsBuilder extends MicrometerMetricsBuilder { @@ -342,7 +345,7 @@ public MicrometerMetrics build() { } } - private interface Cleaner { + interface Cleaner { Cleaner NOOP = new Cleaner() {}; default void removeMetersFor(ResourceID resourceID) {} @@ -354,7 +357,7 @@ default Set recordedMeterIdsFor(ResourceID resourceID) { } } - private static class DefaultCleaner implements Cleaner { + static class DefaultCleaner implements Cleaner { private final Map> metersPerResource = new ConcurrentHashMap<>(); private final MeterRegistry registry; @@ -384,7 +387,7 @@ public Set recordedMeterIdsFor(ResourceID resourceID) { } } - private static class DelayedCleaner extends MicrometerMetrics.DefaultCleaner { + static class DelayedCleaner extends MicrometerMetrics.DefaultCleaner { private final ScheduledExecutorService metersCleaner; private final int cleanUpDelayInSeconds; diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index 3b5b111b7d..d476dcba57 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -1,6 +1,5 @@ package io.javaoperatorsdk.operator.monitoring.micrometer; -import java.time.Duration; import java.util.HashSet; import java.util.Set; @@ -64,9 +63,7 @@ void properlyHandlesResourceDeletion() throws Exception { // delete the resource and wait for it to be deleted operator.delete(testResource); await().until(() -> operator.get(ConfigMap.class, testResourceName) == null); - // make sure delete event is propagated and handled by the SDK - Thread.sleep(Duration.ofSeconds(1).toMillis()); - + postDeleteChecks(resourceID, meters); } diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java index 5eb7804eb0..9d8923e8c2 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java @@ -14,6 +14,12 @@ protected MicrometerMetrics getMetrics() { .withCleanUpDelayInSeconds(0).build(); } + @Override + protected Set preDeleteChecks(ResourceID resourceID) { + assertThat(metrics.getCleaner()).isInstanceOf(MicrometerMetrics.DefaultCleaner.class); + return super.preDeleteChecks(resourceID); + } + @Override protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) throws Exception { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java index f973a7d354..7bf499c1c3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java @@ -238,6 +238,7 @@ synchronized void eventProcessingFinished( cleanupForDeletedEvent(executionScope.getResourceID()); } else if (postExecutionControl.isFinalizerRemoved()) { state.markProcessedMarkForDeletion(); + metrics.cleanupDoneFor(resourceID, metricsMetadata); } else { postExecutionControl .getUpdatedCustomResource() From edc953054274a2b72056c83921cc58a3d46a6452 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 17 Mar 2023 18:17:11 +0100 Subject: [PATCH 14/29] fix: format --- .../micrometer/AbstractMicrometerMetricsTestFixture.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java index d476dcba57..aa67c75f76 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/AbstractMicrometerMetricsTestFixture.java @@ -63,7 +63,7 @@ void properlyHandlesResourceDeletion() throws Exception { // delete the resource and wait for it to be deleted operator.delete(testResource); await().until(() -> operator.get(ConfigMap.class, testResourceName) == null); - + postDeleteChecks(resourceID, meters); } From 6d146631c5dc6012c360a454e9cf31cf67a264be Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 20 Mar 2023 12:44:51 +0100 Subject: [PATCH 15/29] refactor: extract common tags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Sébastien CROCQUESEL <88554524+scrocquesel@users.noreply.github.com> --- .../operator/monitoring/micrometer/MicrometerMetrics.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 562214c5e1..50232a4ee9 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -104,15 +104,13 @@ public T timeControllerExecution(ControllerExecution execution) { final var resourceID = execution.resourceID(); final var metadata = execution.metadata(); final var tags = new ArrayList(metadata.size() + 4); + tags.add("controller"); + tags.add(name); if (collectPerResourceMetrics) { tags.addAll(List.of( - "controller", name, "resource.name", resourceID.getName(), "resource.namespace", resourceID.getNamespace().orElse(""), "resource.scope", getScope(resourceID))); - } else { - tags.add("controller"); - tags.add(name); } final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY); if (gvk != null) { From 916d849da51d17658494751c12eba4f63877ba2a Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 20 Mar 2023 16:37:31 +0100 Subject: [PATCH 16/29] feat: make per-resource collecting finer-grained We now still collect GVK information when per-resource collection is switched off. --- .../micrometer/MicrometerMetrics.java | 84 +++++++++---------- .../processing/event/EventProcessor.java | 1 - 2 files changed, 38 insertions(+), 47 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 50232a4ee9..15ee74efdb 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -153,53 +153,45 @@ private static String getScope(ResourceID resourceID) { @Override public void receivedEvent(Event event, Map metadata) { - if (collectPerResourceMetrics) { - final String[] tags; - if (event instanceof ResourceEvent) { - tags = new String[] {"event", event.getClass().getSimpleName(), "action", - ((ResourceEvent) event).getAction().toString()}; - } else { - tags = new String[] {"event", event.getClass().getSimpleName()}; - } - - incrementCounter(event.getRelatedCustomResourceID(), "events.received", - metadata, - tags); + final String[] tags; + if (event instanceof ResourceEvent) { + tags = new String[] {"event", event.getClass().getSimpleName(), "action", + ((ResourceEvent) event).getAction().toString()}; + } else { + tags = new String[] {"event", event.getClass().getSimpleName()}; } + + incrementCounter(event.getRelatedCustomResourceID(), "events.received", + metadata, + tags); } @Override public void cleanupDoneFor(ResourceID resourceID, Map metadata) { - if (collectPerResourceMetrics) { - incrementCounter(resourceID, "events.delete", metadata); + incrementCounter(resourceID, "events.delete", metadata); - cleaner.removeMetersFor(resourceID); - } + cleaner.removeMetersFor(resourceID); } @Override public void reconcileCustomResource(HasMetadata resource, RetryInfo retryInfoNullable, Map metadata) { - if (collectPerResourceMetrics) { - Optional retryInfo = Optional.ofNullable(retryInfoNullable); - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "started", - metadata, - RECONCILIATIONS + "retries.number", - String.valueOf(retryInfo.map(RetryInfo::getAttemptCount).orElse(0)), - RECONCILIATIONS + "retries.last", - String.valueOf(retryInfo.map(RetryInfo::isLastAttempt).orElse(true))); - - var controllerQueueSize = - gauges.get(RECONCILIATIONS_QUEUE_SIZE + metadata.get(CONTROLLER_NAME)); - controllerQueueSize.incrementAndGet(); - } + Optional retryInfo = Optional.ofNullable(retryInfoNullable); + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "started", + metadata, + RECONCILIATIONS + "retries.number", + String.valueOf(retryInfo.map(RetryInfo::getAttemptCount).orElse(0)), + RECONCILIATIONS + "retries.last", + String.valueOf(retryInfo.map(RetryInfo::isLastAttempt).orElse(true))); + + var controllerQueueSize = + gauges.get(RECONCILIATIONS_QUEUE_SIZE + metadata.get(CONTROLLER_NAME)); + controllerQueueSize.incrementAndGet(); } @Override public void finishedReconciliation(HasMetadata resource, Map metadata) { - if (collectPerResourceMetrics) { - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "success", metadata); - } + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "success", metadata); } @Override @@ -223,17 +215,15 @@ public void reconciliationExecutionFinished(HasMetadata resource, Map metadata) { - if (collectPerResourceMetrics) { - var cause = exception.getCause(); - if (cause == null) { - cause = exception; - } else if (cause instanceof RuntimeException) { - cause = cause.getCause() != null ? cause.getCause() : cause; - } - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "failed", metadata, - "exception", - cause.getClass().getSimpleName()); + var cause = exception.getCause(); + if (cause == null) { + cause = exception; + } else if (cause instanceof RuntimeException) { + cause = cause.getCause() != null ? cause.getCause() : cause; } + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "failed", metadata, + "exception", + cause.getClass().getSimpleName()); } @Override @@ -253,10 +243,12 @@ private void incrementCounter(ResourceID id, String counterName, Map 0 ? additionalTags.length : 0; final var metadataNb = metadata != null ? metadata.size() : 0; final var tags = new ArrayList(6 + additionalTagsNb + metadataNb); - tags.addAll(List.of( - "name", id.getName(), - "namespace", id.getNamespace().orElse(""), - "scope", getScope(id))); + if (collectPerResourceMetrics) { + tags.addAll(List.of( + "name", id.getName(), + "namespace", id.getNamespace().orElse(""), + "scope", getScope(id))); + } if (additionalTagsNb > 0) { tags.addAll(List.of(additionalTags)); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java index 7bf499c1c3..f973a7d354 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java @@ -238,7 +238,6 @@ synchronized void eventProcessingFinished( cleanupForDeletedEvent(executionScope.getResourceID()); } else if (postExecutionControl.isFinalizerRemoved()) { state.markProcessedMarkForDeletion(); - metrics.cleanupDoneFor(resourceID, metricsMetadata); } else { postExecutionControl .getUpdatedCustomResource() From bc5b5f4004f3e88868afe1655664c5030a69b0f1 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 20 Mar 2023 17:03:38 +0100 Subject: [PATCH 17/29] fix: do not create tag for group if not present --- .../micrometer/MicrometerMetrics.java | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 15ee74efdb..4428dc83a2 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -233,8 +233,12 @@ public void failedReconciliation(HasMetadata resource, Exception exception, public static List gvkTags(Class resourceClass) { final var gvk = GroupVersionKind.gvkFor(resourceClass); - return List.of(Tag.of("group", gvk.group), Tag.of("version", gvk.version), - Tag.of("kind", gvk.kind)); + if (groupExists(gvk)) { + return List.of(Tag.of("group", gvk.group), Tag.of("version", gvk.version), + Tag.of("kind", gvk.kind)); + } else { + return List.of(Tag.of("version", gvk.version), Tag.of("kind", gvk.kind)); + } } private void incrementCounter(ResourceID id, String counterName, Map metadata, @@ -254,16 +258,21 @@ private void incrementCounter(ResourceID id, String counterName, Map 0) { final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY); - tags.addAll(List.of( - "group", gvk.group, - "version", gvk.version, - "kind", gvk.kind)); + if (groupExists(gvk)) { + tags.add("group"); + tags.add(gvk.group); + } + tags.addAll(List.of("version", gvk.version, "kind", gvk.kind)); } final var counter = registry.counter(PREFIX + counterName, tags.toArray(new String[0])); cleaner.recordAssociation(id, counter); counter.increment(); } + private static boolean groupExists(GroupVersionKind gvk) { + return gvk.group != null && !gvk.group.isBlank(); + } + protected Set recordedMeterIdsFor(ResourceID resourceID) { return cleaner.recordedMeterIdsFor(resourceID); } From 445c891d0c880b052ad2e1f19c1e24188b8200ed Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 22 Mar 2023 11:07:12 +0100 Subject: [PATCH 18/29] fix: remove unreliable no-delay implementation, defaulting to 1s delay --- .../micrometer/MicrometerMetrics.java | 12 ++++--- .../NoDelayMetricsCleaningOnDeleteIT.java | 32 ------------------- .../processing/event/EventProcessor.java | 1 + 3 files changed, 8 insertions(+), 37 deletions(-) delete mode 100644 micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 4428dc83a2..ed90d1f3f0 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -304,20 +304,22 @@ public PerResourceCollectingMicrometerMetricsBuilder withCleaningThreadNumber( /** * @param cleanUpDelayInSeconds the number of seconds to wait before {@link Meter}s are removed - * for deleted resources, defaults to 0 (meaning meters will be removed immediately after + * 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 or equal to 0 + * 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 PerResourceCollectingMicrometerMetricsBuilder withCleanUpDelayInSeconds( int cleanUpDelayInSeconds) { - this.cleanUpDelayInSeconds = Math.max(cleanUpDelayInSeconds, 0); + this.cleanUpDelayInSeconds = Math.max(cleanUpDelayInSeconds, 1); return this; } @Override public MicrometerMetrics build() { - final var cleaner = cleanUpDelayInSeconds == 0 ? new DefaultCleaner(registry) - : new DelayedCleaner(registry, cleanUpDelayInSeconds, cleaningThreadsNumber); + final var cleaner = + new DelayedCleaner(registry, cleanUpDelayInSeconds, cleaningThreadsNumber); return new MicrometerMetrics(registry, cleaner, true); } } diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java deleted file mode 100644 index 9d8923e8c2..0000000000 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/NoDelayMetricsCleaningOnDeleteIT.java +++ /dev/null @@ -1,32 +0,0 @@ -package io.javaoperatorsdk.operator.monitoring.micrometer; - -import java.util.Set; - -import io.javaoperatorsdk.operator.processing.event.ResourceID; -import io.micrometer.core.instrument.Meter; - -import static org.assertj.core.api.Assertions.assertThat; - -public class NoDelayMetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsTestFixture { - @Override - protected MicrometerMetrics getMetrics() { - return MicrometerMetrics.newPerResourceCollectingMicrometerMetrics(registry) - .withCleanUpDelayInSeconds(0).build(); - } - - @Override - protected Set preDeleteChecks(ResourceID resourceID) { - assertThat(metrics.getCleaner()).isInstanceOf(MicrometerMetrics.DefaultCleaner.class); - return super.preDeleteChecks(resourceID); - } - - @Override - protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) - throws Exception { - // check that the meters are properly immediately removed - assertThat(registry.getRemoved()).isEqualTo(recordedMeters); - assertThat(metrics.recordedMeterIdsFor(resourceID)).isNull(); - } - - -} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java index f973a7d354..7bf499c1c3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java @@ -238,6 +238,7 @@ synchronized void eventProcessingFinished( cleanupForDeletedEvent(executionScope.getResourceID()); } else if (postExecutionControl.isFinalizerRemoved()) { state.markProcessedMarkForDeletion(); + metrics.cleanupDoneFor(resourceID, metricsMetadata); } else { postExecutionControl .getUpdatedCustomResource() From 7565e1b268c0565160606656eb59c45bbe003854 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 23 Mar 2023 10:27:52 +0100 Subject: [PATCH 19/29] refactor: renamed & documented factory methods to make things clearer --- .../micrometer/MicrometerMetrics.java | 33 +++++++++++++++++-- .../micrometer/DefaultBehaviorIT.java | 2 +- .../DelayedMetricsCleaningOnDeleteIT.java | 2 +- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index ed90d1f3f0..a886e69ab1 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -49,15 +49,37 @@ public MicrometerMetrics(MeterRegistry registry) { this(registry, Cleaner.NOOP, true); } + /** + * Creates a MicrometerMetrics instance configured to not collect per-resource metrics, just + * aggregates per resource **type** + * + * @param registry the {@link MeterRegistry} instance to use for metrics recording + * @return a MicrometerMetrics instance configured to not collect per-resource metrics + */ public static MicrometerMetrics withoutPerResourceMetrics(MeterRegistry registry) { return new MicrometerMetrics(registry, Cleaner.NOOP, false); } - public static MicrometerMetricsBuilder newMicrometerMetrics(MeterRegistry registry) { + /** + * Creates a new builder to configure how the eventual MicrometerMetrics instance will behave. + * + * @param registry the {@link MeterRegistry} instance to use for metrics recording + * @return a MicrometerMetrics instance configured to not collect per-resource metrics + * @see MicrometerMetricsBuilder + */ + public static MicrometerMetricsBuilder newMicrometerMetricsBuilder(MeterRegistry registry) { return new MicrometerMetricsBuilder(registry); } - public static PerResourceCollectingMicrometerMetricsBuilder newPerResourceCollectingMicrometerMetrics( + /** + * Creates a new builder to configure how the eventual MicrometerMetrics instance will behave, + * pre-configuring it to collect metrics per resource. + * + * @param registry the {@link MeterRegistry} instance to use for metrics recording + * @return a MicrometerMetrics instance configured to not collect per-resource metrics + * @see PerResourceCollectingMicrometerMetricsBuilder + */ + public static PerResourceCollectingMicrometerMetricsBuilder newPerResourceCollectingMicrometerMetricsBuilder( MeterRegistry registry) { return new PerResourceCollectingMicrometerMetricsBuilder(registry); } @@ -331,11 +353,18 @@ private MicrometerMetricsBuilder(MeterRegistry registry) { this.registry = registry; } + /** + * Configures the instance to collect metrics on a per-resource basis. + */ public PerResourceCollectingMicrometerMetricsBuilder collectingMetricsPerResource() { collectingPerResourceMetrics = true; return new PerResourceCollectingMicrometerMetricsBuilder(registry); } + /** + * Configures the instance to only collect metrics per resource **type**, in an aggregate + * fashion, instead of per resource instance. + */ public MicrometerMetricsBuilder notCollectingMetricsPerResource() { collectingPerResourceMetrics = false; return this; diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java index 753b1db4b0..7123b8a8c7 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java @@ -11,7 +11,7 @@ public class DefaultBehaviorIT extends AbstractMicrometerMetricsTestFixture { @Override protected MicrometerMetrics getMetrics() { - return MicrometerMetrics.newMicrometerMetrics(registry).build(); + return MicrometerMetrics.newMicrometerMetricsBuilder(registry).build(); } @Override diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java index af798a9521..26dfe59f84 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DelayedMetricsCleaningOnDeleteIT.java @@ -14,7 +14,7 @@ public class DelayedMetricsCleaningOnDeleteIT extends AbstractMicrometerMetricsT @Override protected MicrometerMetrics getMetrics() { - return MicrometerMetrics.newPerResourceCollectingMicrometerMetrics(registry) + return MicrometerMetrics.newPerResourceCollectingMicrometerMetricsBuilder(registry) .withCleanUpDelayInSeconds(testDelay).withCleaningThreadNumber(2).build(); } From e22dc7501797b7fa7ded184756a884790b3ea92f Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 23 Mar 2023 10:28:27 +0100 Subject: [PATCH 20/29] docs: updated metrics section for code changes --- docs/documentation/features.md | 70 +++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/docs/documentation/features.md b/docs/documentation/features.md index 2a27250559..11404a8b14 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -774,32 +774,56 @@ ConfigurationServiceProvider.overrideCurrent(overrider->overrider.withMetrics(me ### Micrometer implementation -The micrometer implementation records a lot of metrics associated to each resource handled by the operator by default. -In order to be efficient, the implementation removes meters associated with resources when they are deleted. Since it -might be useful to keep these metrics around for a bit before they are deleted, it is possible to configure a delay -before their removal. As this is done asynchronously, it is also possible to configure how many threads you want to -devote to these operations. Both aspects are controlled by the `MicrometerMetrics` constructor so changing the defaults -is a matter of instantiating `MicrometerMetrics` with the desired values and tell `ConfigurationServiceProvider` about -it as shown above. +The micrometer implementation is typically created using one of the provided factory methods which, depending on which +is used, will return either a ready to use instance or a builder allowing users to customized how the implementation +behaves, in particular when it comes to the granularity of collected metrics. It is, for example, possible to collect +metrics on a per-resource basis via tags that are associated with meters. This is the default, historical behavior but +this might change in a future version of JOSDK because this dramatically increases the cardinality of metrics, which +could lead to performance issues. + +To create a `MicrometerMetrics` implementation that behaves how it has historically behaved, you can just create an +instance via: + +```java +MeterRegistry registry= …; +Metrics metrics=new MicrometerMetrics(registry) +``` + +Note, however, that this constructor is deprecated and we encourage you to use the factory methods instead, which either +return a fully pre-configured instance or a builder object that will allow you to configure more easily how the instance +will behave. You can, for example, configure whether or not the implementation should collect metrics on a per-resource +basis, whether or not associated meters should be removed when a resource is deleted and how the clean-up is performed. +See the relevant classes documentation for more details. + +For example, the following will create a `MicrometerMetrics` instance configured to collect metrics on a per-resource +basis, deleting the associated meters after 5 seconds when a resource is deleted, using up to 2 threads to do so. + +```java +MicrometerMetrics.newPerResourceCollectingMicrometerMetricsBuilder(registry) + .withCleanUpDelayInSeconds(5) + .withCleaningThreadNumber(2) + .build() +``` The micrometer implementation records the following metrics: -| Meter name | Type | Tags | Description | -|-----------------------------------------------------------|----------------|------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------| -| operator.sdk.reconciliations.executions. | gauge | group, version, kind | Number of executions of the named reconciler | -| operator.sdk.reconciliations.queue.size. | gauge | group, version, kind | How many resources are queued to get reconciled by named reconciler | -| operator.sdk..size | gauge map size | | Gauge tracking the size of a specified map (currently unused but could be used to monitor caches size) | -| operator.sdk.events.received | counter | group, version, kind, name, namespace, scope, event, action | Number of received Kubernetes events | -| operator.sdk.events.delete | counter | group, version, kind, name, namespace, scope | Number of received Kubernetes delete events | -| operator.sdk.reconciliations.started | counter | group, version, kind, name, namespace, scope, reconciliations.retries.last, reconciliations.retries.number | Number of started reconciliations per resource type | -| operator.sdk.reconciliations.failed | counter | group, version, kind, name, namespace, scope, exception | Number of failed reconciliations per resource type | -| operator.sdk.reconciliations.success | counter | group, version, kind, name, namespace, scope | Number of successful reconciliations per resource type | -| operator.sdk.controllers.execution.reconcile.success | counter | controller, type | Number of successful reconciliations per controller | -| operator.sdk.controllers.execution.reconcile.failure | counter | controller, exception | Number of failed reconciliations per controller | -| operator.sdk.controllers.execution.cleanup.success | counter | controller, type | Number of successful cleanups per controller | -| operator.sdk.controllers.execution.cleanup.failure | counter | controller, exception | Number of failed cleanups per controller | - -As you can see all the recorded metrics start with the `operator.sdk` prefix. +| Meter name | Type | Tags | Description | +|-----------------------------------------------------------|----------------|---------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------| +| operator.sdk.reconciliations.executions. | gauge | group, version, kind | Number of executions of the named reconciler | +| operator.sdk.reconciliations.queue.size. | gauge | group, version, kind | How many resources are queued to get reconciled by named reconciler | +| operator.sdk..size | gauge map size | | Gauge tracking the size of a specified map (currently unused but could be used to monitor caches size) | +| operator.sdk.events.received | counter | group, version, kind, (name, namespace)*, scope, event, action | Number of received Kubernetes events | +| operator.sdk.events.delete | counter | group, version, kind, (name, namespace)*, scope | Number of received Kubernetes delete events | +| operator.sdk.reconciliations.started | counter | group, version, kind, (name, namespace)*, scope, reconciliations.retries.last, reconciliations.retries.number | Number of started reconciliations per resource type | +| operator.sdk.reconciliations.failed | counter | group, version, kind, (name, namespace)*, scope, exception | Number of failed reconciliations per resource type | +| operator.sdk.reconciliations.success | counter | group, version, kind, (name, namespace)*, scope | Number of successful reconciliations per resource type | +| operator.sdk.controllers.execution.reconcile.success | counter | controller, type | Number of successful reconciliations per controller | +| operator.sdk.controllers.execution.reconcile.failure | counter | controller, exception | Number of failed reconciliations per controller | +| operator.sdk.controllers.execution.cleanup.success | counter | controller, type | Number of successful cleanups per controller | +| operator.sdk.controllers.execution.cleanup.failure | counter | controller, exception | Number of failed cleanups per controller | + +As you can see all the recorded metrics start with the `operator.sdk` prefix. The tags in parenthesis and marked with an +asterisk (`*`) are not used if the implementation is configured to **not** collect metrics per resource. ## Optimizing Caches From 9c8d77efccfdf01722f91213d923bfc3e20fe3d3 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 23 Mar 2023 14:19:41 +0100 Subject: [PATCH 21/29] feat: avoid emitting tag on empty value --- .../micrometer/MicrometerMetrics.java | 67 +++++++++++-------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index a886e69ab1..69f837fe82 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -30,6 +30,13 @@ public class MicrometerMetrics implements Metrics { private static final String RECONCILIATIONS = "reconciliations."; private static final String RECONCILIATIONS_EXECUTIONS = PREFIX + RECONCILIATIONS + "executions."; private static final String RECONCILIATIONS_QUEUE_SIZE = PREFIX + RECONCILIATIONS + "queue.size."; + private static final String NAME = "name"; + private static final String NAMESPACE = "namespace"; + private static final String GROUP = "group"; + private static final String VERSION = "version"; + private static final String KIND = "kind"; + private static final String SCOPE = "scope"; + private static final String METADATA_PREFIX = "resource."; private final boolean collectPerResourceMetrics; private final MeterRegistry registry; private final Map gauges = new ConcurrentHashMap<>(); @@ -125,22 +132,10 @@ public T timeControllerExecution(ControllerExecution execution) { final var execName = PREFIX + "controllers.execution." + execution.name(); final var resourceID = execution.resourceID(); final var metadata = execution.metadata(); - final var tags = new ArrayList(metadata.size() + 4); + final var tags = new ArrayList(16); tags.add("controller"); tags.add(name); - if (collectPerResourceMetrics) { - tags.addAll(List.of( - "resource.name", resourceID.getName(), - "resource.namespace", resourceID.getNamespace().orElse(""), - "resource.scope", getScope(resourceID))); - } - final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY); - if (gvk != null) { - tags.addAll(List.of( - "resource.group", gvk.group, - "resource.version", gvk.version, - "resource.kind", gvk.kind)); - } + addMetadataTags(resourceID, metadata, tags, true); final var timer = Timer.builder(execName) .tags(tags.toArray(new String[0])) @@ -169,6 +164,35 @@ public T timeControllerExecution(ControllerExecution execution) { } } + private void addMetadataTags(ResourceID resourceID, Map metadata, + List tags, boolean prefixed) { + if (collectPerResourceMetrics) { + addTag(NAME, resourceID.getName(), tags, prefixed); + addTagOmittingOnEmptyValue(NAMESPACE, resourceID.getNamespace().orElse(""), tags, prefixed); + } + addTag(SCOPE, getScope(resourceID), tags, prefixed); + final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY); + if (gvk != null) { + addTagOmittingOnEmptyValue(GROUP, gvk.group, tags, prefixed); + addTag(VERSION, gvk.version, tags, prefixed); + addTag(KIND, gvk.kind, tags, prefixed); + } + } + + private static void addTag(String name, String value, List tags, boolean prefixed) { + tags.add(getPrefixedMetadataTag(name, prefixed)); + tags.add(value); + } + private static void addTagOmittingOnEmptyValue(String name, String value, List tags, boolean prefixed) { + if (value != null && !value.isBlank()) { + addTag(name, value, tags, prefixed); + } + } + + private static String getPrefixedMetadataTag(String tagName, boolean prefixed) { + return prefixed ? METADATA_PREFIX + tagName : tagName; + } + private static String getScope(ResourceID resourceID) { return resourceID.getNamespace().isPresent() ? "namespace" : "cluster"; } @@ -269,23 +293,10 @@ private void incrementCounter(ResourceID id, String counterName, Map 0 ? additionalTags.length : 0; final var metadataNb = metadata != null ? metadata.size() : 0; final var tags = new ArrayList(6 + additionalTagsNb + metadataNb); - if (collectPerResourceMetrics) { - tags.addAll(List.of( - "name", id.getName(), - "namespace", id.getNamespace().orElse(""), - "scope", getScope(id))); - } + addMetadataTags(id, metadata, tags, false); if (additionalTagsNb > 0) { tags.addAll(List.of(additionalTags)); } - if (metadataNb > 0) { - final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY); - if (groupExists(gvk)) { - tags.add("group"); - tags.add(gvk.group); - } - tags.addAll(List.of("version", gvk.version, "kind", gvk.kind)); - } final var counter = registry.counter(PREFIX + counterName, tags.toArray(new String[0])); cleaner.recordAssociation(id, counter); counter.increment(); From 2624f7bbcc28d41f7b491b26dfc82e414bb489d8 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 23 Mar 2023 14:19:55 +0100 Subject: [PATCH 22/29] docs: update --- docs/documentation/features.md | 39 ++++++++++--------- .../micrometer/MicrometerMetrics.java | 2 + .../micrometer/DefaultBehaviorIT.java | 3 +- 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/docs/documentation/features.md b/docs/documentation/features.md index 11404a8b14..f17450d68b 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -807,24 +807,27 @@ MicrometerMetrics.newPerResourceCollectingMicrometerMetricsBuilder(registry) The micrometer implementation records the following metrics: -| Meter name | Type | Tags | Description | -|-----------------------------------------------------------|----------------|---------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------| -| operator.sdk.reconciliations.executions. | gauge | group, version, kind | Number of executions of the named reconciler | -| operator.sdk.reconciliations.queue.size. | gauge | group, version, kind | How many resources are queued to get reconciled by named reconciler | -| operator.sdk..size | gauge map size | | Gauge tracking the size of a specified map (currently unused but could be used to monitor caches size) | -| operator.sdk.events.received | counter | group, version, kind, (name, namespace)*, scope, event, action | Number of received Kubernetes events | -| operator.sdk.events.delete | counter | group, version, kind, (name, namespace)*, scope | Number of received Kubernetes delete events | -| operator.sdk.reconciliations.started | counter | group, version, kind, (name, namespace)*, scope, reconciliations.retries.last, reconciliations.retries.number | Number of started reconciliations per resource type | -| operator.sdk.reconciliations.failed | counter | group, version, kind, (name, namespace)*, scope, exception | Number of failed reconciliations per resource type | -| operator.sdk.reconciliations.success | counter | group, version, kind, (name, namespace)*, scope | Number of successful reconciliations per resource type | -| operator.sdk.controllers.execution.reconcile.success | counter | controller, type | Number of successful reconciliations per controller | -| operator.sdk.controllers.execution.reconcile.failure | counter | controller, exception | Number of failed reconciliations per controller | -| operator.sdk.controllers.execution.cleanup.success | counter | controller, type | Number of successful cleanups per controller | -| operator.sdk.controllers.execution.cleanup.failure | counter | controller, exception | Number of failed cleanups per controller | - -As you can see all the recorded metrics start with the `operator.sdk` prefix. The tags in parenthesis and marked with an -asterisk (`*`) are not used if the implementation is configured to **not** collect metrics per resource. - +| Meter name | Type | Tag names | Description | +|-----------------------------------------------------------|----------------|-----------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------| +| operator.sdk.reconciliations.executions. | gauge | group, version, kind | Number of executions of the named reconciler | +| operator.sdk.reconciliations.queue.size. | gauge | group, version, kind | How many resources are queued to get reconciled by named reconciler | +| operator.sdk..size | gauge map size | | Gauge tracking the size of a specified map (currently unused but could be used to monitor caches size) | +| operator.sdk.events.received | counter | , event, action | Number of received Kubernetes events | +| operator.sdk.events.delete | counter | | Number of received Kubernetes delete events | +| operator.sdk.reconciliations.started | counter | , reconciliations.retries.last, reconciliations.retries.number | Number of started reconciliations per resource type | +| operator.sdk.reconciliations.failed | counter | , exception | Number of failed reconciliations per resource type | +| operator.sdk.reconciliations.success | counter | | Number of successful reconciliations per resource type | +| operator.sdk.controllers.execution.reconcile.success | counter | , controller, type | Number of successful reconciliations per controller | +| operator.sdk.controllers.execution.reconcile.failure | counter | , controller, exception | Number of failed reconciliations per controller | +| operator.sdk.controllers.execution.cleanup.success | counter | , controller, type | Number of successful cleanups per controller | +| operator.sdk.controllers.execution.cleanup.failure | counter | , controller, exception | Number of failed cleanups per controller | + +As you can see all the recorded metrics start with the `operator.sdk` prefix. ``, in the table above, +refers to resource-specific metadata and depends on the considered metric and how the implementation is configured and +could be summed up as follows: `group?, version, kind, [name, namespace?], scope` where the tags in square +brackets (`[]`) won't be present when per-resource collection is disabled and tags followed by a question mark are +omitted if the associated value is empty. Of note, when in the context of controllers' execution metrics, these tag +names are prefixed with `resource.`. This prefix might be removed in a future version for greater consistency. ## Optimizing Caches diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 69f837fe82..0874f7a046 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -367,6 +367,7 @@ private MicrometerMetricsBuilder(MeterRegistry registry) { /** * Configures the instance to collect metrics on a per-resource basis. */ + @SuppressWarnings("unused") public PerResourceCollectingMicrometerMetricsBuilder collectingMetricsPerResource() { collectingPerResourceMetrics = true; return new PerResourceCollectingMicrometerMetricsBuilder(registry); @@ -376,6 +377,7 @@ public PerResourceCollectingMicrometerMetricsBuilder collectingMetricsPerResourc * Configures the instance to only collect metrics per resource **type**, in an aggregate * fashion, instead of per resource instance. */ + @SuppressWarnings("unused") public MicrometerMetricsBuilder notCollectingMetricsPerResource() { collectingPerResourceMetrics = false; return this; diff --git a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java index 7123b8a8c7..6f0388c49b 100644 --- a/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java +++ b/micrometer-support/src/test/java/io/javaoperatorsdk/operator/monitoring/micrometer/DefaultBehaviorIT.java @@ -24,8 +24,7 @@ protected Set preDeleteChecks(ResourceID resourceID) { } @Override - protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) - throws Exception { + protected void postDeleteChecks(ResourceID resourceID, Set recordedMeters) { // meters should be neither recorded, nor removed by default assertThat(registry.getRemoved()).isEmpty(); } From 3fd613ef860efe468f022df23f9465f1d09010b7 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 23 Mar 2023 16:37:57 +0100 Subject: [PATCH 23/29] fix: format [skip ci] --- .../operator/monitoring/micrometer/MicrometerMetrics.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 0874f7a046..11d65e3daf 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -183,7 +183,9 @@ private static void addTag(String name, String value, List tags, boolean tags.add(getPrefixedMetadataTag(name, prefixed)); tags.add(value); } - private static void addTagOmittingOnEmptyValue(String name, String value, List tags, boolean prefixed) { + + private static void addTagOmittingOnEmptyValue(String name, String value, List tags, + boolean prefixed) { if (value != null && !value.isBlank()) { addTag(name, value, tags, prefixed); } From ee88028bbfa81e1e6a9e6ecc57e3d633829356a1 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 24 Mar 2023 11:11:51 +0100 Subject: [PATCH 24/29] refactor: use Tag more directly, avoid unneeded work, use constants --- .../micrometer/MicrometerMetrics.java | 180 +++++++++--------- .../operator/api/monitoring/Metrics.java | 5 +- 2 files changed, 94 insertions(+), 91 deletions(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 11d65e3daf..2087bc4ceb 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -28,6 +28,11 @@ public class MicrometerMetrics implements Metrics { private static final String PREFIX = "operator.sdk."; private static final String RECONCILIATIONS = "reconciliations."; + private static final String RECONCILIATIONS_FAILED = RECONCILIATIONS + "failed"; + private static final String RECONCILIATIONS_SUCCESS = RECONCILIATIONS + "success"; + private static final String RECONCILIATIONS_RETRIES_LAST = RECONCILIATIONS + "retries.last"; + private static final String RECONCILIATIONS_RETRIES_NUMBER = RECONCILIATIONS + "retries.number"; + private static final String RECONCILIATIONS_STARTED = RECONCILIATIONS + "started"; private static final String RECONCILIATIONS_EXECUTIONS = PREFIX + RECONCILIATIONS + "executions."; private static final String RECONCILIATIONS_QUEUE_SIZE = PREFIX + RECONCILIATIONS + "queue.size."; private static final String NAME = "name"; @@ -37,6 +42,18 @@ public class MicrometerMetrics implements Metrics { private static final String KIND = "kind"; private static final String SCOPE = "scope"; private static final String METADATA_PREFIX = "resource."; + private static final String CONTROLLERS_EXECUTION = "controllers.execution."; + private static final String CONTROLLER = "controller"; + private static final String SUCCESS_SUFFIX = ".success"; + private static final String FAILURE_SUFFIX = ".failure"; + private static final String TYPE = "type"; + private static final String EXCEPTION = "exception"; + private static final String EVENT = "event"; + private static final String ACTION = "action"; + private static final String EVENTS_RECEIVED = "events.received"; + private static final String EVENTS_DELETE = "events.delete"; + private static final String CLUSTER = "cluster"; + private static final String SIZE_SUFFIX = ".size"; private final boolean collectPerResourceMetrics; private final MeterRegistry registry; private final Map gauges = new ConcurrentHashMap<>(); @@ -108,37 +125,35 @@ private MicrometerMetrics(MeterRegistry registry, Cleaner cleaner, } @Override - public void controllerRegistered(Controller controller) { - String executingThreadsName = - RECONCILIATIONS_EXECUTIONS + controller.getConfiguration().getName(); + public void controllerRegistered(Controller controller) { + final var configuration = controller.getConfiguration(); + final var name = configuration.getName(); + final var executingThreadsName = RECONCILIATIONS_EXECUTIONS + name; + final var resourceClass = configuration.getResourceClass(); + final var tags = new ArrayList(3); + addGVKTags(GroupVersionKind.gvkFor(resourceClass), tags, false); AtomicInteger executingThreads = - registry.gauge(executingThreadsName, - gvkTags(controller.getConfiguration().getResourceClass()), - new AtomicInteger(0)); + registry.gauge(executingThreadsName, tags, new AtomicInteger(0)); gauges.put(executingThreadsName, executingThreads); - String controllerQueueName = - RECONCILIATIONS_QUEUE_SIZE + controller.getConfiguration().getName(); + final var controllerQueueName = RECONCILIATIONS_QUEUE_SIZE + name; AtomicInteger controllerQueueSize = - registry.gauge(controllerQueueName, - gvkTags(controller.getConfiguration().getResourceClass()), - new AtomicInteger(0)); + registry.gauge(controllerQueueName, tags, new AtomicInteger(0)); gauges.put(controllerQueueName, controllerQueueSize); } @Override public T timeControllerExecution(ControllerExecution execution) { final var name = execution.controllerName(); - final var execName = PREFIX + "controllers.execution." + execution.name(); + final var execName = PREFIX + CONTROLLERS_EXECUTION + execution.name(); final var resourceID = execution.resourceID(); final var metadata = execution.metadata(); - final var tags = new ArrayList(16); - tags.add("controller"); - tags.add(name); + final var tags = new ArrayList(16); + tags.add(Tag.of(CONTROLLER, name)); addMetadataTags(resourceID, metadata, tags, true); final var timer = Timer.builder(execName) - .tags(tags.toArray(new String[0])) + .tags(tags) .publishPercentiles(0.3, 0.5, 0.95) .publishPercentileHistogram() .register(registry); @@ -152,71 +167,35 @@ public T timeControllerExecution(ControllerExecution execution) { }); final var successType = execution.successTypeName(result); registry - .counter(execName + ".success", "controller", name, "type", successType) + .counter(execName + SUCCESS_SUFFIX, CONTROLLER, name, TYPE, successType) .increment(); return result; } catch (Exception e) { final var exception = e.getClass().getSimpleName(); registry - .counter(execName + ".failure", "controller", name, "exception", exception) + .counter(execName + FAILURE_SUFFIX, CONTROLLER, name, EXCEPTION, exception) .increment(); throw e; } } - private void addMetadataTags(ResourceID resourceID, Map metadata, - List tags, boolean prefixed) { - if (collectPerResourceMetrics) { - addTag(NAME, resourceID.getName(), tags, prefixed); - addTagOmittingOnEmptyValue(NAMESPACE, resourceID.getNamespace().orElse(""), tags, prefixed); - } - addTag(SCOPE, getScope(resourceID), tags, prefixed); - final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY); - if (gvk != null) { - addTagOmittingOnEmptyValue(GROUP, gvk.group, tags, prefixed); - addTag(VERSION, gvk.version, tags, prefixed); - addTag(KIND, gvk.kind, tags, prefixed); - } - } - - private static void addTag(String name, String value, List tags, boolean prefixed) { - tags.add(getPrefixedMetadataTag(name, prefixed)); - tags.add(value); - } - - private static void addTagOmittingOnEmptyValue(String name, String value, List tags, - boolean prefixed) { - if (value != null && !value.isBlank()) { - addTag(name, value, tags, prefixed); - } - } - - private static String getPrefixedMetadataTag(String tagName, boolean prefixed) { - return prefixed ? METADATA_PREFIX + tagName : tagName; - } - - private static String getScope(ResourceID resourceID) { - return resourceID.getNamespace().isPresent() ? "namespace" : "cluster"; - } - @Override public void receivedEvent(Event event, Map metadata) { - final String[] tags; if (event instanceof ResourceEvent) { - tags = new String[] {"event", event.getClass().getSimpleName(), "action", - ((ResourceEvent) event).getAction().toString()}; + incrementCounter(event.getRelatedCustomResourceID(), EVENTS_RECEIVED, + metadata, + Tag.of(EVENT, event.getClass().getSimpleName()), + Tag.of(ACTION, ((ResourceEvent) event).getAction().toString())); } else { - tags = new String[] {"event", event.getClass().getSimpleName()}; + incrementCounter(event.getRelatedCustomResourceID(), EVENTS_RECEIVED, + metadata, + Tag.of(EVENT, event.getClass().getSimpleName())); } - - incrementCounter(event.getRelatedCustomResourceID(), "events.received", - metadata, - tags); } @Override public void cleanupDoneFor(ResourceID resourceID, Map metadata) { - incrementCounter(resourceID, "events.delete", metadata); + incrementCounter(resourceID, EVENTS_DELETE, metadata); cleaner.removeMetersFor(resourceID); } @@ -225,12 +204,12 @@ public void cleanupDoneFor(ResourceID resourceID, Map metadata) public void reconcileCustomResource(HasMetadata resource, RetryInfo retryInfoNullable, Map metadata) { Optional retryInfo = Optional.ofNullable(retryInfoNullable); - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "started", + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS_STARTED, metadata, - RECONCILIATIONS + "retries.number", - String.valueOf(retryInfo.map(RetryInfo::getAttemptCount).orElse(0)), - RECONCILIATIONS + "retries.last", - String.valueOf(retryInfo.map(RetryInfo::isLastAttempt).orElse(true))); + Tag.of(RECONCILIATIONS_RETRIES_NUMBER, + String.valueOf(retryInfo.map(RetryInfo::getAttemptCount).orElse(0))), + Tag.of(RECONCILIATIONS_RETRIES_LAST, + String.valueOf(retryInfo.map(RetryInfo::isLastAttempt).orElse(true)))); var controllerQueueSize = gauges.get(RECONCILIATIONS_QUEUE_SIZE + metadata.get(CONTROLLER_NAME)); @@ -239,7 +218,7 @@ public void reconcileCustomResource(HasMetadata resource, RetryInfo retryInfoNul @Override public void finishedReconciliation(HasMetadata resource, Map metadata) { - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "success", metadata); + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS_SUCCESS, metadata); } @Override @@ -269,53 +248,74 @@ public void failedReconciliation(HasMetadata resource, Exception exception, } else if (cause instanceof RuntimeException) { cause = cause.getCause() != null ? cause.getCause() : cause; } - incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS + "failed", metadata, - "exception", - cause.getClass().getSimpleName()); + incrementCounter(ResourceID.fromResource(resource), RECONCILIATIONS_FAILED, metadata, + Tag.of(EXCEPTION, cause.getClass().getSimpleName())); } @Override public > T monitorSizeOf(T map, String name) { - return registry.gaugeMapSize(PREFIX + name + ".size", Collections.emptyList(), map); + return registry.gaugeMapSize(PREFIX + name + SIZE_SUFFIX, Collections.emptyList(), map); } - public static List gvkTags(Class resourceClass) { - final var gvk = GroupVersionKind.gvkFor(resourceClass); - if (groupExists(gvk)) { - return List.of(Tag.of("group", gvk.group), Tag.of("version", gvk.version), - Tag.of("kind", gvk.kind)); - } else { - return List.of(Tag.of("version", gvk.version), Tag.of("kind", gvk.kind)); + + private void addMetadataTags(ResourceID resourceID, Map metadata, + List tags, boolean prefixed) { + if (collectPerResourceMetrics) { + addTag(NAME, resourceID.getName(), tags, prefixed); + addTagOmittingOnEmptyValue(NAMESPACE, resourceID.getNamespace().orElse(null), tags, prefixed); + } + addTag(SCOPE, getScope(resourceID), tags, prefixed); + final var gvk = (GroupVersionKind) metadata.get(Constants.RESOURCE_GVK_KEY); + if (gvk != null) { + addGVKTags(gvk, tags, prefixed); + } + } + + private static void addTag(String name, String value, List tags, boolean prefixed) { + tags.add(Tag.of(getPrefixedMetadataTag(name, prefixed), value)); + } + + private static void addTagOmittingOnEmptyValue(String name, String value, List tags, + boolean prefixed) { + if (value != null && !value.isBlank()) { + addTag(name, value, tags, prefixed); } } + private static String getPrefixedMetadataTag(String tagName, boolean prefixed) { + return prefixed ? METADATA_PREFIX + tagName : tagName; + } + + private static String getScope(ResourceID resourceID) { + return resourceID.getNamespace().isPresent() ? NAMESPACE : CLUSTER; + } + + private static void addGVKTags(GroupVersionKind gvk, List tags, boolean prefixed) { + addTagOmittingOnEmptyValue(GROUP, gvk.group, tags, prefixed); + addTag(VERSION, gvk.version, tags, prefixed); + addTag(KIND, gvk.kind, tags, prefixed); + } + private void incrementCounter(ResourceID id, String counterName, Map metadata, - String... additionalTags) { + Tag... additionalTags) { final var additionalTagsNb = additionalTags != null && additionalTags.length > 0 ? additionalTags.length : 0; final var metadataNb = metadata != null ? metadata.size() : 0; - final var tags = new ArrayList(6 + additionalTagsNb + metadataNb); + final var tags = new ArrayList(6 + additionalTagsNb + metadataNb); addMetadataTags(id, metadata, tags, false); if (additionalTagsNb > 0) { tags.addAll(List.of(additionalTags)); } - final var counter = registry.counter(PREFIX + counterName, tags.toArray(new String[0])); + + final var counter = registry.counter(PREFIX + counterName, tags); cleaner.recordAssociation(id, counter); counter.increment(); } - private static boolean groupExists(GroupVersionKind gvk) { - return gvk.group != null && !gvk.group.isBlank(); - } - protected Set recordedMeterIdsFor(ResourceID resourceID) { return cleaner.recordedMeterIdsFor(resourceID); } - protected Cleaner getCleaner() { - return cleaner; - } - public static class PerResourceCollectingMicrometerMetricsBuilder extends MicrometerMetricsBuilder { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java index 3f73e6c9a4..1ce27d29df 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java @@ -24,7 +24,7 @@ public interface Metrics { /** * Do initialization if necessary; */ - default void controllerRegistered(Controller controller) {} + default void controllerRegistered(Controller controller) {} /** * Called when an event has been accepted by the SDK from an event source, which would result in @@ -39,6 +39,7 @@ default void receivedEvent(Event event, Map metadata) {} * @deprecated Use {@link #reconcileCustomResource(HasMetadata, RetryInfo, Map)} instead */ @Deprecated(forRemoval = true) + @SuppressWarnings("unused") default void reconcileCustomResource(ResourceID resourceID, RetryInfo retryInfo, Map metadata) {} @@ -58,6 +59,7 @@ default void reconcileCustomResource(HasMetadata resource, RetryInfo retryInfo, * @deprecated Use {@link #failedReconciliation(HasMetadata, Exception, Map)} instead */ @Deprecated(forRemoval = true) + @SuppressWarnings("unused") default void failedReconciliation(ResourceID resourceID, Exception exception, Map metadata) {} @@ -112,6 +114,7 @@ default void finishedReconciliation(ResourceID resourceID) { * @deprecated Use {@link #finishedReconciliation(HasMetadata, Map)} instead */ @Deprecated(forRemoval = true) + @SuppressWarnings("unused") default void finishedReconciliation(ResourceID resourceID, Map metadata) {} /** From 40441f093eeaaa1ad04be444283cda8d3c48a4db Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 24 Mar 2023 11:27:07 +0100 Subject: [PATCH 25/29] fix: change will happen instead of might --- docs/documentation/features.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/documentation/features.md b/docs/documentation/features.md index f17450d68b..f70a95d729 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -778,7 +778,7 @@ The micrometer implementation is typically created using one of the provided fac is used, will return either a ready to use instance or a builder allowing users to customized how the implementation behaves, in particular when it comes to the granularity of collected metrics. It is, for example, possible to collect metrics on a per-resource basis via tags that are associated with meters. This is the default, historical behavior but -this might change in a future version of JOSDK because this dramatically increases the cardinality of metrics, which +this will change in a future version of JOSDK because this dramatically increases the cardinality of metrics, which could lead to performance issues. To create a `MicrometerMetrics` implementation that behaves how it has historically behaved, you can just create an From 3bf20453240177431f25d0187df166f2a9e1900d Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 24 Mar 2023 15:06:46 +0100 Subject: [PATCH 26/29] docs: add missing timer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Sébastien CROCQUESEL <88554524+scrocquesel@users.noreply.github.com> --- docs/documentation/features.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/documentation/features.md b/docs/documentation/features.md index f70a95d729..a3d96b7ac5 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -817,6 +817,7 @@ The micrometer implementation records the following metrics: | operator.sdk.reconciliations.started | counter | , reconciliations.retries.last, reconciliations.retries.number | Number of started reconciliations per resource type | | operator.sdk.reconciliations.failed | counter | , exception | Number of failed reconciliations per resource type | | operator.sdk.reconciliations.success | counter | | Number of successful reconciliations per resource type | +| operator.sdk.controllers.execution.reconcile | timer | , controller | Time taken for reconciliations per controller | | operator.sdk.controllers.execution.reconcile.success | counter | , controller, type | Number of successful reconciliations per controller | | operator.sdk.controllers.execution.reconcile.failure | counter | , controller, exception | Number of failed reconciliations per controller | | operator.sdk.controllers.execution.cleanup.success | counter | , controller, type | Number of successful cleanups per controller | From 70462a842ccdbf3c395da0b069be4eb68a0b17be Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 24 Mar 2023 15:22:08 +0100 Subject: [PATCH 27/29] docs: fix wrong & missing information --- docs/documentation/features.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/documentation/features.md b/docs/documentation/features.md index a3d96b7ac5..9a62271e6c 100644 --- a/docs/documentation/features.md +++ b/docs/documentation/features.md @@ -817,18 +817,19 @@ The micrometer implementation records the following metrics: | operator.sdk.reconciliations.started | counter | , reconciliations.retries.last, reconciliations.retries.number | Number of started reconciliations per resource type | | operator.sdk.reconciliations.failed | counter | , exception | Number of failed reconciliations per resource type | | operator.sdk.reconciliations.success | counter | | Number of successful reconciliations per resource type | -| operator.sdk.controllers.execution.reconcile | timer | , controller | Time taken for reconciliations per controller | -| operator.sdk.controllers.execution.reconcile.success | counter | , controller, type | Number of successful reconciliations per controller | -| operator.sdk.controllers.execution.reconcile.failure | counter | , controller, exception | Number of failed reconciliations per controller | -| operator.sdk.controllers.execution.cleanup.success | counter | , controller, type | Number of successful cleanups per controller | -| operator.sdk.controllers.execution.cleanup.failure | counter | , controller, exception | Number of failed cleanups per controller | +| operator.sdk.controllers.execution.reconcile | timer | , controller | Time taken for reconciliations per controller | +| operator.sdk.controllers.execution.cleanup | timer | , controller | Time taken for cleanups per controller | +| operator.sdk.controllers.execution.reconcile.success | counter | controller, type | Number of successful reconciliations per controller | +| operator.sdk.controllers.execution.reconcile.failure | counter | controller, exception | Number of failed reconciliations per controller | +| operator.sdk.controllers.execution.cleanup.success | counter | controller, type | Number of successful cleanups per controller | +| operator.sdk.controllers.execution.cleanup.failure | counter | controller, exception | Number of failed cleanups per controller | As you can see all the recorded metrics start with the `operator.sdk` prefix. ``, in the table above, refers to resource-specific metadata and depends on the considered metric and how the implementation is configured and could be summed up as follows: `group?, version, kind, [name, namespace?], scope` where the tags in square brackets (`[]`) won't be present when per-resource collection is disabled and tags followed by a question mark are omitted if the associated value is empty. Of note, when in the context of controllers' execution metrics, these tag -names are prefixed with `resource.`. This prefix might be removed in a future version for greater consistency. +names are prefixed with `resource.`. This prefix might be removed in a future version for greater consistency. ## Optimizing Caches From 450576176f6908ff46b4f9bf3f05954bd36ee4e6 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 24 Mar 2023 15:22:25 +0100 Subject: [PATCH 28/29] refactor: add constants --- .../operator/processing/Controller.java | 37 ++++++++----------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java index f547fee778..e59d2a789b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java @@ -1,11 +1,6 @@ package io.javaoperatorsdk.operator.processing; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.Set; +import java.util.*; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -25,16 +20,7 @@ import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.monitoring.Metrics; import io.javaoperatorsdk.operator.api.monitoring.Metrics.ControllerExecution; -import io.javaoperatorsdk.operator.api.reconciler.Cleaner; -import io.javaoperatorsdk.operator.api.reconciler.Constants; -import io.javaoperatorsdk.operator.api.reconciler.Context; -import io.javaoperatorsdk.operator.api.reconciler.ContextInitializer; -import io.javaoperatorsdk.operator.api.reconciler.DeleteControl; -import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; -import io.javaoperatorsdk.operator.api.reconciler.EventSourceInitializer; -import io.javaoperatorsdk.operator.api.reconciler.Ignore; -import io.javaoperatorsdk.operator.api.reconciler.Reconciler; -import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.*; import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceNotFoundException; import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceProvider; import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceReferencer; @@ -56,6 +42,13 @@ public class Controller

RegisteredController

{ private static final Logger log = LoggerFactory.getLogger(Controller.class); + private static final String CLEANUP = "cleanup"; + private static final String DELETE = "delete"; + private static final String FINALIZER_NOT_REMOVED = "finalizerNotRemoved"; + private static final String RECONCILE = "reconcile"; + private static final String RESOURCE = "resource"; + private static final String STATUS = "status"; + private static final String BOTH = "both"; private final Reconciler

reconciler; private final ControllerConfiguration

configuration; @@ -103,7 +96,7 @@ public UpdateControl

reconcile(P resource, Context

context) throws Excepti new ControllerExecution<>() { @Override public String name() { - return "reconcile"; + return RECONCILE; } @Override @@ -113,12 +106,12 @@ public String controllerName() { @Override public String successTypeName(UpdateControl

result) { - String successType = "resource"; + String successType = RESOURCE; if (result.isUpdateStatus()) { - successType = "status"; + successType = STATUS; } if (result.isUpdateResourceAndStatus()) { - successType = "both"; + successType = BOTH; } return successType; } @@ -154,7 +147,7 @@ public DeleteControl cleanup(P resource, Context

context) { new ControllerExecution<>() { @Override public String name() { - return "cleanup"; + return CLEANUP; } @Override @@ -164,7 +157,7 @@ public String controllerName() { @Override public String successTypeName(DeleteControl deleteControl) { - return deleteControl.isRemoveFinalizer() ? "delete" : "finalizerNotRemoved"; + return deleteControl.isRemoveFinalizer() ? DELETE : FINALIZER_NOT_REMOVED; } @Override From 31d1326010aa702e57e39579a0a03b116a072c05 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 27 Mar 2023 15:46:34 +0200 Subject: [PATCH 29/29] fix: wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [skip ci] Co-authored-by: Attila Mészáros --- .../operator/monitoring/micrometer/MicrometerMetrics.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 2087bc4ceb..61ff1562ee 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -115,7 +115,7 @@ public static PerResourceCollectingMicrometerMetricsBuilder newPerResourceCollec * * @param registry the {@link MeterRegistry} instance to use for metrics recording * @param cleaner the {@link Cleaner} to use - * @param collectingPerResourceMetrics whether or not to collect per resource metrics + * @param collectingPerResourceMetrics whether to collect per resource metrics */ private MicrometerMetrics(MeterRegistry registry, Cleaner cleaner, boolean collectingPerResourceMetrics) {