From 27cf90d04f56b897ae459a2562f481ae0b94c3fd Mon Sep 17 00:00:00 2001 From: Ed Coleman Date: Fri, 10 May 2024 12:59:26 +0000 Subject: [PATCH 1/3] Use micrometer noop classes for default meter initialization - removes custom NoOpDistributionSummary - adds default initializations for other meter types --- .../metrics/NoOpDistributionSummary.java | 83 ------------------- .../accumulo/server/metrics/NoopMetrics.java | 79 ++++++++++++++++++ .../server/metrics/ThriftMetrics.java | 4 +- ...nSummaryTest.java => NoopMetricsTest.java} | 20 ++++- .../server/metrics/ThriftMetricsTest.java | 3 +- .../manager/metrics/ReplicationMetrics.java | 3 +- .../accumulo/tserver/ScanServerMetrics.java | 5 +- .../metrics/TabletServerMinCMetrics.java | 5 +- .../metrics/TabletServerScanMetrics.java | 16 ++-- .../metrics/TabletServerUpdateMetrics.java | 16 ++-- 10 files changed, 125 insertions(+), 109 deletions(-) delete mode 100644 server/base/src/main/java/org/apache/accumulo/server/metrics/NoOpDistributionSummary.java create mode 100644 server/base/src/main/java/org/apache/accumulo/server/metrics/NoopMetrics.java rename server/base/src/test/java/org/apache/accumulo/server/metrics/{NoOpDistributionSummaryTest.java => NoopMetricsTest.java} (67%) diff --git a/server/base/src/main/java/org/apache/accumulo/server/metrics/NoOpDistributionSummary.java b/server/base/src/main/java/org/apache/accumulo/server/metrics/NoOpDistributionSummary.java deleted file mode 100644 index 262dc0802ac..00000000000 --- a/server/base/src/main/java/org/apache/accumulo/server/metrics/NoOpDistributionSummary.java +++ /dev/null @@ -1,83 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.accumulo.server.metrics; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import io.micrometer.core.instrument.DistributionSummary; -import io.micrometer.core.instrument.Tag; -import io.micrometer.core.instrument.Tags; -import io.micrometer.core.instrument.distribution.HistogramSnapshot; - -/** - * Provides a default DistributionSummary that does not do anything. This can be used to prevent NPE - * if metrics have not been initialized when a DistributionSummary is expected. - *

- * Normally DistributionSummaries are created using a builder that takes a registry. - * - * 

- *   DistributionSummary distSum;
- *   ...
- *   public void registerMetrics(MeterRegistry registry) {
- *      ...
- *      distSum = DistributionSummary.builder("metric name").description("...").register(registry);
- *      ...
- *   }
- *
- * - * Until the registration is called, the instance variable is null. If code then tries to update the - * metric, a NPE is thrown. Using this class to provide a default value prevents this from occurring - * and on registration, it is replaced with a valid instance. - */ -public class NoOpDistributionSummary implements DistributionSummary { - - private static final Logger LOG = LoggerFactory.getLogger(NoOpDistributionSummary.class); - - @Override - public void record(double v) { - LOG.debug("record ignored - distribution summary will not be available."); - } - - @Override - public long count() { - return 0; - } - - @Override - public double totalAmount() { - return 0; - } - - @Override - public double max() { - return 0; - } - - @Override - public HistogramSnapshot takeSnapshot() { - return new HistogramSnapshot(0L, 0.0, 0.0, null, null, null); - } - - @Override - public Id getId() { - return new Id("thrift.metrics.uninitialized", Tags.of(Tag.of("none", "uninitialized")), null, - "placeholder for uninitialized thrift metrics", Type.OTHER); - } -} diff --git a/server/base/src/main/java/org/apache/accumulo/server/metrics/NoopMetrics.java b/server/base/src/main/java/org/apache/accumulo/server/metrics/NoopMetrics.java new file mode 100644 index 00000000000..e8f497be238 --- /dev/null +++ b/server/base/src/main/java/org/apache/accumulo/server/metrics/NoopMetrics.java @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.server.metrics; + +import java.util.concurrent.atomic.AtomicInteger; + +import io.micrometer.core.instrument.Counter; +import io.micrometer.core.instrument.DistributionSummary; +import io.micrometer.core.instrument.FunctionTimer; +import io.micrometer.core.instrument.Gauge; +import io.micrometer.core.instrument.Meter; +import io.micrometer.core.instrument.Tags; +import io.micrometer.core.instrument.Timer; +import io.micrometer.core.instrument.noop.NoopCounter; +import io.micrometer.core.instrument.noop.NoopDistributionSummary; +import io.micrometer.core.instrument.noop.NoopFunctionTimer; +import io.micrometer.core.instrument.noop.NoopGauge; +import io.micrometer.core.instrument.noop.NoopTimer; + +/** + * Convenience utility class to return micrometer noop metrics. Initialization of the metrics system + * registry can be delayed so that common tags with values determined at runtime (for example, port + * numbers). Initializing meters that are create from the registry at initialization with a noop + * implementation prevents NPEs if something tries to record a metric value before the + * initialization has run. + */ +public class NoopMetrics { + private static AtomicInteger idCount = new AtomicInteger(0); + + private NoopMetrics() {} + + public static Counter useNoopCounter() { + return new NoopCounter(noopId(Meter.Type.COUNTER)); + } + + public static Gauge useNoopGauge() { + return new NoopGauge(noopId(Meter.Type.GAUGE)); + } + + public static DistributionSummary useNoopDistributionSummary() { + return new NoopDistributionSummary(noopId(Meter.Type.DISTRIBUTION_SUMMARY)); + } + + public static Timer useNoopTimer() { + return new NoopTimer(noopId(Meter.Type.TIMER)); + } + + public static FunctionTimer useNoopFunctionTimer() { + return new NoopFunctionTimer(noopId(Meter.Type.TIMER)); + } + + /** + * provide a unique id to guard against the id being used as a key. + */ + private static Meter.Id noopId(final Meter.Type type) { + return new Meter.Id(uniqueName(), Tags.of("noop", "none"), "", "", type); + } + + private static String uniqueName() { + return "noop" + idCount.incrementAndGet(); + } + +} diff --git a/server/base/src/main/java/org/apache/accumulo/server/metrics/ThriftMetrics.java b/server/base/src/main/java/org/apache/accumulo/server/metrics/ThriftMetrics.java index 1d113d3e1e7..c6288752ad9 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/metrics/ThriftMetrics.java +++ b/server/base/src/main/java/org/apache/accumulo/server/metrics/ThriftMetrics.java @@ -25,8 +25,8 @@ public class ThriftMetrics implements MetricsProducer { - private DistributionSummary idle = new NoOpDistributionSummary(); - private DistributionSummary execute = new NoOpDistributionSummary(); + private DistributionSummary idle = NoopMetrics.useNoopDistributionSummary(); + private DistributionSummary execute = NoopMetrics.useNoopDistributionSummary(); public ThriftMetrics() {} diff --git a/server/base/src/test/java/org/apache/accumulo/server/metrics/NoOpDistributionSummaryTest.java b/server/base/src/test/java/org/apache/accumulo/server/metrics/NoopMetricsTest.java similarity index 67% rename from server/base/src/test/java/org/apache/accumulo/server/metrics/NoOpDistributionSummaryTest.java rename to server/base/src/test/java/org/apache/accumulo/server/metrics/NoopMetricsTest.java index d374791d027..073e467b187 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/metrics/NoOpDistributionSummaryTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/metrics/NoopMetricsTest.java @@ -19,13 +19,29 @@ package org.apache.accumulo.server.metrics; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import org.junit.jupiter.api.Test; -class NoOpDistributionSummaryTest { +import io.micrometer.core.instrument.Counter; +import io.micrometer.core.instrument.DistributionSummary; +import io.micrometer.core.instrument.Gauge; + +public class NoopMetricsTest { + @Test + public void testNames() { + Counter c1 = NoopMetrics.useNoopCounter(); + Counter c2 = NoopMetrics.useNoopCounter(); + + Gauge g1 = NoopMetrics.useNoopGauge(); + + assertNotEquals(c1.getId(), c2.getId()); + assertNotEquals(c1.getId(), g1.getId()); + } + @Test public void testNoOp() { - NoOpDistributionSummary noop = new NoOpDistributionSummary(); + DistributionSummary noop = NoopMetrics.useNoopDistributionSummary(); assertDoesNotThrow(() -> noop.getId()); assertDoesNotThrow(() -> noop.takeSnapshot()); assertDoesNotThrow(() -> noop.max()); diff --git a/server/base/src/test/java/org/apache/accumulo/server/metrics/ThriftMetricsTest.java b/server/base/src/test/java/org/apache/accumulo/server/metrics/ThriftMetricsTest.java index 02250959763..91730d0582e 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/metrics/ThriftMetricsTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/metrics/ThriftMetricsTest.java @@ -31,6 +31,7 @@ import io.micrometer.core.instrument.DistributionSummary; import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.noop.NoopDistributionSummary; import io.micrometer.core.instrument.simple.SimpleMeterRegistry; class ThriftMetricsTest { @@ -54,7 +55,7 @@ void registerMetrics() { registry.forEachMeter(m -> { LOG.trace("meter: {}", m.getId()); assertInstanceOf(DistributionSummary.class, m); - assertFalse(m instanceof NoOpDistributionSummary); + assertFalse(m instanceof NoopDistributionSummary); }); assertTrue(registry.get(METRICS_THRIFT_IDLE).summary().count() > 0); assertTrue(registry.get(METRICS_THRIFT_EXECUTE).summary().count() > 0); diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/metrics/ReplicationMetrics.java b/server/manager/src/main/java/org/apache/accumulo/manager/metrics/ReplicationMetrics.java index 931fb302152..181df66c695 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/metrics/ReplicationMetrics.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/metrics/ReplicationMetrics.java @@ -36,6 +36,7 @@ import org.apache.accumulo.core.replication.ReplicationTarget; import org.apache.accumulo.core.util.threads.ThreadPools; import org.apache.accumulo.manager.Manager; +import org.apache.accumulo.server.metrics.NoopMetrics; import org.apache.accumulo.server.replication.ReplicationUtil; import org.apache.hadoop.fs.Path; import org.slf4j.Logger; @@ -53,7 +54,7 @@ public class ReplicationMetrics implements MetricsProducer { private final ReplicationUtil replicationUtil; private final Map pathModTimes; - private Timer replicationQueueTimer; + private Timer replicationQueueTimer = NoopMetrics.useNoopTimer(); private AtomicLong pendingFiles; private AtomicInteger numPeers; private AtomicInteger maxReplicationThreads; diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServerMetrics.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServerMetrics.java index 1a516b597bb..742c7c4098f 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServerMetrics.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServerMetrics.java @@ -21,6 +21,7 @@ import org.apache.accumulo.core.dataImpl.KeyExtent; import org.apache.accumulo.core.metadata.schema.TabletMetadata; import org.apache.accumulo.core.metrics.MetricsProducer; +import org.apache.accumulo.server.metrics.NoopMetrics; import com.github.benmanes.caffeine.cache.LoadingCache; @@ -31,8 +32,8 @@ public class ScanServerMetrics implements MetricsProducer { - private Timer reservationTimer; - private Counter busyTimeoutCount; + private Timer reservationTimer = NoopMetrics.useNoopTimer();; + private Counter busyTimeoutCount = NoopMetrics.useNoopCounter();; private final LoadingCache tabletMetadataCache; diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerMinCMetrics.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerMinCMetrics.java index a7f402343c7..bf95b002620 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerMinCMetrics.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerMinCMetrics.java @@ -21,14 +21,15 @@ import java.time.Duration; import org.apache.accumulo.core.metrics.MetricsProducer; +import org.apache.accumulo.server.metrics.NoopMetrics; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Timer; public class TabletServerMinCMetrics implements MetricsProducer { - private Timer activeMinc; - private Timer queuedMinc; + private Timer activeMinc = NoopMetrics.useNoopTimer(); + private Timer queuedMinc = NoopMetrics.useNoopTimer(); public void addActive(long value) { activeMinc.record(Duration.ofMillis(value)); diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java index bf22d1d297e..c2c8dd0d322 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java @@ -23,7 +23,7 @@ import java.util.concurrent.atomic.LongAdder; import org.apache.accumulo.core.metrics.MetricsProducer; -import org.apache.accumulo.server.metrics.NoOpDistributionSummary; +import org.apache.accumulo.server.metrics.NoopMetrics; import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.DistributionSummary; @@ -34,13 +34,13 @@ public class TabletServerScanMetrics implements MetricsProducer { private final AtomicInteger openFiles = new AtomicInteger(0); - private Timer scans; - private DistributionSummary resultsPerScan = new NoOpDistributionSummary(); - private DistributionSummary yields = new NoOpDistributionSummary(); - private Counter startScanCalls; - private Counter continueScanCalls; - private Counter closeScanCalls; - private Counter busyTimeoutCount; + private Timer scans = NoopMetrics.useNoopTimer(); + private DistributionSummary resultsPerScan = NoopMetrics.useNoopDistributionSummary(); + private DistributionSummary yields = NoopMetrics.useNoopDistributionSummary(); + private Counter startScanCalls = NoopMetrics.useNoopCounter(); + private Counter continueScanCalls = NoopMetrics.useNoopCounter();; + private Counter closeScanCalls = NoopMetrics.useNoopCounter();; + private Counter busyTimeoutCount = NoopMetrics.useNoopCounter();; private final LongAdder lookupCount = new LongAdder(); private final LongAdder queryResultCount = new LongAdder(); diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerUpdateMetrics.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerUpdateMetrics.java index 09ad197b6dd..a369458b6ca 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerUpdateMetrics.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerUpdateMetrics.java @@ -21,7 +21,7 @@ import java.time.Duration; import org.apache.accumulo.core.metrics.MetricsProducer; -import org.apache.accumulo.server.metrics.NoOpDistributionSummary; +import org.apache.accumulo.server.metrics.NoopMetrics; import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.DistributionSummary; @@ -30,13 +30,13 @@ public class TabletServerUpdateMetrics implements MetricsProducer { - private Counter permissionErrorsCounter; - private Counter unknownTabletErrorsCounter; - private Counter constraintViolationsCounter; - private Timer commitPrepStat; - private Timer walogWriteTimeStat; - private Timer commitTimeStat; - private DistributionSummary mutationArraySizeStat = new NoOpDistributionSummary(); + private Counter permissionErrorsCounter = NoopMetrics.useNoopCounter(); + private Counter unknownTabletErrorsCounter = NoopMetrics.useNoopCounter(); + private Counter constraintViolationsCounter = NoopMetrics.useNoopCounter(); + private Timer commitPrepStat = NoopMetrics.useNoopTimer(); + private Timer walogWriteTimeStat = NoopMetrics.useNoopTimer();; + private Timer commitTimeStat = NoopMetrics.useNoopTimer();; + private DistributionSummary mutationArraySizeStat = NoopMetrics.useNoopDistributionSummary(); public void addPermissionErrors(long value) { permissionErrorsCounter.increment(value); From b706a065c7dff7bf3f803d86de7cd420279e8bdb Mon Sep 17 00:00:00 2001 From: Ed Coleman Date: Tue, 28 May 2024 21:51:20 +0000 Subject: [PATCH 2/3] clean-up --- .../accumulo/test/metrics/MetricsIT.java | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/test/src/main/java/org/apache/accumulo/test/metrics/MetricsIT.java b/test/src/main/java/org/apache/accumulo/test/metrics/MetricsIT.java index 5d9c833be54..f0b53992b48 100644 --- a/test/src/main/java/org/apache/accumulo/test/metrics/MetricsIT.java +++ b/test/src/main/java/org/apache/accumulo/test/metrics/MetricsIT.java @@ -98,14 +98,22 @@ public void confirmMetricsPublished() throws Exception { doWorkToGenerateMetrics(); cluster.stop(); + // meter names sorted and formatting disabled to make it easier to diff changes + // @formatter:off + Set unexpectedMetrics = + Set.of(METRICS_COMPACTOR_MAJC_STUCK, + METRICS_REPLICATION_QUEUE, + METRICS_SCAN_YIELDS, + METRICS_UPDATE_ERRORS); - Set unexpectedMetrics = Set.of(METRICS_SCAN_YIELDS, METRICS_UPDATE_ERRORS, - METRICS_REPLICATION_QUEUE, METRICS_COMPACTOR_MAJC_STUCK, METRICS_SCAN_BUSY_TIMEOUT_COUNTER); // add sserver as flaky until scan server included in mini tests. - Set flakyMetrics = Set.of(METRICS_GC_WAL_ERRORS, METRICS_FATE_TYPE_IN_PROGRESS, - METRICS_SCAN_BUSY_TIMEOUT_COUNTER, METRICS_SCAN_RESERVATION_TOTAL_TIMER, - METRICS_SCAN_RESERVATION_WRITEOUT_TIMER, METRICS_SCAN_RESERVATION_CONFLICT_COUNTER, - METRICS_SCAN_TABLET_METADATA_CACHE); + Set flakyMetrics = Set.of(METRICS_FATE_TYPE_IN_PROGRESS, + METRICS_SCAN_BUSY_TIMEOUT_COUNTER, + METRICS_SCAN_RESERVATION_CONFLICT_COUNTER, + METRICS_SCAN_RESERVATION_TOTAL_TIMER, + METRICS_SCAN_RESERVATION_WRITEOUT_TIMER, + METRICS_SCAN_TABLET_METADATA_CACHE); + // @formatter:on Map expectedMetricNames = this.getMetricFields(); flakyMetrics.forEach(expectedMetricNames::remove); // might not see these From a1cb1cf1c2da7348f9efc98fb6b98cf8bad5912a Mon Sep 17 00:00:00 2001 From: Ed Coleman Date: Tue, 28 May 2024 22:11:49 +0000 Subject: [PATCH 3/3] minor doc clean-up --- .../java/org/apache/accumulo/server/metrics/NoopMetrics.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/base/src/main/java/org/apache/accumulo/server/metrics/NoopMetrics.java b/server/base/src/main/java/org/apache/accumulo/server/metrics/NoopMetrics.java index e8f497be238..065106e2bbc 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/metrics/NoopMetrics.java +++ b/server/base/src/main/java/org/apache/accumulo/server/metrics/NoopMetrics.java @@ -36,12 +36,12 @@ /** * Convenience utility class to return micrometer noop metrics. Initialization of the metrics system * registry can be delayed so that common tags with values determined at runtime (for example, port - * numbers). Initializing meters that are create from the registry at initialization with a noop + * numbers). Initializing meters that are created from the registry at initialization with a noop * implementation prevents NPEs if something tries to record a metric value before the * initialization has run. */ public class NoopMetrics { - private static AtomicInteger idCount = new AtomicInteger(0); + private final static AtomicInteger idCount = new AtomicInteger(0); private NoopMetrics() {}