From d0dd094291b43a69fdb1ad4131dad91cca40e9b0 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Wed, 15 Dec 2021 13:00:38 +0100 Subject: No functional changes --- .../main/java/com/yahoo/metrics/simple/Bucket.java | 2 +- .../com/yahoo/metrics/simple/MetricAggregator.java | 5 ++-- .../com/yahoo/metrics/simple/MetricManager.java | 8 +++--- .../com/yahoo/metrics/simple/MetricReceiver.java | 4 ++- .../metrics/simple/jdisc/SimpleMetricConsumer.java | 2 +- .../jdisc/metric/ForwardingMetricConsumer.java | 6 ++--- .../container/jdisc/metric/MetricProvider.java | 4 +-- .../ai/vespa/metricsproxy/core/VespaMetrics.java | 2 +- .../java/ai/vespa/metricsproxy/metric/Metrics.java | 30 +++------------------- .../metricsproxy/node/NodeMetricGatherer.java | 2 -- .../ai/vespa/metricsproxy/metric/MetricsTest.java | 6 ++--- .../ai/vespa/metricsproxy/rpc/RpcMetricsTest.java | 18 +++++++++---- .../metricsproxy/service/ContainerServiceTest.java | 2 +- .../metricsproxy/service/MetricsFetcherTest.java | 14 ++++++++-- .../metricsproxy/service/SystemPollerTest.java | 2 +- .../metricsproxy/service/VespaServiceTest.java | 18 ++++++++++--- 16 files changed, 66 insertions(+), 59 deletions(-) diff --git a/container-core/src/main/java/com/yahoo/metrics/simple/Bucket.java b/container-core/src/main/java/com/yahoo/metrics/simple/Bucket.java index 9162571830d..90e0115736f 100644 --- a/container-core/src/main/java/com/yahoo/metrics/simple/Bucket.java +++ b/container-core/src/main/java/com/yahoo/metrics/simple/Bucket.java @@ -175,7 +175,7 @@ public class Bucket { @Override public String toString() { - return "Bucket [values=" + (values != null ? toString(values.entrySet(), 3) : null) + "]"; + return "Bucket [values=" + toString(values.entrySet(), 3) + "]"; } private String toString(Collection collection, int maxLen) { diff --git a/container-core/src/main/java/com/yahoo/metrics/simple/MetricAggregator.java b/container-core/src/main/java/com/yahoo/metrics/simple/MetricAggregator.java index 3839de28294..5fd9afa3e4c 100644 --- a/container-core/src/main/java/com/yahoo/metrics/simple/MetricAggregator.java +++ b/container-core/src/main/java/com/yahoo/metrics/simple/MetricAggregator.java @@ -23,8 +23,9 @@ class MetricAggregator implements Runnable { private long fromMillis; private final DimensionCache dimensions; - MetricAggregator(ThreadLocalDirectory metricsCollection, AtomicReference currentSnapshot, - ManagerConfig settings) { + MetricAggregator(ThreadLocalDirectory metricsCollection, + AtomicReference currentSnapshot, + ManagerConfig settings) { if (settings.reportPeriodSeconds() < 10) { throw new IllegalArgumentException("Do not use this metrics implementation" + " if report periods of less than 10 seconds is desired."); diff --git a/container-core/src/main/java/com/yahoo/metrics/simple/MetricManager.java b/container-core/src/main/java/com/yahoo/metrics/simple/MetricManager.java index 40242285486..99465a55a31 100644 --- a/container-core/src/main/java/com/yahoo/metrics/simple/MetricManager.java +++ b/container-core/src/main/java/com/yahoo/metrics/simple/MetricManager.java @@ -21,11 +21,11 @@ import java.util.logging.Level; */ public class MetricManager extends AbstractComponent implements Provider { - private static Logger log = Logger.getLogger(MetricManager.class.getName()); + private static final Logger log = Logger.getLogger(MetricManager.class.getName()); private final ScheduledThreadPoolExecutor executor; private final MetricReceiver receiver; - private ThreadLocalDirectory metricsCollection; + private final ThreadLocalDirectory metricsCollection; public MetricManager(ManagerConfig settings) { this(settings, new MetricUpdater()); @@ -42,7 +42,9 @@ public class MetricManager extends AbstractComponent implements Provider metricsCollection; + + // A reference to the current snapshot. The *reference* is shared with MetricsAggregator and updated from there :-/ private final AtomicReference currentSnapshot; // metricSettings is volatile for reading, the lock is for updates @@ -91,7 +93,7 @@ public class MetricReceiver { /** Gathers all data since last snapshot */ public Bucket getSnapshot() { - final Bucket merged = new Bucket(); + Bucket merged = new Bucket(); for (Bucket b : collection.fetch()) { merged.merge(b, true); } diff --git a/container-core/src/main/java/com/yahoo/metrics/simple/jdisc/SimpleMetricConsumer.java b/container-core/src/main/java/com/yahoo/metrics/simple/jdisc/SimpleMetricConsumer.java index 12f27e9b0ca..bea17bd91bc 100644 --- a/container-core/src/main/java/com/yahoo/metrics/simple/jdisc/SimpleMetricConsumer.java +++ b/container-core/src/main/java/com/yahoo/metrics/simple/jdisc/SimpleMetricConsumer.java @@ -14,7 +14,7 @@ import com.yahoo.metrics.simple.Sample; import com.yahoo.metrics.simple.UntypedMetric.AssumedType; /** - * The single user facing part of the JDisc interfaces of simple metrics. + * The metrics consumer in JDisc. * * @author Steinar Knutsen */ diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/metric/ForwardingMetricConsumer.java b/container-disc/src/main/java/com/yahoo/container/jdisc/metric/ForwardingMetricConsumer.java index aa6f2fa3465..18aea820a1e 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/metric/ForwardingMetricConsumer.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/metric/ForwardingMetricConsumer.java @@ -8,9 +8,9 @@ import com.yahoo.jdisc.application.MetricConsumer; import java.util.Map; /** - *

If more than one {@link MetricConsumerFactory} is registered in a container, calls to Metric need to be - * forwarded to all the underlying MetricConsumers. That is the responsibility of this class. Instances of this - * class is created by the {@link MetricConsumerProvider} in those cases.

+ * If more than one {@link MetricConsumerFactory} is registered in a container, calls to Metric need to be + * forwarded to all the underlying MetricConsumers. That is the responsibility of this class. + * Instances of this class is created by the {@link MetricConsumerProvider} in those cases. * * @author Simon Thoresen Hult */ diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/metric/MetricProvider.java b/container-disc/src/main/java/com/yahoo/container/jdisc/metric/MetricProvider.java index 4108ca729a0..b547b3dd897 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/metric/MetricProvider.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/metric/MetricProvider.java @@ -6,9 +6,9 @@ import com.yahoo.jdisc.Metric; import com.yahoo.jdisc.application.MetricConsumer; /** - *

This class implements a {@link Provider} component of Metric. Because this class depends on {@link + * An implementation of {@link Provider} component of Metric. Because this class depends on {@link * MetricConsumerProvider}, any change to the consumer configuration will trigger reconfiguration of this component, - * which in turn triggers reconfiguration of any component that depends on Metric.

+ * which in turn triggers reconfiguration of any component that depends on Metric. * * @author Simon Thoresen Hult */ diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/core/VespaMetrics.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/core/VespaMetrics.java index 3629e81582a..7a3f260cd34 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/core/VespaMetrics.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/core/VespaMetrics.java @@ -171,7 +171,7 @@ public class VespaMetrics { builder.putDimension(METRIC_TYPE_DIMENSION_ID, "system") .putDimension(INSTANCE_DIMENSION_ID, service.getInstanceName()) .putDimensions(service.getDimensions()) - .putMetrics(systemMetrics.getMetrics()); + .putMetrics(systemMetrics.list()); builder.addConsumers(metricsConsumers.getAllConsumers()); return Optional.of(builder); diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/Metrics.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/Metrics.java index 571ffc030de..8450d9f6be7 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/Metrics.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/Metrics.java @@ -1,16 +1,12 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package ai.vespa.metricsproxy.metric; -import ai.vespa.metricsproxy.metric.model.MetricId; - import java.util.ArrayList; import java.util.Collections; import java.util.List; /** * Once a getter is called, the instance is frozen and no more metrics can be added. - * - * @author Unknown */ // TODO: remove timestamp, only used as temporary storage. // TODO: instances of this class can probably be replaced by a simple freezable map. @@ -52,37 +48,17 @@ public class Metrics { this.metrics.add(m); } - /** - * Get the size of the metrics covered. Note that this might also contain expired metrics - * - * @return size of metrics - */ + /** Returns the size of the metrics covered. Note that this might also contain expired metrics. */ public int size() { return this.metrics.size(); } - /** - * TODO: Remove, might be multiple metrics with same name but different dimensions - * - * @param key metric name - * @return the metric, or null - */ - public Metric getMetric(MetricId key) { - isFrozen = true; - for (Metric m: metrics) { - if (m.getName().equals(key)) { - return m; - } - } - return null; - } - - public List getMetrics() { + public List list() { isFrozen = true; return Collections.unmodifiableList(metrics); } - + @Override public String toString() { StringBuilder sb = new StringBuilder(); for (Metric m : metrics) { diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/NodeMetricGatherer.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/NodeMetricGatherer.java index 5aa95e9efc0..9686bb1bf6b 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/NodeMetricGatherer.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/NodeMetricGatherer.java @@ -30,8 +30,6 @@ import static ai.vespa.metricsproxy.node.ServiceHealthGatherer.gatherServiceHeal * * @author olaa */ - - public class NodeMetricGatherer { private final VespaServices vespaServices; diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/MetricsTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/MetricsTest.java index 809eb441c14..1cd0281b7cd 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/MetricsTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/MetricsTest.java @@ -30,7 +30,7 @@ public class MetricsTest { Map map = new HashMap<>(); - for (Metric metric: m.getMetrics()) { + for (Metric metric: m.list()) { String k = metric.getName().id; assertThat(map.containsKey(k), is(false)); @@ -45,8 +45,8 @@ public class MetricsTest { public void testBasicMetric() { Metrics m = new Metrics(); m.add(new Metric(toMetricId("count"), 1, System.currentTimeMillis() / 1000)); - assertThat(m.getMetrics().size(), is(1)); - assertThat(m.getMetrics().get(0).getName(), is(toMetricId("count"))); + assertThat(m.list().size(), is(1)); + assertThat(m.list().get(0).getName(), is(toMetricId("count"))); } @Test diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java index d06ca43f8f8..214df36d6a1 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java @@ -108,9 +108,9 @@ public class RpcMetricsTest { Metrics metrics = qrserver.getMetrics(); assertThat("Fetched number of metrics is not correct", metrics.size(), is(2)); - Metric m = metrics.getMetric(toMetricId("foo.count")); + Metric m = getMetric("foo.count", metrics); assertNotNull("Did not find expected metric with name 'foo.count'", m); - Metric m2 = metrics.getMetric(toMetricId("bar.count")); + Metric m2 = getMetric("bar.count", metrics); assertNotNull("Did not find expected metric with name 'bar.count'", m2); try (RpcClient rpcClient = new RpcClient(tester.rpcPort())) { @@ -134,6 +134,14 @@ public class RpcMetricsTest { } } + public Metric getMetric(String metric, Metrics metrics) { + for (Metric m: metrics.list()) { + if (m.getName().equals(toMetricId(metric))) + return m; + } + return null; + } + private static void verifyMetricsFromRpcRequest(VespaService service, RpcClient client) throws IOException { String jsonResponse = getMetricsForYamas(service.getMonitoringName().id, client).trim(); ArrayNode metrics = (ArrayNode) jsonMapper.readTree(jsonResponse).get("metrics"); @@ -161,7 +169,7 @@ public class RpcMetricsTest { private void verfiyMetricsFromServiceObject(VespaService service) { Metrics storageMetrics = service.getMetrics(); assertThat(storageMetrics.size(), is(2)); - Metric foo = storageMetrics.getMetric(toMetricId("foo.count")); + Metric foo = getMetric("foo.count", storageMetrics); assertNotNull("Did not find expected metric with name 'foo.count'", foo); assertThat("Expected 2 dimensions for metric foo", foo.getDimensions().size(), is(2)); assertThat("Metric foo did not contain correct dimension mapping for key = foo.count", foo.getDimensions().containsKey(toDimensionId("foo")), is(true)); @@ -187,10 +195,10 @@ public class RpcMetricsTest { assertThat(services.size(), is(1)); Metrics metrics = services.get(0).getMetrics(); assertThat("Fetched number of metrics is not correct", metrics.size(), is(2)); - Metric m = metrics.getMetric(toMetricId("foo.count")); + Metric m = getMetric("foo.count", metrics); assertNotNull("Did not find expected metric with name 'foo.count'", m); - Metric m2 = metrics.getMetric(toMetricId("bar.count")); + Metric m2 = getMetric("bar.count", metrics); assertNotNull("Did not find expected metric with name 'bar'", m2); try (RpcClient rpcClient = new RpcClient(tester.rpcPort())) { diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/ContainerServiceTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/ContainerServiceTest.java index 7b047cee0a7..44f0e9608ac 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/ContainerServiceTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/ContainerServiceTest.java @@ -40,7 +40,7 @@ public class ContainerServiceTest { public void testMultipleQueryDimensions() { int count = 0; VespaService service = VespaService.create("service1", "id", httpServer.port()); - for (Metric m : service.getMetrics().getMetrics()) { + for (Metric m : service.getMetrics().list()) { if (m.getName().equals(toMetricId("queries.rate"))) { count++; System.out.println("Name: " + m.getName() + " value: " + m.getValue()); diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java index b571ddcf84b..3af317ab36b 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/MetricsFetcherTest.java @@ -7,6 +7,7 @@ import ai.vespa.metricsproxy.metric.Metrics; import ai.vespa.metricsproxy.metric.model.MetricId; import org.junit.Test; +import static ai.vespa.metricsproxy.metric.model.MetricId.toMetricId; import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertThat; @@ -35,8 +36,8 @@ public class MetricsFetcherTest { String jsonData = TestUtil.getFileContents("metrics-state.json"); Metrics metrics = fetch(jsonData); assertThat(metrics.size(), is(10)); - assertThat(metrics.getMetric(MetricId.toMetricId("query_hits.count")).getValue().intValue(), is(28)); - assertThat(metrics.getMetric(MetricId.toMetricId("queries.rate")).getValue().doubleValue(), is(0.4667)); + assertThat(getMetric("query_hits.count", metrics).getValue().intValue(), is(28)); + assertThat(getMetric("queries.rate", metrics).getValue().doubleValue(), is(0.4667)); assertThat(metrics.getTimeStamp(), is(1334134700L)); } @@ -94,4 +95,13 @@ public class MetricsFetcherTest { metrics = fetch(jsonData); assertThat("Wrong number of metrics", metrics.size(), is(0)); } + + public Metric getMetric(String metric, Metrics metrics) { + for (Metric m: metrics.list()) { + if (m.getName().equals(toMetricId(metric))) + return m; + } + return null; + } + } diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/SystemPollerTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/SystemPollerTest.java index 2db8d6b543b..f5ae0048275 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/SystemPollerTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/SystemPollerTest.java @@ -142,7 +142,7 @@ public class SystemPollerTest { SystemPoller.JiffiesAndCpus diff = next.diff(prev); Metrics m = s1.getSystemMetrics(); - List metricList = m.getMetrics(); + List metricList = m.list(); assertEquals(4, metricList.size()); assertEquals(new Metric(MetricId.toMetricId("memory_virt"), 0L, 1), metricList.get(0)); assertEquals(new Metric(MetricId.toMetricId("memory_rss"), 0L, 1), metricList.get(1)); diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/VespaServiceTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/VespaServiceTest.java index 54004e7872a..a54e050bac7 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/VespaServiceTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/service/VespaServiceTest.java @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package ai.vespa.metricsproxy.service; +import ai.vespa.metricsproxy.metric.Metric; import ai.vespa.metricsproxy.metric.Metrics; import ai.vespa.metricsproxy.metric.model.MetricId; import org.junit.After; @@ -8,9 +9,12 @@ import org.junit.Before; import org.junit.Test; import static ai.vespa.metricsproxy.TestUtil.getFileContents; +import static ai.vespa.metricsproxy.metric.model.MetricId.toMetricId; import static ai.vespa.metricsproxy.service.RemoteMetricsFetcher.METRICS_PATH; import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; /** * @author Unknown @@ -53,15 +57,13 @@ public class VespaServiceTest { // TODO: Make it possible to test this without running a HTTP server to create the response public void testMetricsFetching() { VespaService service = VespaService.create("service1", "id", httpServer.port()); - Metrics metrics = service.getMetrics(); - assertThat(metrics.getMetric(MetricId.toMetricId("queries.count")).getValue().intValue(), is(28)); + assertEquals(28, getMetric("queries.count", service.getMetrics()).getValue().intValue()); // Shutdown server and check that no metrics are returned (should use empty metrics // when unable to fetch new metrics) shutdown(); - metrics = service.getMetrics(); - assertThat(metrics.size(), is(0)); + assertTrue(service.getMetrics().list().isEmpty()); } @After @@ -69,4 +71,12 @@ public class VespaServiceTest { this.httpServer.close(); } + public Metric getMetric(String metric, Metrics metrics) { + for (Metric m: metrics.list()) { + if (m.getName().equals(toMetricId(metric))) + return m; + } + return null; + } + } -- cgit v1.2.3