From 413393a7287829615ffb0e0208e8c1ae5ff5ef2f Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Wed, 24 Apr 2024 18:58:31 -0700 Subject: [PATCH 1/7] added MetricRecordedImpl and unite tests for MetricInstrumentRegistry --- .../grpc/DoubleCounterMetricInstrument.java | 2 +- .../grpc/DoubleHistogramMetricInstrument.java | 2 +- .../io/grpc/LongCounterMetricInstrument.java | 2 +- .../io/grpc/LongGaugeMetricInstrument.java | 2 +- .../grpc/LongHistogramMetricInstrument.java | 2 +- .../main/java/io/grpc/MetricInstrument.java | 2 +- .../io/grpc/MetricInstrumentRegistry.java | 150 +++++++----- api/src/main/java/io/grpc/MetricSink.java | 2 + .../java/io/grpc/PartialMetricInstrument.java | 6 +- .../io/grpc/MetricInstrumentRegistryTest.java | 229 ++++++++++++++++++ .../MetricInstrumentRegistryAccessor.java | 35 +++ .../io/grpc/internal/MetricRecorderImpl.java | 140 +++++++++++ .../grpc/internal/MetricRecorderImplTest.java | 195 +++++++++++++++ 13 files changed, 706 insertions(+), 63 deletions(-) create mode 100644 api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java create mode 100644 api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java create mode 100644 core/src/main/java/io/grpc/internal/MetricRecorderImpl.java create mode 100644 core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java diff --git a/api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java b/api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java index 52e16476411..3f07d83d58f 100644 --- a/api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java +++ b/api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java @@ -23,7 +23,7 @@ */ @Internal public final class DoubleCounterMetricInstrument extends PartialMetricInstrument { - DoubleCounterMetricInstrument(long index, String name, String description, String unit, + public DoubleCounterMetricInstrument(int index, String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); } diff --git a/api/src/main/java/io/grpc/DoubleHistogramMetricInstrument.java b/api/src/main/java/io/grpc/DoubleHistogramMetricInstrument.java index e9472e29faa..9039a8c62c1 100644 --- a/api/src/main/java/io/grpc/DoubleHistogramMetricInstrument.java +++ b/api/src/main/java/io/grpc/DoubleHistogramMetricInstrument.java @@ -25,7 +25,7 @@ public final class DoubleHistogramMetricInstrument extends PartialMetricInstrument { private final List bucketBoundaries; - DoubleHistogramMetricInstrument(long index, String name, String description, String unit, + public DoubleHistogramMetricInstrument(int index, String name, String description, String unit, List bucketBoundaries, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); diff --git a/api/src/main/java/io/grpc/LongCounterMetricInstrument.java b/api/src/main/java/io/grpc/LongCounterMetricInstrument.java index 233fb757ba4..73516dfb9e4 100644 --- a/api/src/main/java/io/grpc/LongCounterMetricInstrument.java +++ b/api/src/main/java/io/grpc/LongCounterMetricInstrument.java @@ -23,7 +23,7 @@ */ @Internal public final class LongCounterMetricInstrument extends PartialMetricInstrument { - LongCounterMetricInstrument(long index, String name, String description, String unit, + public LongCounterMetricInstrument(int index, String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); } diff --git a/api/src/main/java/io/grpc/LongGaugeMetricInstrument.java b/api/src/main/java/io/grpc/LongGaugeMetricInstrument.java index 1a98c490606..8e24dd715ef 100644 --- a/api/src/main/java/io/grpc/LongGaugeMetricInstrument.java +++ b/api/src/main/java/io/grpc/LongGaugeMetricInstrument.java @@ -23,7 +23,7 @@ */ @Internal public final class LongGaugeMetricInstrument extends PartialMetricInstrument { - LongGaugeMetricInstrument(long index, String name, String description, String unit, + public LongGaugeMetricInstrument(int index, String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); } diff --git a/api/src/main/java/io/grpc/LongHistogramMetricInstrument.java b/api/src/main/java/io/grpc/LongHistogramMetricInstrument.java index 919e733c85a..2a4e56ffd5a 100644 --- a/api/src/main/java/io/grpc/LongHistogramMetricInstrument.java +++ b/api/src/main/java/io/grpc/LongHistogramMetricInstrument.java @@ -25,7 +25,7 @@ public final class LongHistogramMetricInstrument extends PartialMetricInstrument { private final List bucketBoundaries; - LongHistogramMetricInstrument(long index, String name, String description, String unit, + public LongHistogramMetricInstrument(int index, String name, String description, String unit, List bucketBoundaries, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); diff --git a/api/src/main/java/io/grpc/MetricInstrument.java b/api/src/main/java/io/grpc/MetricInstrument.java index f4353d651f3..1930319060d 100644 --- a/api/src/main/java/io/grpc/MetricInstrument.java +++ b/api/src/main/java/io/grpc/MetricInstrument.java @@ -28,7 +28,7 @@ public interface MetricInstrument { * * @return the index of the metric instrument. */ - public long getIndex(); + public int getIndex(); /** * Returns the name of the metric. diff --git a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java index 0eda6f2d4d0..021a53dfa2e 100644 --- a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java +++ b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java @@ -16,24 +16,36 @@ package io.grpc; -import java.util.Collections; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; +import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Set; -import java.util.concurrent.CopyOnWriteArrayList; -import java.util.concurrent.CopyOnWriteArraySet; +import java.util.concurrent.atomic.AtomicInteger; /** * A registry for globally registered metric instruments. */ @Internal public final class MetricInstrumentRegistry { + private static final int DEFAULT_INSTRUMENT_LIST_CAPACITY = 20; private static MetricInstrumentRegistry instance; - private final List metricInstruments; + private final Object lock = new Object(); private final Set registeredMetricNames; + private final AtomicInteger instrumentIndexAlloc = new AtomicInteger(); + private volatile List metricInstruments; + private volatile int instrumentListCapacity = DEFAULT_INSTRUMENT_LIST_CAPACITY; private MetricInstrumentRegistry() { - this.metricInstruments = new CopyOnWriteArrayList<>(); - this.registeredMetricNames = new CopyOnWriteArraySet<>(); + this(new ArrayList<>(DEFAULT_INSTRUMENT_LIST_CAPACITY), new HashSet<>()); + } + + @VisibleForTesting + MetricInstrumentRegistry(List metricInstruments, + Set registeredMetricNames) { + this.metricInstruments = metricInstruments; + this.registeredMetricNames = registeredMetricNames; } /** @@ -50,7 +62,7 @@ public static synchronized MetricInstrumentRegistry getDefaultRegistry() { * Returns a list of registered metric instruments. */ public List getMetricInstruments() { - return Collections.unmodifiableList(metricInstruments); + return ImmutableList.copyOf(metricInstruments); } /** @@ -65,20 +77,25 @@ public List getMetricInstruments() { * @return the newly created DoubleCounterMetricInstrument * @throws IllegalStateException if a metric with the same name already exists */ - // TODO(dnvindhya): Evaluate locks over synchronized methods and update if needed - public synchronized DoubleCounterMetricInstrument registerDoubleCounter(String name, + public DoubleCounterMetricInstrument registerDoubleCounter(String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { - if (registeredMetricNames.contains(name)) { - throw new IllegalStateException("Metric with name " + name + " already exists"); + // Null check for arguments + synchronized (lock) { + if (registeredMetricNames.contains(name)) { + throw new IllegalStateException("Metric with name " + name + " already exists"); + } + int index = instrumentIndexAlloc.getAndIncrement(); + if (index + 1 == instrumentListCapacity) { + resizeMetricInstruments(); + } + DoubleCounterMetricInstrument instrument = new DoubleCounterMetricInstrument( + index, name, description, unit, requiredLabelKeys, optionalLabelKeys, + enableByDefault); + metricInstruments.add(index, instrument); + registeredMetricNames.add(name); + return instrument; } - long instrumentIndex = metricInstruments.size(); - DoubleCounterMetricInstrument instrument = new DoubleCounterMetricInstrument( - instrumentIndex, name, description, unit, requiredLabelKeys, optionalLabelKeys, - enableByDefault); - metricInstruments.add(instrument); - registeredMetricNames.add(name); - return instrument; } /** @@ -93,20 +110,24 @@ public synchronized DoubleCounterMetricInstrument registerDoubleCounter(String n * @return the newly created LongCounterMetricInstrument * @throws IllegalStateException if a metric with the same name already exists */ - public synchronized LongCounterMetricInstrument registerLongCounter(String name, + public LongCounterMetricInstrument registerLongCounter(String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { - if (registeredMetricNames.contains(name)) { - throw new IllegalStateException("Metric with name " + name + " already exists"); + synchronized (lock) { + if (registeredMetricNames.contains(name)) { + throw new IllegalStateException("Metric with name " + name + " already exists"); + } + int index = instrumentIndexAlloc.getAndIncrement(); + if (index + 1 == instrumentListCapacity) { + resizeMetricInstruments(); + } + LongCounterMetricInstrument instrument = new LongCounterMetricInstrument( + index, name, description, unit, requiredLabelKeys, optionalLabelKeys, + enableByDefault); + metricInstruments.add(instrument); + registeredMetricNames.add(name); + return instrument; } - // Acquire lock? - long instrumentIndex = metricInstruments.size(); - LongCounterMetricInstrument instrument = new LongCounterMetricInstrument( - instrumentIndex, name, description, unit, requiredLabelKeys, optionalLabelKeys, - enableByDefault); - metricInstruments.add(instrument); - registeredMetricNames.add(name); - return instrument; } /** @@ -122,20 +143,25 @@ public synchronized LongCounterMetricInstrument registerLongCounter(String name, * @return the newly created DoubleHistogramMetricInstrument * @throws IllegalStateException if a metric with the same name already exists */ - public synchronized DoubleHistogramMetricInstrument registerDoubleHistogram(String name, + public DoubleHistogramMetricInstrument registerDoubleHistogram(String name, String description, String unit, List bucketBoundaries, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { - if (registeredMetricNames.contains(name)) { - throw new IllegalStateException("Metric with name " + name + " already exists"); + synchronized (lock) { + if (registeredMetricNames.contains(name)) { + throw new IllegalStateException("Metric with name " + name + " already exists"); + } + int index = instrumentIndexAlloc.getAndIncrement(); + if (index + 1 == instrumentListCapacity) { + resizeMetricInstruments(); + } + DoubleHistogramMetricInstrument instrument = new DoubleHistogramMetricInstrument( + index, name, description, unit, bucketBoundaries, requiredLabelKeys, + optionalLabelKeys, + enableByDefault); + metricInstruments.add(instrument); + registeredMetricNames.add(name); + return instrument; } - long indexToInsertInstrument = metricInstruments.size(); - DoubleHistogramMetricInstrument instrument = new DoubleHistogramMetricInstrument( - indexToInsertInstrument, name, description, unit, bucketBoundaries, requiredLabelKeys, - optionalLabelKeys, - enableByDefault); - metricInstruments.add(instrument); - registeredMetricNames.add(name); - return instrument; } /** @@ -151,20 +177,25 @@ public synchronized DoubleHistogramMetricInstrument registerDoubleHistogram(Stri * @return the newly created LongHistogramMetricInstrument * @throws IllegalStateException if a metric with the same name already exists */ - public synchronized LongHistogramMetricInstrument registerLongHistogram(String name, + public LongHistogramMetricInstrument registerLongHistogram(String name, String description, String unit, List bucketBoundaries, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { - if (registeredMetricNames.contains(name)) { - throw new IllegalStateException("Metric with name " + name + " already exists"); + synchronized (lock) { + if (registeredMetricNames.contains(name)) { + throw new IllegalStateException("Metric with name " + name + " already exists"); + } + int index = instrumentIndexAlloc.getAndIncrement(); + if (index + 1 == instrumentListCapacity) { + resizeMetricInstruments(); + } + LongHistogramMetricInstrument instrument = new LongHistogramMetricInstrument( + index, name, description, unit, bucketBoundaries, requiredLabelKeys, + optionalLabelKeys, + enableByDefault); + metricInstruments.add(instrument); + registeredMetricNames.add(name); + return instrument; } - long indexToInsertInstrument = metricInstruments.size(); - LongHistogramMetricInstrument instrument = new LongHistogramMetricInstrument( - indexToInsertInstrument, name, description, unit, bucketBoundaries, requiredLabelKeys, - optionalLabelKeys, - enableByDefault); - metricInstruments.add(instrument); - registeredMetricNames.add(name); - return instrument; } @@ -180,18 +211,29 @@ public synchronized LongHistogramMetricInstrument registerLongHistogram(String n * @return the newly created LongGaugeMetricInstrument * @throws IllegalStateException if a metric with the same name already exists */ - public synchronized LongGaugeMetricInstrument registerLongGauge(String name, String description, + public LongGaugeMetricInstrument registerLongGauge(String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); } - long indexToInsertInstrument = metricInstruments.size(); + int index = instrumentIndexAlloc.getAndIncrement(); + if (index + 1 == metricInstruments.size()) { + resizeMetricInstruments(); + } LongGaugeMetricInstrument instrument = new LongGaugeMetricInstrument( - indexToInsertInstrument, name, description, unit, requiredLabelKeys, optionalLabelKeys, + index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); metricInstruments.add(instrument); registeredMetricNames.add(name); return instrument; } + + private synchronized void resizeMetricInstruments() { + // Resize by factor of DEFAULT_INSTRUMENT_LIST_CAPACITY + instrumentListCapacity = metricInstruments.size() + DEFAULT_INSTRUMENT_LIST_CAPACITY + 1; + List newList = new ArrayList<>(instrumentListCapacity); + newList.addAll(metricInstruments); + metricInstruments = newList; + } } diff --git a/api/src/main/java/io/grpc/MetricSink.java b/api/src/main/java/io/grpc/MetricSink.java index c6d8aaa644c..c4b7b192596 100644 --- a/api/src/main/java/io/grpc/MetricSink.java +++ b/api/src/main/java/io/grpc/MetricSink.java @@ -98,4 +98,6 @@ default void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrum default void recordLongHistogram(LongHistogramMetricInstrument metricInstrument, long value, List requiredLabelValues, List optionalLabelValues) { } + + default void updateMeasures(List instruments) {} } diff --git a/api/src/main/java/io/grpc/PartialMetricInstrument.java b/api/src/main/java/io/grpc/PartialMetricInstrument.java index 5cfecf3499c..c246b67f810 100644 --- a/api/src/main/java/io/grpc/PartialMetricInstrument.java +++ b/api/src/main/java/io/grpc/PartialMetricInstrument.java @@ -25,7 +25,7 @@ */ @Internal abstract class PartialMetricInstrument implements MetricInstrument { - protected final long index; + protected final int index; protected final String name; protected final String description; protected final String unit; @@ -44,7 +44,7 @@ abstract class PartialMetricInstrument implements MetricInstrument { * @param optionalLabelKeys a list of optional label keys for the metric * @param enableByDefault whether the metric should be enabled by default */ - protected PartialMetricInstrument(long index, String name, String description, String unit, + protected PartialMetricInstrument(int index, String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { this.index = index; this.name = name; @@ -56,7 +56,7 @@ protected PartialMetricInstrument(long index, String name, String description, S } @Override - public long getIndex() { + public int getIndex() { return index; } diff --git a/api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java b/api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java new file mode 100644 index 00000000000..ac908dc58b1 --- /dev/null +++ b/api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java @@ -0,0 +1,229 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed 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 + * + * http://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 io.grpc; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Unit test for {@link MetricInstrumentRegistry}. + */ +@RunWith(JUnit4.class) +public class MetricInstrumentRegistryTest { + private static final int TEST_INSTRUMENT_CAPACITY = 20; + private static final ImmutableList REQUIRED_LABEL_KEYS = ImmutableList.of("KEY1", "KEY2"); + private static final ImmutableList OPTIONAL_LABEL_KEYS = ImmutableList.of( + "OPTIONAL_KEY_1"); + private static final ImmutableList DOUBLE_HISTOGRAM_BUCKETS = ImmutableList.of(0.01, 0.1); + private static final ImmutableList LONG_HISTOGRAM_BUCKETS = ImmutableList.of(1L, 10L); + private static final String METRIC_NAME_1 = "testMetric1"; + private static final String DESCRIPTION_1 = "description1"; + private static final String DESCRIPTION_2 = "description2"; + private static final String UNIT_1 = "unit1"; + private static final String UNIT_2 = "unit2"; + private static final boolean ENABLED = true; + private static final boolean DISABLED = true; + private MetricInstrumentRegistry registry; + + @Test + public void registerDoubleCounterSuccess() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + DoubleCounterMetricInstrument instrument = registry.registerDoubleCounter( + METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); + assertThat(registry.getMetricInstruments().contains(instrument)).isTrue(); + assertThat(registry.getMetricInstruments().size()).isEqualTo(1); + assertThat(instrument.getName()).isEqualTo(METRIC_NAME_1); + assertThat(instrument.getDescription()).isEqualTo(DESCRIPTION_1); + assertThat(instrument.getUnit()).isEqualTo(UNIT_1); + assertThat(instrument.getRequiredLabelKeys()).isEqualTo(REQUIRED_LABEL_KEYS); + assertThat(instrument.getOptionalLabelKeys()).isEqualTo(OPTIONAL_LABEL_KEYS); + assertThat(instrument.isEnableByDefault()).isTrue(); + } + + @Test + public void registerLongCounterSuccess() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + LongCounterMetricInstrument instrument2 = registry.registerLongCounter( + METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); + assertThat(registry.getMetricInstruments().contains(instrument2)).isTrue(); + assertThat(registry.getMetricInstruments().size()).isEqualTo(1); + assertThat(instrument2.getName()).isEqualTo(METRIC_NAME_1); + assertThat(instrument2.getDescription()).isEqualTo(DESCRIPTION_1); + assertThat(instrument2.getUnit()).isEqualTo(UNIT_1); + assertThat(instrument2.getRequiredLabelKeys()).isEqualTo(REQUIRED_LABEL_KEYS); + assertThat(instrument2.getOptionalLabelKeys()).isEqualTo(OPTIONAL_LABEL_KEYS); + assertThat(instrument2.isEnableByDefault()).isTrue(); + } + + @Test + public void registerDoubleHistogramSuccess() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + DoubleHistogramMetricInstrument instrument3 = registry.registerDoubleHistogram( + METRIC_NAME_1, DESCRIPTION_1, UNIT_1, DOUBLE_HISTOGRAM_BUCKETS, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, ENABLED); + assertThat(registry.getMetricInstruments().contains(instrument3)).isTrue(); + assertThat(registry.getMetricInstruments().size()).isEqualTo(1); + assertThat(instrument3.getName()).isEqualTo(METRIC_NAME_1); + assertThat(instrument3.getDescription()).isEqualTo(DESCRIPTION_1); + assertThat(instrument3.getUnit()).isEqualTo(UNIT_1); + assertThat(instrument3.getBucketBoundaries()).isEqualTo(DOUBLE_HISTOGRAM_BUCKETS); + assertThat(instrument3.getRequiredLabelKeys()).isEqualTo(REQUIRED_LABEL_KEYS); + assertThat(instrument3.getOptionalLabelKeys()).isEqualTo(OPTIONAL_LABEL_KEYS); + assertThat(instrument3.isEnableByDefault()).isTrue(); + } + + @Test + public void registerLongHistogramSuccess() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + LongHistogramMetricInstrument instrument4 = registry.registerLongHistogram( + METRIC_NAME_1, DESCRIPTION_1, UNIT_1, LONG_HISTOGRAM_BUCKETS, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, ENABLED); + assertThat(registry.getMetricInstruments().contains(instrument4)).isTrue(); + assertThat(registry.getMetricInstruments().size()).isEqualTo(1); + assertThat(instrument4.getName()).isEqualTo(METRIC_NAME_1); + assertThat(instrument4.getDescription()).isEqualTo(DESCRIPTION_1); + assertThat(instrument4.getUnit()).isEqualTo(UNIT_1); + assertThat(instrument4.getBucketBoundaries()).isEqualTo(LONG_HISTOGRAM_BUCKETS); + assertThat(instrument4.getRequiredLabelKeys()).isEqualTo(REQUIRED_LABEL_KEYS); + assertThat(instrument4.getOptionalLabelKeys()).isEqualTo(OPTIONAL_LABEL_KEYS); + assertThat(instrument4.isEnableByDefault()).isTrue(); + } + + @Test + public void registerLongGaugeSuccess() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + LongGaugeMetricInstrument instrument4 = registry.registerLongGauge( + METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, ENABLED); + assertThat(registry.getMetricInstruments().contains(instrument4)).isTrue(); + assertThat(registry.getMetricInstruments().size()).isEqualTo(1); + assertThat(instrument4.getName()).isEqualTo(METRIC_NAME_1); + assertThat(instrument4.getDescription()).isEqualTo(DESCRIPTION_1); + assertThat(instrument4.getUnit()).isEqualTo(UNIT_1); + assertThat(instrument4.getRequiredLabelKeys()).isEqualTo(REQUIRED_LABEL_KEYS); + assertThat(instrument4.getOptionalLabelKeys()).isEqualTo(OPTIONAL_LABEL_KEYS); + assertThat(instrument4.isEnableByDefault()).isTrue(); + } + + @Test(expected = IllegalStateException.class) + public void registerDoubleCounterDuplicateName() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + registry.registerDoubleCounter(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, ENABLED); + registry.registerDoubleCounter(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, DISABLED); + } + + @Test(expected = IllegalStateException.class) + public void registerLongCounterDuplicateName() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + registry.registerDoubleCounter(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, ENABLED); + registry.registerLongCounter(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, DISABLED); + } + + @Test(expected = IllegalStateException.class) + public void registerDoubleHistogramDuplicateName() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + registry.registerLongHistogram(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, LONG_HISTOGRAM_BUCKETS, + REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); + registry.registerDoubleHistogram(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, DOUBLE_HISTOGRAM_BUCKETS, + REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, DISABLED); + } + + @Test(expected = IllegalStateException.class) + public void registerLongHistogramDuplicateName() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + registry.registerLongCounter(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, ENABLED); + registry.registerLongHistogram(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, LONG_HISTOGRAM_BUCKETS, + REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, DISABLED); + } + + @Test(expected = IllegalStateException.class) + public void registerLongGaugeDuplicateName() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + registry.registerDoubleHistogram(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, DOUBLE_HISTOGRAM_BUCKETS, + REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); + registry.registerLongGauge(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, DISABLED); + } + + @Test + public void getMetricInstrumentsMultipleRegistered() { + registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), + new HashSet<>()); + + DoubleCounterMetricInstrument instrument1 = registry.registerDoubleCounter( + "testMetric1", DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); + LongCounterMetricInstrument instrument2 = registry.registerLongCounter( + "testMetric2", DESCRIPTION_2, UNIT_2, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, DISABLED); + DoubleHistogramMetricInstrument instrument3 = registry.registerDoubleHistogram( + "testMetric3", DESCRIPTION_2, UNIT_2, DOUBLE_HISTOGRAM_BUCKETS, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, DISABLED); + + List instruments = registry.getMetricInstruments(); + assertThat(instruments.size()).isEqualTo(3); + assertThat(instruments.contains(instrument1)).isTrue(); + assertThat(instruments.contains(instrument2)).isTrue(); + assertThat(instruments.contains(instrument3)).isTrue(); + } + + @Test + public void resizeMetricInstrumentsCapacityIncrease() { + int initialCapacity = TEST_INSTRUMENT_CAPACITY; + registry = new MetricInstrumentRegistry(new ArrayList<>(initialCapacity), + new HashSet<>()); + + // Registering enough instruments to trigger resize + for (int i = 0; i < initialCapacity + 1; i++) { + registry.registerLongHistogram("name" + i, "desc", "unit", ImmutableList.of(), + ImmutableList.of(), ImmutableList.of(), true); + } + + assertThat(registry.getMetricInstruments().size()).isGreaterThan(initialCapacity); + } + +} diff --git a/api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java b/api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java new file mode 100644 index 00000000000..5c152bf25d5 --- /dev/null +++ b/api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java @@ -0,0 +1,35 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed 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 + * + * http://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 io.grpc; + +import java.util.List; +import java.util.Set; + +/** + * Accesses test-only methods of {@link MetricInstrumentRegistry}. + */ +public final class MetricInstrumentRegistryAccessor { + + private MetricInstrumentRegistryAccessor() { + } + + public static MetricInstrumentRegistry createMetricInstrumentRegistry( + List instruments, + Set registeredNames) { + return new MetricInstrumentRegistry(instruments, registeredNames); + } +} diff --git a/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java new file mode 100644 index 00000000000..4ae83a65e50 --- /dev/null +++ b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java @@ -0,0 +1,140 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed 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 + * + * http://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 io.grpc.internal; + +import com.google.common.annotations.VisibleForTesting; +import io.grpc.DoubleCounterMetricInstrument; +import io.grpc.DoubleHistogramMetricInstrument; +import io.grpc.LongCounterMetricInstrument; +import io.grpc.LongHistogramMetricInstrument; +import io.grpc.MetricInstrument; +import io.grpc.MetricInstrumentRegistry; +import io.grpc.MetricRecorder; +import io.grpc.MetricSink; +import java.util.List; + +/** + * Provides a central point for gRPC components to record metric values. Metrics can be exported to + * monitoring systems by configuring one or more {@link MetricSink}s. + * + *

This class encapsulates the interaction with metric sinks, including updating them with + * the latest set of {@link MetricInstrument}s provided by the {@link MetricInstrumentRegistry}. + */ +final class MetricRecorderImpl implements MetricRecorder { + + private final List metricSinks; + private volatile MetricInstrumentRegistry registry; + + @VisibleForTesting + MetricRecorderImpl(List metricSinks, MetricInstrumentRegistry registry) { + this.metricSinks = metricSinks; + this.registry = registry; + } + + /** + * Records a double counter value. + * + * @param metricInstrument the {@link DoubleCounterMetricInstrument} to record. + * @param value the value to record. + * @param requiredLabelValues the required label values for the metric. + * @param optionalLabelValues the optional label values for the metric. + */ + @Override + public void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value, + List requiredLabelValues, List optionalLabelValues) { + for (MetricSink sink : metricSinks) { + List measures = sink.getMetricsMeasures(); + if (measures.size() <= metricInstrument.getIndex()) { + // Measures may need updating in two cases: + // 1. When the sink is initially created with an empty list of measures. + // 2. When new metric instruments are registered, requiring the sink to accommodate them. + sink.updateMeasures(registry.getMetricInstruments()); + } + sink.recordDoubleCounter(metricInstrument, value, requiredLabelValues, optionalLabelValues); + } + } + + + /** + * Records a long counter value. + * + * @param metricInstrument the {@link LongCounterMetricInstrument} to record. + * @param value the value to record. + * @param requiredLabelValues the required label values for the metric. + * @param optionalLabelValues the optional label values for the metric. + */ + @Override + public void recordLongCounter(LongCounterMetricInstrument metricInstrument, long value, + List requiredLabelValues, List optionalLabelValues) { + for (MetricSink sink : metricSinks) { + List measures = sink.getMetricsMeasures(); + if (measures.size() <= metricInstrument.getIndex()) { + // Measures may need updating in two cases: + // 1. When the sink is initially created with an empty list of measures. + // 2. When new metric instruments are registered, requiring the sink to accommodate them. + sink.updateMeasures(registry.getMetricInstruments()); + } + sink.recordLongCounter(metricInstrument, value, requiredLabelValues, optionalLabelValues); + } + } + + /** + * Records a double histogram value. + * + * @param metricInstrument the {@link DoubleHistogramMetricInstrument} to record. + * @param value the value to record. + * @param requiredLabelValues the required label values for the metric. + * @param optionalLabelValues the optional label values for the metric. + */ + @Override + public void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrument, double value, + List requiredLabelValues, List optionalLabelValues) { + for (MetricSink sink : metricSinks) { + List measures = sink.getMetricsMeasures(); + if (measures.size() <= metricInstrument.getIndex()) { + // Measures may need updating in two cases: + // 1. When the sink is initially created with an empty list of measures. + // 2. When new metric instruments are registered, requiring the sink to accommodate them. + sink.updateMeasures(registry.getMetricInstruments()); + } + sink.recordDoubleHistogram(metricInstrument, value, requiredLabelValues, optionalLabelValues); + } + } + + /** + * Records a long histogram value. + * + * @param metricInstrument the {@link LongHistogramMetricInstrument} to record. + * @param value the value to record. + * @param requiredLabelValues the required label values for the metric. + * @param optionalLabelValues the optional label values for the metric. + */ + @Override + public void recordLongHistogram(LongHistogramMetricInstrument metricInstrument, long value, + List requiredLabelValues, List optionalLabelValues) { + for (MetricSink sink : metricSinks) { + List measures = sink.getMetricsMeasures(); + if (measures.size() <= metricInstrument.getIndex()) { + // Measures may need updating in two cases: + // 1. When the sink is initially created with an empty list of measures. + // 2. When new metric instruments are registered, requiring the sink to accommodate them. + sink.updateMeasures(registry.getMetricInstruments()); + } + sink.recordLongHistogram(metricInstrument, value, requiredLabelValues, optionalLabelValues); + } + } +} diff --git a/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java b/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java new file mode 100644 index 00000000000..56944f20de8 --- /dev/null +++ b/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java @@ -0,0 +1,195 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed 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 + * + * http://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 io.grpc.internal; + +import static com.google.common.truth.Truth.assertThat; +import static org.hamcrest.CoreMatchers.hasItems; +import static org.mockito.ArgumentMatchers.anyList; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.google.common.collect.ImmutableList; +import io.grpc.DoubleCounterMetricInstrument; +import io.grpc.DoubleHistogramMetricInstrument; +import io.grpc.LongCounterMetricInstrument; +import io.grpc.LongHistogramMetricInstrument; +import io.grpc.MetricInstrument; +import io.grpc.MetricInstrumentRegistry; +import io.grpc.MetricInstrumentRegistryAccessor; +import io.grpc.MetricRecorder; +import io.grpc.MetricSink; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mockito.ArgumentCaptor; + +/** + * Unit test for {@link MetricRecorderImpl}. + */ +@RunWith(JUnit4.class) +public class MetricRecorderImplTest { + + private static final String DESCRIPTION = "description"; + private static final String UNIT = "unit"; + private static final boolean ENABLED = true; + private static final ImmutableList REQUIRED_LABEL_KEYS = ImmutableList.of("KEY1", "KEY2"); + private static final ImmutableList OPTIONAL_LABEL_KEYS = ImmutableList.of( + "OPTIONAL_KEY_1"); + private static final ImmutableList REQUIRED_LABEL_VALUES = ImmutableList.of("VALUE1", + "VALUE2"); + private static final ImmutableList OPTIONAL_LABEL_VALUES = ImmutableList.of( + "OPTIONAL_VALUE_1"); + private static final DoubleCounterMetricInstrument DOUBLE_COUNTER_INSTRUMENT = + new DoubleCounterMetricInstrument(0, "counter0", DESCRIPTION, UNIT, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, ENABLED); + private static final LongCounterMetricInstrument LONG_COUNTER_INSTRUMENT = + new LongCounterMetricInstrument(1, "counter1", DESCRIPTION, UNIT, REQUIRED_LABEL_KEYS, + OPTIONAL_LABEL_KEYS, ENABLED); + private static final DoubleHistogramMetricInstrument DOUBLE_HISTOGRAM_INSTRUMENT = + new DoubleHistogramMetricInstrument(2, "histogram1", DESCRIPTION, UNIT, + Collections.emptyList(), REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); + private static final LongHistogramMetricInstrument LONG_HISTOGRAM_INSTRUMENT = + new LongHistogramMetricInstrument(3, "histogram2", DESCRIPTION, UNIT, + Collections.emptyList(), REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); + private final List instruments = Arrays.asList(DOUBLE_COUNTER_INSTRUMENT, + LONG_COUNTER_INSTRUMENT, DOUBLE_HISTOGRAM_INSTRUMENT, LONG_HISTOGRAM_INSTRUMENT); + private final Set metricNames = Stream.of("counter1", "counter2", "histogram1", + "histogram2") + .collect(Collectors.toCollection(HashSet::new)); + private MetricSink mockSink = mock(MetricSink.class); + private List sinks = Arrays.asList(mockSink, mockSink); + private MetricInstrumentRegistry registry; + private MetricRecorder recorder; + private ArgumentCaptor doubleCounterInstrumentCaptor = + ArgumentCaptor.forClass(DoubleCounterMetricInstrument.class); + private ArgumentCaptor longCounterInstrumentCaptor = + ArgumentCaptor.forClass(LongCounterMetricInstrument.class); + private ArgumentCaptor doubleHistogramInstrumentCaptor = + ArgumentCaptor.forClass(DoubleHistogramMetricInstrument.class); + private ArgumentCaptor longHistogramInstrumentCaptor = + ArgumentCaptor.forClass(LongHistogramMetricInstrument.class); + + + @Before + public void setUp() { + registry = MetricInstrumentRegistryAccessor.createMetricInstrumentRegistry(instruments, + metricNames); + recorder = new MetricRecorderImpl(sinks, registry); + } + + @Test + public void recordCounter() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.recordDoubleCounter(DOUBLE_COUNTER_INSTRUMENT, 1.0, REQUIRED_LABEL_VALUES, + OPTIONAL_LABEL_VALUES); + verify(mockSink, times(2)).recordDoubleCounter(doubleCounterInstrumentCaptor.capture(), eq(1D), + argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), + argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); + assertThat(doubleCounterInstrumentCaptor.getValue()).isEqualTo(DOUBLE_COUNTER_INSTRUMENT); + + recorder.recordLongCounter(LONG_COUNTER_INSTRUMENT, 1, REQUIRED_LABEL_VALUES, + OPTIONAL_LABEL_VALUES); + verify(mockSink, times(2)).recordLongCounter(longCounterInstrumentCaptor.capture(), eq(1L), + argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), + argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); + assertThat(longCounterInstrumentCaptor.getValue()).isEqualTo(LONG_COUNTER_INSTRUMENT); + + verify(mockSink, never()).updateMeasures(registry.getMetricInstruments()); + } + + @Test + public void recordHistogram() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.recordDoubleHistogram(DOUBLE_HISTOGRAM_INSTRUMENT, 99.0, REQUIRED_LABEL_VALUES, + OPTIONAL_LABEL_VALUES); + verify(mockSink, times(2)).recordDoubleHistogram(doubleHistogramInstrumentCaptor.capture(), + eq(99D), + argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), + argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); + assertThat(doubleHistogramInstrumentCaptor.getValue()).isEqualTo(DOUBLE_HISTOGRAM_INSTRUMENT); + + recorder.recordLongHistogram(LONG_HISTOGRAM_INSTRUMENT, 99, REQUIRED_LABEL_VALUES, + OPTIONAL_LABEL_VALUES); + verify(mockSink, times(2)).recordLongHistogram(longHistogramInstrumentCaptor.capture(), eq(99L), + argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), + argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); + assertThat(longHistogramInstrumentCaptor.getValue()).isEqualTo(LONG_HISTOGRAM_INSTRUMENT); + + verify(mockSink, never()).updateMeasures(registry.getMetricInstruments()); + } + + @Test + public void newRegisteredMetricUpdateMeasures() { + // Sink is initialized with zero measures, should trigger updateMeasures() on sinks + when(mockSink.getMetricsMeasures()).thenReturn(new ArrayList<>()); + + // Double Counter + recorder.recordDoubleCounter(DOUBLE_COUNTER_INSTRUMENT, 1.0, REQUIRED_LABEL_VALUES, + OPTIONAL_LABEL_VALUES); + verify(mockSink, times(2)).updateMeasures(anyList()); + verify(mockSink, times(2)).recordDoubleCounter(doubleCounterInstrumentCaptor.capture(), eq(1D), + argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), + argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); + assertThat(doubleCounterInstrumentCaptor.getValue()).isEqualTo(DOUBLE_COUNTER_INSTRUMENT); + + // Long Counter + recorder.recordLongCounter(LONG_COUNTER_INSTRUMENT, 1, REQUIRED_LABEL_VALUES, + OPTIONAL_LABEL_VALUES); + verify(mockSink, times(4)).updateMeasures(anyList()); + verify(mockSink, times(2)).recordLongCounter(longCounterInstrumentCaptor.capture(), eq(1L), + argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), + argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); + assertThat(longCounterInstrumentCaptor.getValue()).isEqualTo(LONG_COUNTER_INSTRUMENT); + + // Double Histogram + recorder.recordDoubleHistogram(DOUBLE_HISTOGRAM_INSTRUMENT, 99.0, REQUIRED_LABEL_VALUES, + OPTIONAL_LABEL_VALUES); + verify(mockSink, times(6)).updateMeasures(anyList()); + verify(mockSink, times(2)).recordDoubleHistogram(doubleHistogramInstrumentCaptor.capture(), + eq(99D), + argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), + argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); + assertThat(doubleHistogramInstrumentCaptor.getValue()).isEqualTo(DOUBLE_HISTOGRAM_INSTRUMENT); + + // Long Histogram + recorder.recordLongHistogram(LONG_HISTOGRAM_INSTRUMENT, 99, REQUIRED_LABEL_VALUES, + OPTIONAL_LABEL_VALUES); + verify(mockSink, times(8)).updateMeasures(registry.getMetricInstruments()); + verify(mockSink, times(2)).recordLongHistogram(longHistogramInstrumentCaptor.capture(), eq(99L), + argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), + argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); + assertThat(longHistogramInstrumentCaptor.getValue()).isEqualTo(LONG_HISTOGRAM_INSTRUMENT); + } +} From 295e58ded84fad6a8c4a860d6bfe490735ed15ff Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Wed, 24 Apr 2024 19:03:57 -0700 Subject: [PATCH 2/7] changed MetricRcorderImpl to public --- core/src/main/java/io/grpc/internal/MetricRecorderImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java index 4ae83a65e50..31348792a56 100644 --- a/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java +++ b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java @@ -34,7 +34,7 @@ *

This class encapsulates the interaction with metric sinks, including updating them with * the latest set of {@link MetricInstrument}s provided by the {@link MetricInstrumentRegistry}. */ -final class MetricRecorderImpl implements MetricRecorder { +public final class MetricRecorderImpl implements MetricRecorder { private final List metricSinks; private volatile MetricInstrumentRegistry registry; From 0f95f8e120f36a449c7408497982a7219cc5de3a Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Thu, 25 Apr 2024 15:08:32 -0700 Subject: [PATCH 3/7] updated MetricInstrumenetRegistry to use array instead of ArrayList; addressed review comments --- .../io/grpc/MetricInstrumentRegistry.java | 88 +++++++------- .../io/grpc/MetricInstrumentRegistryTest.java | 50 ++------ .../MetricInstrumentRegistryAccessor.java | 9 +- .../io/grpc/internal/MetricRecorderImpl.java | 6 +- .../grpc/internal/MetricRecorderImplTest.java | 115 ++++++------------ 5 files changed, 95 insertions(+), 173 deletions(-) diff --git a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java index 021a53dfa2e..3bf5f0b4734 100644 --- a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java +++ b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java @@ -17,35 +17,31 @@ package io.grpc; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableList; -import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; +import javax.annotation.concurrent.GuardedBy; /** * A registry for globally registered metric instruments. */ @Internal public final class MetricInstrumentRegistry { - private static final int DEFAULT_INSTRUMENT_LIST_CAPACITY = 20; + public static final int INITIAL_INSTRUMENT_CAPACITY = 5; private static MetricInstrumentRegistry instance; private final Object lock = new Object(); private final Set registeredMetricNames; - private final AtomicInteger instrumentIndexAlloc = new AtomicInteger(); - private volatile List metricInstruments; - private volatile int instrumentListCapacity = DEFAULT_INSTRUMENT_LIST_CAPACITY; - - private MetricInstrumentRegistry() { - this(new ArrayList<>(DEFAULT_INSTRUMENT_LIST_CAPACITY), new HashSet<>()); - } + private volatile MetricInstrument[] metricInstruments; + private volatile int instrumentListCapacity = INITIAL_INSTRUMENT_CAPACITY; + @GuardedBy("lock") + private int nextAvailableMetricIndex; @VisibleForTesting - MetricInstrumentRegistry(List metricInstruments, - Set registeredMetricNames) { - this.metricInstruments = metricInstruments; - this.registeredMetricNames = registeredMetricNames; + MetricInstrumentRegistry() { + this.metricInstruments = new MetricInstrument[INITIAL_INSTRUMENT_CAPACITY]; + this.registeredMetricNames = new HashSet<>(); } /** @@ -62,7 +58,10 @@ public static synchronized MetricInstrumentRegistry getDefaultRegistry() { * Returns a list of registered metric instruments. */ public List getMetricInstruments() { - return ImmutableList.copyOf(metricInstruments); + synchronized (lock) { + return Collections.unmodifiableList( + Arrays.asList(Arrays.copyOfRange(metricInstruments, 0, nextAvailableMetricIndex))); + } } /** @@ -85,15 +84,16 @@ public DoubleCounterMetricInstrument registerDoubleCounter(String name, if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); } - int index = instrumentIndexAlloc.getAndIncrement(); + int index = nextAvailableMetricIndex; if (index + 1 == instrumentListCapacity) { resizeMetricInstruments(); } DoubleCounterMetricInstrument instrument = new DoubleCounterMetricInstrument( index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); - metricInstruments.add(index, instrument); + metricInstruments[index] = instrument; registeredMetricNames.add(name); + nextAvailableMetricIndex += 1; return instrument; } } @@ -117,15 +117,16 @@ public LongCounterMetricInstrument registerLongCounter(String name, if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); } - int index = instrumentIndexAlloc.getAndIncrement(); + int index = nextAvailableMetricIndex; if (index + 1 == instrumentListCapacity) { resizeMetricInstruments(); } LongCounterMetricInstrument instrument = new LongCounterMetricInstrument( index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); - metricInstruments.add(instrument); + metricInstruments[index] = instrument; registeredMetricNames.add(name); + nextAvailableMetricIndex += 1; return instrument; } } @@ -150,7 +151,7 @@ public DoubleHistogramMetricInstrument registerDoubleHistogram(String name, if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); } - int index = instrumentIndexAlloc.getAndIncrement(); + int index = nextAvailableMetricIndex; if (index + 1 == instrumentListCapacity) { resizeMetricInstruments(); } @@ -158,8 +159,9 @@ public DoubleHistogramMetricInstrument registerDoubleHistogram(String name, index, name, description, unit, bucketBoundaries, requiredLabelKeys, optionalLabelKeys, enableByDefault); - metricInstruments.add(instrument); + metricInstruments[index] = instrument; registeredMetricNames.add(name); + nextAvailableMetricIndex += 1; return instrument; } } @@ -184,7 +186,7 @@ public LongHistogramMetricInstrument registerLongHistogram(String name, if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); } - int index = instrumentIndexAlloc.getAndIncrement(); + int index = nextAvailableMetricIndex; if (index + 1 == instrumentListCapacity) { resizeMetricInstruments(); } @@ -192,8 +194,9 @@ public LongHistogramMetricInstrument registerLongHistogram(String name, index, name, description, unit, bucketBoundaries, requiredLabelKeys, optionalLabelKeys, enableByDefault); - metricInstruments.add(instrument); + metricInstruments[index] = instrument; registeredMetricNames.add(name); + nextAvailableMetricIndex += 1; return instrument; } } @@ -214,26 +217,29 @@ public LongHistogramMetricInstrument registerLongHistogram(String name, public LongGaugeMetricInstrument registerLongGauge(String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { - if (registeredMetricNames.contains(name)) { - throw new IllegalStateException("Metric with name " + name + " already exists"); - } - int index = instrumentIndexAlloc.getAndIncrement(); - if (index + 1 == metricInstruments.size()) { - resizeMetricInstruments(); + synchronized (lock) { + if (registeredMetricNames.contains(name)) { + throw new IllegalStateException("Metric with name " + name + " already exists"); + } + int index = nextAvailableMetricIndex; + if (index + 1 == instrumentListCapacity) { + resizeMetricInstruments(); + } + LongGaugeMetricInstrument instrument = new LongGaugeMetricInstrument( + index, name, description, unit, requiredLabelKeys, optionalLabelKeys, + enableByDefault); + metricInstruments[index] = instrument; + registeredMetricNames.add(name); + nextAvailableMetricIndex += 1; + return instrument; } - LongGaugeMetricInstrument instrument = new LongGaugeMetricInstrument( - index, name, description, unit, requiredLabelKeys, optionalLabelKeys, - enableByDefault); - metricInstruments.add(instrument); - registeredMetricNames.add(name); - return instrument; } private synchronized void resizeMetricInstruments() { - // Resize by factor of DEFAULT_INSTRUMENT_LIST_CAPACITY - instrumentListCapacity = metricInstruments.size() + DEFAULT_INSTRUMENT_LIST_CAPACITY + 1; - List newList = new ArrayList<>(instrumentListCapacity); - newList.addAll(metricInstruments); - metricInstruments = newList; + // Increase the capacity of the metricInstruments array by INITIAL_INSTRUMENT_CAPACITY + instrumentListCapacity += INITIAL_INSTRUMENT_CAPACITY; + MetricInstrument[] resizedMetricInstruments = Arrays.copyOf(metricInstruments, + instrumentListCapacity); + metricInstruments = resizedMetricInstruments; } } diff --git a/api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java b/api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java index ac908dc58b1..b378f4aaef5 100644 --- a/api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java +++ b/api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java @@ -17,10 +17,9 @@ package io.grpc; import static com.google.common.truth.Truth.assertThat; +import static io.grpc.MetricInstrumentRegistry.INITIAL_INSTRUMENT_CAPACITY; import com.google.common.collect.ImmutableList; -import java.util.ArrayList; -import java.util.HashSet; import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; @@ -31,7 +30,6 @@ */ @RunWith(JUnit4.class) public class MetricInstrumentRegistryTest { - private static final int TEST_INSTRUMENT_CAPACITY = 20; private static final ImmutableList REQUIRED_LABEL_KEYS = ImmutableList.of("KEY1", "KEY2"); private static final ImmutableList OPTIONAL_LABEL_KEYS = ImmutableList.of( "OPTIONAL_KEY_1"); @@ -43,14 +41,11 @@ public class MetricInstrumentRegistryTest { private static final String UNIT_1 = "unit1"; private static final String UNIT_2 = "unit2"; private static final boolean ENABLED = true; - private static final boolean DISABLED = true; - private MetricInstrumentRegistry registry; + private static final boolean DISABLED = false; + private MetricInstrumentRegistry registry = new MetricInstrumentRegistry(); @Test public void registerDoubleCounterSuccess() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - DoubleCounterMetricInstrument instrument = registry.registerDoubleCounter( METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); assertThat(registry.getMetricInstruments().contains(instrument)).isTrue(); @@ -65,9 +60,6 @@ public void registerDoubleCounterSuccess() { @Test public void registerLongCounterSuccess() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - LongCounterMetricInstrument instrument2 = registry.registerLongCounter( METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); assertThat(registry.getMetricInstruments().contains(instrument2)).isTrue(); @@ -82,9 +74,6 @@ public void registerLongCounterSuccess() { @Test public void registerDoubleHistogramSuccess() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - DoubleHistogramMetricInstrument instrument3 = registry.registerDoubleHistogram( METRIC_NAME_1, DESCRIPTION_1, UNIT_1, DOUBLE_HISTOGRAM_BUCKETS, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); @@ -101,9 +90,6 @@ public void registerDoubleHistogramSuccess() { @Test public void registerLongHistogramSuccess() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - LongHistogramMetricInstrument instrument4 = registry.registerLongHistogram( METRIC_NAME_1, DESCRIPTION_1, UNIT_1, LONG_HISTOGRAM_BUCKETS, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); @@ -120,9 +106,6 @@ public void registerLongHistogramSuccess() { @Test public void registerLongGaugeSuccess() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - LongGaugeMetricInstrument instrument4 = registry.registerLongGauge( METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); @@ -138,9 +121,6 @@ public void registerLongGaugeSuccess() { @Test(expected = IllegalStateException.class) public void registerDoubleCounterDuplicateName() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - registry.registerDoubleCounter(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); registry.registerDoubleCounter(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, REQUIRED_LABEL_KEYS, @@ -149,9 +129,6 @@ public void registerDoubleCounterDuplicateName() { @Test(expected = IllegalStateException.class) public void registerLongCounterDuplicateName() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - registry.registerDoubleCounter(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); registry.registerLongCounter(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, REQUIRED_LABEL_KEYS, @@ -160,9 +137,6 @@ public void registerLongCounterDuplicateName() { @Test(expected = IllegalStateException.class) public void registerDoubleHistogramDuplicateName() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - registry.registerLongHistogram(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, LONG_HISTOGRAM_BUCKETS, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); registry.registerDoubleHistogram(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, DOUBLE_HISTOGRAM_BUCKETS, @@ -171,9 +145,6 @@ public void registerDoubleHistogramDuplicateName() { @Test(expected = IllegalStateException.class) public void registerLongHistogramDuplicateName() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - registry.registerLongCounter(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); registry.registerLongHistogram(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, LONG_HISTOGRAM_BUCKETS, @@ -182,9 +153,6 @@ public void registerLongHistogramDuplicateName() { @Test(expected = IllegalStateException.class) public void registerLongGaugeDuplicateName() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - registry.registerDoubleHistogram(METRIC_NAME_1, DESCRIPTION_1, UNIT_1, DOUBLE_HISTOGRAM_BUCKETS, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); registry.registerLongGauge(METRIC_NAME_1, DESCRIPTION_2, UNIT_2, REQUIRED_LABEL_KEYS, @@ -193,9 +161,6 @@ public void registerLongGaugeDuplicateName() { @Test public void getMetricInstrumentsMultipleRegistered() { - registry = new MetricInstrumentRegistry(new ArrayList<>(TEST_INSTRUMENT_CAPACITY), - new HashSet<>()); - DoubleCounterMetricInstrument instrument1 = registry.registerDoubleCounter( "testMetric1", DESCRIPTION_1, UNIT_1, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); LongCounterMetricInstrument instrument2 = registry.registerLongCounter( @@ -213,17 +178,16 @@ public void getMetricInstrumentsMultipleRegistered() { @Test public void resizeMetricInstrumentsCapacityIncrease() { - int initialCapacity = TEST_INSTRUMENT_CAPACITY; - registry = new MetricInstrumentRegistry(new ArrayList<>(initialCapacity), - new HashSet<>()); + int initialCapacity = INITIAL_INSTRUMENT_CAPACITY; + MetricInstrumentRegistry testRegistry = new MetricInstrumentRegistry(); // Registering enough instruments to trigger resize for (int i = 0; i < initialCapacity + 1; i++) { - registry.registerLongHistogram("name" + i, "desc", "unit", ImmutableList.of(), + testRegistry.registerLongHistogram("name" + i, "desc", "unit", ImmutableList.of(), ImmutableList.of(), ImmutableList.of(), true); } - assertThat(registry.getMetricInstruments().size()).isGreaterThan(initialCapacity); + assertThat(testRegistry.getMetricInstruments().size()).isGreaterThan(initialCapacity); } } diff --git a/api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java b/api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java index 5c152bf25d5..bd17dccad58 100644 --- a/api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java +++ b/api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java @@ -16,9 +16,6 @@ package io.grpc; -import java.util.List; -import java.util.Set; - /** * Accesses test-only methods of {@link MetricInstrumentRegistry}. */ @@ -27,9 +24,7 @@ public final class MetricInstrumentRegistryAccessor { private MetricInstrumentRegistryAccessor() { } - public static MetricInstrumentRegistry createMetricInstrumentRegistry( - List instruments, - Set registeredNames) { - return new MetricInstrumentRegistry(instruments, registeredNames); + public static MetricInstrumentRegistry createMetricInstrumentRegistry() { + return new MetricInstrumentRegistry(); } } diff --git a/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java index 31348792a56..d1f721056d4 100644 --- a/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java +++ b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java @@ -34,10 +34,10 @@ *

This class encapsulates the interaction with metric sinks, including updating them with * the latest set of {@link MetricInstrument}s provided by the {@link MetricInstrumentRegistry}. */ -public final class MetricRecorderImpl implements MetricRecorder { +final class MetricRecorderImpl implements MetricRecorder { private final List metricSinks; - private volatile MetricInstrumentRegistry registry; + private final MetricInstrumentRegistry registry; @VisibleForTesting MetricRecorderImpl(List metricSinks, MetricInstrumentRegistry registry) { @@ -57,6 +57,7 @@ public final class MetricRecorderImpl implements MetricRecorder { public void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value, List requiredLabelValues, List optionalLabelValues) { for (MetricSink sink : metricSinks) { + // TODO(dnvindhya): Move updating measures logic from sink to here List measures = sink.getMetricsMeasures(); if (measures.size() <= metricInstrument.getIndex()) { // Measures may need updating in two cases: @@ -68,7 +69,6 @@ public void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument, } } - /** * Records a long counter value. * diff --git a/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java b/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java index 56944f20de8..9f664b61344 100644 --- a/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java +++ b/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java @@ -16,10 +16,7 @@ package io.grpc.internal; -import static com.google.common.truth.Truth.assertThat; -import static org.hamcrest.CoreMatchers.hasItems; import static org.mockito.ArgumentMatchers.anyList; -import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -32,7 +29,6 @@ import io.grpc.DoubleHistogramMetricInstrument; import io.grpc.LongCounterMetricInstrument; import io.grpc.LongHistogramMetricInstrument; -import io.grpc.MetricInstrument; import io.grpc.MetricInstrumentRegistry; import io.grpc.MetricInstrumentRegistryAccessor; import io.grpc.MetricRecorder; @@ -40,23 +36,17 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; -import java.util.stream.Stream; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import org.mockito.ArgumentCaptor; /** * Unit test for {@link MetricRecorderImpl}. */ @RunWith(JUnit4.class) public class MetricRecorderImplTest { - private static final String DESCRIPTION = "description"; private static final String UNIT = "unit"; private static final boolean ENABLED = true; @@ -67,41 +57,26 @@ public class MetricRecorderImplTest { "VALUE2"); private static final ImmutableList OPTIONAL_LABEL_VALUES = ImmutableList.of( "OPTIONAL_VALUE_1"); - private static final DoubleCounterMetricInstrument DOUBLE_COUNTER_INSTRUMENT = - new DoubleCounterMetricInstrument(0, "counter0", DESCRIPTION, UNIT, REQUIRED_LABEL_KEYS, + private MetricSink mockSink = mock(MetricSink.class); + private List sinks = Arrays.asList(mockSink, mockSink); + private MetricInstrumentRegistry registry = + MetricInstrumentRegistryAccessor.createMetricInstrumentRegistry(); + private final DoubleCounterMetricInstrument doubleCounterInstrument = + registry.registerDoubleCounter("counter0", DESCRIPTION, UNIT, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); - private static final LongCounterMetricInstrument LONG_COUNTER_INSTRUMENT = - new LongCounterMetricInstrument(1, "counter1", DESCRIPTION, UNIT, REQUIRED_LABEL_KEYS, + private final LongCounterMetricInstrument longCounterInstrument = + registry.registerLongCounter("counter1", DESCRIPTION, UNIT, REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); - private static final DoubleHistogramMetricInstrument DOUBLE_HISTOGRAM_INSTRUMENT = - new DoubleHistogramMetricInstrument(2, "histogram1", DESCRIPTION, UNIT, + private final DoubleHistogramMetricInstrument doubleHistogramInstrument = + registry.registerDoubleHistogram("histogram1", DESCRIPTION, UNIT, Collections.emptyList(), REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); - private static final LongHistogramMetricInstrument LONG_HISTOGRAM_INSTRUMENT = - new LongHistogramMetricInstrument(3, "histogram2", DESCRIPTION, UNIT, + private final LongHistogramMetricInstrument longHistogramInstrument = + registry.registerLongHistogram("histogram2", DESCRIPTION, UNIT, Collections.emptyList(), REQUIRED_LABEL_KEYS, OPTIONAL_LABEL_KEYS, ENABLED); - private final List instruments = Arrays.asList(DOUBLE_COUNTER_INSTRUMENT, - LONG_COUNTER_INSTRUMENT, DOUBLE_HISTOGRAM_INSTRUMENT, LONG_HISTOGRAM_INSTRUMENT); - private final Set metricNames = Stream.of("counter1", "counter2", "histogram1", - "histogram2") - .collect(Collectors.toCollection(HashSet::new)); - private MetricSink mockSink = mock(MetricSink.class); - private List sinks = Arrays.asList(mockSink, mockSink); - private MetricInstrumentRegistry registry; private MetricRecorder recorder; - private ArgumentCaptor doubleCounterInstrumentCaptor = - ArgumentCaptor.forClass(DoubleCounterMetricInstrument.class); - private ArgumentCaptor longCounterInstrumentCaptor = - ArgumentCaptor.forClass(LongCounterMetricInstrument.class); - private ArgumentCaptor doubleHistogramInstrumentCaptor = - ArgumentCaptor.forClass(DoubleHistogramMetricInstrument.class); - private ArgumentCaptor longHistogramInstrumentCaptor = - ArgumentCaptor.forClass(LongHistogramMetricInstrument.class); - @Before public void setUp() { - registry = MetricInstrumentRegistryAccessor.createMetricInstrumentRegistry(instruments, - metricNames); recorder = new MetricRecorderImpl(sinks, registry); } @@ -110,19 +85,15 @@ public void recordCounter() { when(mockSink.getMetricsMeasures()).thenReturn( Arrays.asList(new Object(), new Object(), new Object(), new Object())); - recorder.recordDoubleCounter(DOUBLE_COUNTER_INSTRUMENT, 1.0, REQUIRED_LABEL_VALUES, + recorder.recordDoubleCounter(doubleCounterInstrument, 1.0, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); - verify(mockSink, times(2)).recordDoubleCounter(doubleCounterInstrumentCaptor.capture(), eq(1D), - argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), - argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); - assertThat(doubleCounterInstrumentCaptor.getValue()).isEqualTo(DOUBLE_COUNTER_INSTRUMENT); + verify(mockSink, times(2)).recordDoubleCounter(eq(doubleCounterInstrument), eq(1D), + eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); - recorder.recordLongCounter(LONG_COUNTER_INSTRUMENT, 1, REQUIRED_LABEL_VALUES, + recorder.recordLongCounter(longCounterInstrument, 1, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); - verify(mockSink, times(2)).recordLongCounter(longCounterInstrumentCaptor.capture(), eq(1L), - argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), - argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); - assertThat(longCounterInstrumentCaptor.getValue()).isEqualTo(LONG_COUNTER_INSTRUMENT); + verify(mockSink, times(2)).recordLongCounter(eq(longCounterInstrument), eq(1L), + eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); verify(mockSink, never()).updateMeasures(registry.getMetricInstruments()); } @@ -132,20 +103,15 @@ public void recordHistogram() { when(mockSink.getMetricsMeasures()).thenReturn( Arrays.asList(new Object(), new Object(), new Object(), new Object())); - recorder.recordDoubleHistogram(DOUBLE_HISTOGRAM_INSTRUMENT, 99.0, REQUIRED_LABEL_VALUES, + recorder.recordDoubleHistogram(doubleHistogramInstrument, 99.0, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); - verify(mockSink, times(2)).recordDoubleHistogram(doubleHistogramInstrumentCaptor.capture(), - eq(99D), - argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), - argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); - assertThat(doubleHistogramInstrumentCaptor.getValue()).isEqualTo(DOUBLE_HISTOGRAM_INSTRUMENT); + verify(mockSink, times(2)).recordDoubleHistogram(eq(doubleHistogramInstrument), + eq(99D), eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); - recorder.recordLongHistogram(LONG_HISTOGRAM_INSTRUMENT, 99, REQUIRED_LABEL_VALUES, + recorder.recordLongHistogram(longHistogramInstrument, 99, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); - verify(mockSink, times(2)).recordLongHistogram(longHistogramInstrumentCaptor.capture(), eq(99L), - argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), - argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); - assertThat(longHistogramInstrumentCaptor.getValue()).isEqualTo(LONG_HISTOGRAM_INSTRUMENT); + verify(mockSink, times(2)).recordLongHistogram(eq(longHistogramInstrument), eq(99L), + eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); verify(mockSink, never()).updateMeasures(registry.getMetricInstruments()); } @@ -156,40 +122,31 @@ public void newRegisteredMetricUpdateMeasures() { when(mockSink.getMetricsMeasures()).thenReturn(new ArrayList<>()); // Double Counter - recorder.recordDoubleCounter(DOUBLE_COUNTER_INSTRUMENT, 1.0, REQUIRED_LABEL_VALUES, + recorder.recordDoubleCounter(doubleCounterInstrument, 1.0, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); verify(mockSink, times(2)).updateMeasures(anyList()); - verify(mockSink, times(2)).recordDoubleCounter(doubleCounterInstrumentCaptor.capture(), eq(1D), - argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), - argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); - assertThat(doubleCounterInstrumentCaptor.getValue()).isEqualTo(DOUBLE_COUNTER_INSTRUMENT); + verify(mockSink, times(2)).recordDoubleCounter(eq(doubleCounterInstrument), eq(1D), + eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); // Long Counter - recorder.recordLongCounter(LONG_COUNTER_INSTRUMENT, 1, REQUIRED_LABEL_VALUES, + recorder.recordLongCounter(longCounterInstrument, 1, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); verify(mockSink, times(4)).updateMeasures(anyList()); - verify(mockSink, times(2)).recordLongCounter(longCounterInstrumentCaptor.capture(), eq(1L), - argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), - argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); - assertThat(longCounterInstrumentCaptor.getValue()).isEqualTo(LONG_COUNTER_INSTRUMENT); + verify(mockSink, times(2)).recordLongCounter(eq(longCounterInstrument), eq(1L), + eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); // Double Histogram - recorder.recordDoubleHistogram(DOUBLE_HISTOGRAM_INSTRUMENT, 99.0, REQUIRED_LABEL_VALUES, + recorder.recordDoubleHistogram(doubleHistogramInstrument, 99.0, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); verify(mockSink, times(6)).updateMeasures(anyList()); - verify(mockSink, times(2)).recordDoubleHistogram(doubleHistogramInstrumentCaptor.capture(), - eq(99D), - argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), - argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); - assertThat(doubleHistogramInstrumentCaptor.getValue()).isEqualTo(DOUBLE_HISTOGRAM_INSTRUMENT); + verify(mockSink, times(2)).recordDoubleHistogram(eq(doubleHistogramInstrument), + eq(99D), eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); // Long Histogram - recorder.recordLongHistogram(LONG_HISTOGRAM_INSTRUMENT, 99, REQUIRED_LABEL_VALUES, + recorder.recordLongHistogram(longHistogramInstrument, 99, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); verify(mockSink, times(8)).updateMeasures(registry.getMetricInstruments()); - verify(mockSink, times(2)).recordLongHistogram(longHistogramInstrumentCaptor.capture(), eq(99L), - argThat(l -> hasItems("VALUE1", "VALUE2").matches(l)), - argThat(l -> hasItems("OPTIONAL_VALUE_1").matches(l))); - assertThat(longHistogramInstrumentCaptor.getValue()).isEqualTo(LONG_HISTOGRAM_INSTRUMENT); + verify(mockSink, times(2)).recordLongHistogram(eq(longHistogramInstrument), eq(99L), + eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); } } From 7167dcbaee99b68a9e3a54205952793d54a60563 Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Thu, 25 Apr 2024 18:05:19 -0700 Subject: [PATCH 4/7] Renamed record<>Counter APIs to add<>Counter. Added check for mismatched label values --- .../io/grpc/MetricInstrumentRegistry.java | 32 +++++++- api/src/main/java/io/grpc/MetricRecorder.java | 21 ++--- .../io/grpc/internal/MetricRecorderImpl.java | 38 ++++++++- .../grpc/internal/MetricRecorderImplTest.java | 80 ++++++++++++++++++- 4 files changed, 151 insertions(+), 20 deletions(-) diff --git a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java index 3bf5f0b4734..681850f7169 100644 --- a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java +++ b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java @@ -16,7 +16,11 @@ package io.grpc; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -79,7 +83,11 @@ public List getMetricInstruments() { public DoubleCounterMetricInstrument registerDoubleCounter(String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { - // Null check for arguments + checkArgument(!Strings.isNullOrEmpty(name), "missing metric name"); + checkNotNull(description, "description"); + checkNotNull(unit, "unit"); + checkNotNull(requiredLabelKeys, "requiredLabelKeys"); + checkNotNull(optionalLabelKeys, "optionalLabelKeys"); synchronized (lock) { if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); @@ -113,6 +121,11 @@ public DoubleCounterMetricInstrument registerDoubleCounter(String name, public LongCounterMetricInstrument registerLongCounter(String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { + checkArgument(!Strings.isNullOrEmpty(name), "missing metric name"); + checkNotNull(description, "description"); + checkNotNull(unit, "unit"); + checkNotNull(requiredLabelKeys, "requiredLabelKeys"); + checkNotNull(optionalLabelKeys, "optionalLabelKeys"); synchronized (lock) { if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); @@ -147,6 +160,12 @@ public LongCounterMetricInstrument registerLongCounter(String name, public DoubleHistogramMetricInstrument registerDoubleHistogram(String name, String description, String unit, List bucketBoundaries, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { + checkArgument(!Strings.isNullOrEmpty(name), "missing metric name"); + checkNotNull(description, "description"); + checkNotNull(unit, "unit"); + checkNotNull(bucketBoundaries, "bucketBoundaries"); + checkNotNull(requiredLabelKeys, "requiredLabelKeys"); + checkNotNull(optionalLabelKeys, "optionalLabelKeys"); synchronized (lock) { if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); @@ -182,6 +201,12 @@ public DoubleHistogramMetricInstrument registerDoubleHistogram(String name, public LongHistogramMetricInstrument registerLongHistogram(String name, String description, String unit, List bucketBoundaries, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { + checkArgument(!Strings.isNullOrEmpty(name), "missing metric name"); + checkNotNull(description, "description"); + checkNotNull(unit, "unit"); + checkNotNull(bucketBoundaries, "bucketBoundaries"); + checkNotNull(requiredLabelKeys, "requiredLabelKeys"); + checkNotNull(optionalLabelKeys, "optionalLabelKeys"); synchronized (lock) { if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); @@ -217,6 +242,11 @@ public LongHistogramMetricInstrument registerLongHistogram(String name, public LongGaugeMetricInstrument registerLongGauge(String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { + checkArgument(!Strings.isNullOrEmpty(name), "missing metric name"); + checkNotNull(description, "description"); + checkNotNull(unit, "unit"); + checkNotNull(requiredLabelKeys, "requiredLabelKeys"); + checkNotNull(optionalLabelKeys, "optionalLabelKeys"); synchronized (lock) { if (registeredMetricNames.contains(name)) { throw new IllegalStateException("Metric with name " + name + " already exists"); diff --git a/api/src/main/java/io/grpc/MetricRecorder.java b/api/src/main/java/io/grpc/MetricRecorder.java index bcf237f9328..24968b30854 100644 --- a/api/src/main/java/io/grpc/MetricRecorder.java +++ b/api/src/main/java/io/grpc/MetricRecorder.java @@ -16,11 +16,6 @@ package io.grpc; -import io.grpc.DoubleCounterMetricInstrument; -import io.grpc.DoubleHistogramMetricInstrument; -import io.grpc.Internal; -import io.grpc.LongCounterMetricInstrument; -import io.grpc.LongHistogramMetricInstrument; import java.util.List; /** @@ -30,25 +25,25 @@ @Internal public interface MetricRecorder { /** - * Records a value for a double-precision counter metric instrument. + * Adds a value for a double-precision counter metric instrument. * - * @param metricInstrument The counter metric instrument to record the value against. - * @param value The value to record. + * @param metricInstrument The counter metric instrument to add the value against. + * @param value The value to add. * @param requiredLabelValues A list of required label values for the metric. * @param optionalLabelValues A list of additional, optional label values for the metric. */ - default void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value, + default void addDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value, List requiredLabelValues, List optionalLabelValues) {} /** - * Records a value for a long valued counter metric instrument. + * Adds a value for a long valued counter metric instrument. * - * @param metricInstrument The counter metric instrument to record the value against. - * @param value The value to record. + * @param metricInstrument The counter metric instrument to add the value against. + * @param value The value to add. * @param requiredLabelValues A list of required label values for the metric. * @param optionalLabelValues A list of additional, optional label values for the metric. */ - default void recordLongCounter(LongCounterMetricInstrument metricInstrument, long value, + default void addLongCounter(LongCounterMetricInstrument metricInstrument, long value, List requiredLabelValues, List optionalLabelValues) {} /** diff --git a/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java index d1f721056d4..eab3e84d09d 100644 --- a/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java +++ b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java @@ -16,6 +16,8 @@ package io.grpc.internal; +import static com.google.common.base.Preconditions.checkArgument; + import com.google.common.annotations.VisibleForTesting; import io.grpc.DoubleCounterMetricInstrument; import io.grpc.DoubleHistogramMetricInstrument; @@ -54,8 +56,16 @@ final class MetricRecorderImpl implements MetricRecorder { * @param optionalLabelValues the optional label values for the metric. */ @Override - public void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value, + public void addDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value, List requiredLabelValues, List optionalLabelValues) { + checkArgument(requiredLabelValues != null + && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(), + "Incorrect number of required labels provided. Expected: " + + metricInstrument.getRequiredLabelKeys().size()); + checkArgument(optionalLabelValues != null + && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(), + "Incorrect number of optional labels provided. Expected: " + + metricInstrument.getOptionalLabelKeys().size()); for (MetricSink sink : metricSinks) { // TODO(dnvindhya): Move updating measures logic from sink to here List measures = sink.getMetricsMeasures(); @@ -78,8 +88,16 @@ public void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument, * @param optionalLabelValues the optional label values for the metric. */ @Override - public void recordLongCounter(LongCounterMetricInstrument metricInstrument, long value, + public void addLongCounter(LongCounterMetricInstrument metricInstrument, long value, List requiredLabelValues, List optionalLabelValues) { + checkArgument(requiredLabelValues != null + && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(), + "Incorrect number of required labels provided. Expected: " + + metricInstrument.getRequiredLabelKeys().size()); + checkArgument(optionalLabelValues != null + && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(), + "Incorrect number of optional labels provided. Expected: " + + metricInstrument.getOptionalLabelKeys().size()); for (MetricSink sink : metricSinks) { List measures = sink.getMetricsMeasures(); if (measures.size() <= metricInstrument.getIndex()) { @@ -103,6 +121,14 @@ public void recordLongCounter(LongCounterMetricInstrument metricInstrument, long @Override public void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrument, double value, List requiredLabelValues, List optionalLabelValues) { + checkArgument(requiredLabelValues != null + && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(), + "Incorrect number of required labels provided. Expected: " + + metricInstrument.getRequiredLabelKeys().size()); + checkArgument(optionalLabelValues != null + && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(), + "Incorrect number of optional labels provided. Expected: " + + metricInstrument.getOptionalLabelKeys().size()); for (MetricSink sink : metricSinks) { List measures = sink.getMetricsMeasures(); if (measures.size() <= metricInstrument.getIndex()) { @@ -126,6 +152,14 @@ public void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrume @Override public void recordLongHistogram(LongHistogramMetricInstrument metricInstrument, long value, List requiredLabelValues, List optionalLabelValues) { + checkArgument(requiredLabelValues != null + && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(), + "Incorrect number of required labels provided. Expected: " + + metricInstrument.getRequiredLabelKeys().size()); + checkArgument(optionalLabelValues != null + && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(), + "Incorrect number of optional labels provided. Expected: " + + metricInstrument.getOptionalLabelKeys().size()); for (MetricSink sink : metricSinks) { List measures = sink.getMetricsMeasures(); if (measures.size() <= metricInstrument.getIndex()) { diff --git a/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java b/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java index 9f664b61344..9f79732c7eb 100644 --- a/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java +++ b/core/src/test/java/io/grpc/internal/MetricRecorderImplTest.java @@ -85,12 +85,12 @@ public void recordCounter() { when(mockSink.getMetricsMeasures()).thenReturn( Arrays.asList(new Object(), new Object(), new Object(), new Object())); - recorder.recordDoubleCounter(doubleCounterInstrument, 1.0, REQUIRED_LABEL_VALUES, + recorder.addDoubleCounter(doubleCounterInstrument, 1.0, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); verify(mockSink, times(2)).recordDoubleCounter(eq(doubleCounterInstrument), eq(1D), eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); - recorder.recordLongCounter(longCounterInstrument, 1, REQUIRED_LABEL_VALUES, + recorder.addLongCounter(longCounterInstrument, 1, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); verify(mockSink, times(2)).recordLongCounter(eq(longCounterInstrument), eq(1L), eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); @@ -122,14 +122,14 @@ public void newRegisteredMetricUpdateMeasures() { when(mockSink.getMetricsMeasures()).thenReturn(new ArrayList<>()); // Double Counter - recorder.recordDoubleCounter(doubleCounterInstrument, 1.0, REQUIRED_LABEL_VALUES, + recorder.addDoubleCounter(doubleCounterInstrument, 1.0, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); verify(mockSink, times(2)).updateMeasures(anyList()); verify(mockSink, times(2)).recordDoubleCounter(eq(doubleCounterInstrument), eq(1D), eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); // Long Counter - recorder.recordLongCounter(longCounterInstrument, 1, REQUIRED_LABEL_VALUES, + recorder.addLongCounter(longCounterInstrument, 1, REQUIRED_LABEL_VALUES, OPTIONAL_LABEL_VALUES); verify(mockSink, times(4)).updateMeasures(anyList()); verify(mockSink, times(2)).recordLongCounter(eq(longCounterInstrument), eq(1L), @@ -149,4 +149,76 @@ public void newRegisteredMetricUpdateMeasures() { verify(mockSink, times(2)).recordLongHistogram(eq(longHistogramInstrument), eq(99L), eq(REQUIRED_LABEL_VALUES), eq(OPTIONAL_LABEL_VALUES)); } + + @Test(expected = IllegalArgumentException.class) + public void recordDoubleCounterMismatchedRequiredLabelValues() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.addDoubleCounter(doubleCounterInstrument, 1.0, ImmutableList.of(), + OPTIONAL_LABEL_VALUES); + } + + @Test(expected = IllegalArgumentException.class) + public void recordLongCounterMismatchedRequiredLabelValues() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.addLongCounter(longCounterInstrument, 1, ImmutableList.of(), + OPTIONAL_LABEL_VALUES); + } + + @Test(expected = IllegalArgumentException.class) + public void recordDoubleHistogramMismatchedRequiredLabelValues() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.recordDoubleHistogram(doubleHistogramInstrument, 99.0, ImmutableList.of(), + OPTIONAL_LABEL_VALUES); + } + + @Test(expected = IllegalArgumentException.class) + public void recordLongHistogramMismatchedRequiredLabelValues() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.recordLongHistogram(longHistogramInstrument, 99, ImmutableList.of(), + OPTIONAL_LABEL_VALUES); + } + + @Test(expected = IllegalArgumentException.class) + public void recordDoubleCounterMismatchedOptionalLabelValues() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.addDoubleCounter(doubleCounterInstrument, 1.0, REQUIRED_LABEL_VALUES, + ImmutableList.of()); + } + + @Test(expected = IllegalArgumentException.class) + public void recordLongCounterMismatchedOptionalLabelValues() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.addLongCounter(longCounterInstrument, 1, REQUIRED_LABEL_VALUES, + ImmutableList.of()); + } + + @Test(expected = IllegalArgumentException.class) + public void recordDoubleHistogramMismatchedOptionalLabelValues() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.recordDoubleHistogram(doubleHistogramInstrument, 99.0, REQUIRED_LABEL_VALUES, + ImmutableList.of()); + } + + @Test(expected = IllegalArgumentException.class) + public void recordLongHistogramMismatchedOptionalLabelValues() { + when(mockSink.getMetricsMeasures()).thenReturn( + Arrays.asList(new Object(), new Object(), new Object(), new Object())); + + recorder.recordLongHistogram(longHistogramInstrument, 99, REQUIRED_LABEL_VALUES, + ImmutableList.of()); + } } From 677f82c73d811945af164da6b775c648970edc35 Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Fri, 26 Apr 2024 08:10:43 -0700 Subject: [PATCH 5/7] addressed review comments --- api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java | 2 +- .../main/java/io/grpc/DoubleHistogramMetricInstrument.java | 2 +- api/src/main/java/io/grpc/LongCounterMetricInstrument.java | 2 +- api/src/main/java/io/grpc/LongGaugeMetricInstrument.java | 2 +- api/src/main/java/io/grpc/LongHistogramMetricInstrument.java | 2 +- api/src/main/java/io/grpc/MetricInstrumentRegistry.java | 5 +++-- 6 files changed, 8 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java b/api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java index 3f07d83d58f..6657a6d8d42 100644 --- a/api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java +++ b/api/src/main/java/io/grpc/DoubleCounterMetricInstrument.java @@ -23,7 +23,7 @@ */ @Internal public final class DoubleCounterMetricInstrument extends PartialMetricInstrument { - public DoubleCounterMetricInstrument(int index, String name, String description, String unit, + DoubleCounterMetricInstrument(int index, String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); } diff --git a/api/src/main/java/io/grpc/DoubleHistogramMetricInstrument.java b/api/src/main/java/io/grpc/DoubleHistogramMetricInstrument.java index 9039a8c62c1..76d1b284daa 100644 --- a/api/src/main/java/io/grpc/DoubleHistogramMetricInstrument.java +++ b/api/src/main/java/io/grpc/DoubleHistogramMetricInstrument.java @@ -25,7 +25,7 @@ public final class DoubleHistogramMetricInstrument extends PartialMetricInstrument { private final List bucketBoundaries; - public DoubleHistogramMetricInstrument(int index, String name, String description, String unit, + DoubleHistogramMetricInstrument(int index, String name, String description, String unit, List bucketBoundaries, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); diff --git a/api/src/main/java/io/grpc/LongCounterMetricInstrument.java b/api/src/main/java/io/grpc/LongCounterMetricInstrument.java index 73516dfb9e4..ab29ba59266 100644 --- a/api/src/main/java/io/grpc/LongCounterMetricInstrument.java +++ b/api/src/main/java/io/grpc/LongCounterMetricInstrument.java @@ -23,7 +23,7 @@ */ @Internal public final class LongCounterMetricInstrument extends PartialMetricInstrument { - public LongCounterMetricInstrument(int index, String name, String description, String unit, + LongCounterMetricInstrument(int index, String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); } diff --git a/api/src/main/java/io/grpc/LongGaugeMetricInstrument.java b/api/src/main/java/io/grpc/LongGaugeMetricInstrument.java index 8e24dd715ef..c8804e98832 100644 --- a/api/src/main/java/io/grpc/LongGaugeMetricInstrument.java +++ b/api/src/main/java/io/grpc/LongGaugeMetricInstrument.java @@ -23,7 +23,7 @@ */ @Internal public final class LongGaugeMetricInstrument extends PartialMetricInstrument { - public LongGaugeMetricInstrument(int index, String name, String description, String unit, + LongGaugeMetricInstrument(int index, String name, String description, String unit, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); } diff --git a/api/src/main/java/io/grpc/LongHistogramMetricInstrument.java b/api/src/main/java/io/grpc/LongHistogramMetricInstrument.java index 2a4e56ffd5a..87006021f4f 100644 --- a/api/src/main/java/io/grpc/LongHistogramMetricInstrument.java +++ b/api/src/main/java/io/grpc/LongHistogramMetricInstrument.java @@ -25,7 +25,7 @@ public final class LongHistogramMetricInstrument extends PartialMetricInstrument { private final List bucketBoundaries; - public LongHistogramMetricInstrument(int index, String name, String description, String unit, + LongHistogramMetricInstrument(int index, String name, String description, String unit, List bucketBoundaries, List requiredLabelKeys, List optionalLabelKeys, boolean enableByDefault) { super(index, name, description, unit, requiredLabelKeys, optionalLabelKeys, enableByDefault); diff --git a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java index 681850f7169..4737e91a9dd 100644 --- a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java +++ b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java @@ -33,12 +33,12 @@ */ @Internal public final class MetricInstrumentRegistry { - public static final int INITIAL_INSTRUMENT_CAPACITY = 5; + static final int INITIAL_INSTRUMENT_CAPACITY = 5; private static MetricInstrumentRegistry instance; private final Object lock = new Object(); private final Set registeredMetricNames; private volatile MetricInstrument[] metricInstruments; - private volatile int instrumentListCapacity = INITIAL_INSTRUMENT_CAPACITY; + private volatile int instrumentListCapacity; @GuardedBy("lock") private int nextAvailableMetricIndex; @@ -46,6 +46,7 @@ public final class MetricInstrumentRegistry { MetricInstrumentRegistry() { this.metricInstruments = new MetricInstrument[INITIAL_INSTRUMENT_CAPACITY]; this.registeredMetricNames = new HashSet<>(); + this.instrumentListCapacity = metricInstruments.length; } /** From 2dbf553a916f4b4f9b33d8de58e7aa7cf80ea94d Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Fri, 26 Apr 2024 10:25:11 -0700 Subject: [PATCH 6/7] updated to use array length --- .../io/grpc/MetricInstrumentRegistry.java | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java index 4737e91a9dd..457d880e3fe 100644 --- a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java +++ b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java @@ -36,18 +36,14 @@ public final class MetricInstrumentRegistry { static final int INITIAL_INSTRUMENT_CAPACITY = 5; private static MetricInstrumentRegistry instance; private final Object lock = new Object(); - private final Set registeredMetricNames; - private volatile MetricInstrument[] metricInstruments; - private volatile int instrumentListCapacity; + private final Set registeredMetricNames = new HashSet<>(); + private volatile MetricInstrument[] metricInstruments = + new MetricInstrument[INITIAL_INSTRUMENT_CAPACITY]; @GuardedBy("lock") private int nextAvailableMetricIndex; @VisibleForTesting - MetricInstrumentRegistry() { - this.metricInstruments = new MetricInstrument[INITIAL_INSTRUMENT_CAPACITY]; - this.registeredMetricNames = new HashSet<>(); - this.instrumentListCapacity = metricInstruments.length; - } + MetricInstrumentRegistry() {} /** * Returns the default metric instrument registry. @@ -94,7 +90,7 @@ public DoubleCounterMetricInstrument registerDoubleCounter(String name, throw new IllegalStateException("Metric with name " + name + " already exists"); } int index = nextAvailableMetricIndex; - if (index + 1 == instrumentListCapacity) { + if (index + 1 == metricInstruments.length) { resizeMetricInstruments(); } DoubleCounterMetricInstrument instrument = new DoubleCounterMetricInstrument( @@ -132,7 +128,7 @@ public LongCounterMetricInstrument registerLongCounter(String name, throw new IllegalStateException("Metric with name " + name + " already exists"); } int index = nextAvailableMetricIndex; - if (index + 1 == instrumentListCapacity) { + if (index + 1 == metricInstruments.length) { resizeMetricInstruments(); } LongCounterMetricInstrument instrument = new LongCounterMetricInstrument( @@ -172,7 +168,7 @@ public DoubleHistogramMetricInstrument registerDoubleHistogram(String name, throw new IllegalStateException("Metric with name " + name + " already exists"); } int index = nextAvailableMetricIndex; - if (index + 1 == instrumentListCapacity) { + if (index + 1 == metricInstruments.length) { resizeMetricInstruments(); } DoubleHistogramMetricInstrument instrument = new DoubleHistogramMetricInstrument( @@ -213,7 +209,7 @@ public LongHistogramMetricInstrument registerLongHistogram(String name, throw new IllegalStateException("Metric with name " + name + " already exists"); } int index = nextAvailableMetricIndex; - if (index + 1 == instrumentListCapacity) { + if (index + 1 == metricInstruments.length) { resizeMetricInstruments(); } LongHistogramMetricInstrument instrument = new LongHistogramMetricInstrument( @@ -253,7 +249,7 @@ public LongGaugeMetricInstrument registerLongGauge(String name, String descripti throw new IllegalStateException("Metric with name " + name + " already exists"); } int index = nextAvailableMetricIndex; - if (index + 1 == instrumentListCapacity) { + if (index + 1 == metricInstruments.length) { resizeMetricInstruments(); } LongGaugeMetricInstrument instrument = new LongGaugeMetricInstrument( @@ -268,9 +264,9 @@ public LongGaugeMetricInstrument registerLongGauge(String name, String descripti private synchronized void resizeMetricInstruments() { // Increase the capacity of the metricInstruments array by INITIAL_INSTRUMENT_CAPACITY - instrumentListCapacity += INITIAL_INSTRUMENT_CAPACITY; + int newInstrumentsCapacity = metricInstruments.length + INITIAL_INSTRUMENT_CAPACITY; MetricInstrument[] resizedMetricInstruments = Arrays.copyOf(metricInstruments, - instrumentListCapacity); + newInstrumentsCapacity); metricInstruments = resizedMetricInstruments; } } From 5f98cdf431566fca19149cb92b4f2ad35ecb781b Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Fri, 26 Apr 2024 11:39:24 -0700 Subject: [PATCH 7/7] added lock for instruments array --- api/src/main/java/io/grpc/MetricInstrumentRegistry.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java index 457d880e3fe..fd74eba281d 100644 --- a/api/src/main/java/io/grpc/MetricInstrumentRegistry.java +++ b/api/src/main/java/io/grpc/MetricInstrumentRegistry.java @@ -36,8 +36,10 @@ public final class MetricInstrumentRegistry { static final int INITIAL_INSTRUMENT_CAPACITY = 5; private static MetricInstrumentRegistry instance; private final Object lock = new Object(); + @GuardedBy("lock") private final Set registeredMetricNames = new HashSet<>(); - private volatile MetricInstrument[] metricInstruments = + @GuardedBy("lock") + private MetricInstrument[] metricInstruments = new MetricInstrument[INITIAL_INSTRUMENT_CAPACITY]; @GuardedBy("lock") private int nextAvailableMetricIndex; @@ -262,7 +264,8 @@ public LongGaugeMetricInstrument registerLongGauge(String name, String descripti } } - private synchronized void resizeMetricInstruments() { + @GuardedBy("lock") + private void resizeMetricInstruments() { // Increase the capacity of the metricInstruments array by INITIAL_INSTRUMENT_CAPACITY int newInstrumentsCapacity = metricInstruments.length + INITIAL_INSTRUMENT_CAPACITY; MetricInstrument[] resizedMetricInstruments = Arrays.copyOf(metricInstruments,