Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add MetricRecorder implementation #11128

Merged
merged 7 commits into from
Apr 26, 2024
Merged

Add MetricRecorder implementation #11128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
413393a
added MetricRecordedImpl and unite tests for MetricInstrumentRegistry
DNVindhya Apr 25, 2024
295e58d
changed MetricRcorderImpl to public
DNVindhya Apr 25, 2024
0f95f8e
updated MetricInstrumenetRegistry to use array instead of ArrayList; …
DNVindhya Apr 25, 2024
7167dcb
Renamed record<>Counter APIs to add<>Counter. Added check for mismatc…
DNVindhya Apr 26, 2024
677f82c
addressed review comments
DNVindhya Apr 26, 2024
2dbf553
updated to use array length
DNVindhya Apr 26, 2024
5f98cdf
added lock for instruments array
DNVindhya Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
88 changes: 47 additions & 41 deletions api/src/main/java/io/grpc/MetricInstrumentRegistry.java
View file
Open in desktop
Expand Up @@ -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;
DNVindhya marked this conversation as resolved.
Show resolved Hide resolved
private static MetricInstrumentRegistry instance;
private final Object lock = new Object();
private final Set<String> registeredMetricNames;
private final AtomicInteger instrumentIndexAlloc = new AtomicInteger();
private volatile List<MetricInstrument> 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;
Copy link
Member

Choose a reason for hiding this comment

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

Use metricInstruments.length instead

@GuardedBy("lock")
private int nextAvailableMetricIndex;

@VisibleForTesting
MetricInstrumentRegistry(List<MetricInstrument> metricInstruments,
Set<String> registeredMetricNames) {
this.metricInstruments = metricInstruments;
this.registeredMetricNames = registeredMetricNames;
MetricInstrumentRegistry() {
this.metricInstruments = new MetricInstrument[INITIAL_INSTRUMENT_CAPACITY];
this.registeredMetricNames = new HashSet<>();
}

/**
Expand All @@ -62,7 +58,10 @@
* Returns a list of registered metric instruments.
*/
public List<MetricInstrument> getMetricInstruments() {
return ImmutableList.copyOf(metricInstruments);
synchronized (lock) {
return Collections.unmodifiableList(
Arrays.asList(Arrays.copyOfRange(metricInstruments, 0, nextAvailableMetricIndex)));
}
}

/**
Expand All @@ -85,15 +84,16 @@
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();

Check warning on line 89 in api/src/main/java/io/grpc/MetricInstrumentRegistry.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/io/grpc/MetricInstrumentRegistry.java#L89 

Added line #L89 was not covered by tests
}
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;
}
}
Expand All @@ -117,15 +117,16 @@
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();

Check warning on line 122 in api/src/main/java/io/grpc/MetricInstrumentRegistry.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/io/grpc/MetricInstrumentRegistry.java#L122 

Added line #L122 was not covered by tests
}
LongCounterMetricInstrument instrument = new LongCounterMetricInstrument(
index, name, description, unit, requiredLabelKeys, optionalLabelKeys,
enableByDefault);
metricInstruments.add(instrument);
metricInstruments[index] = instrument;
registeredMetricNames.add(name);
nextAvailableMetricIndex += 1;
return instrument;
}
}
Expand All @@ -150,16 +151,17 @@
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();

Check warning on line 156 in api/src/main/java/io/grpc/MetricInstrumentRegistry.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/io/grpc/MetricInstrumentRegistry.java#L156 

Added line #L156 was not covered by tests
}
DoubleHistogramMetricInstrument instrument = new DoubleHistogramMetricInstrument(
index, name, description, unit, bucketBoundaries, requiredLabelKeys,
optionalLabelKeys,
enableByDefault);
metricInstruments.add(instrument);
metricInstruments[index] = instrument;
registeredMetricNames.add(name);
nextAvailableMetricIndex += 1;
return instrument;
}
}
Expand All @@ -184,16 +186,17 @@
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();
}
LongHistogramMetricInstrument instrument = new LongHistogramMetricInstrument(
index, name, description, unit, bucketBoundaries, requiredLabelKeys,
optionalLabelKeys,
enableByDefault);
metricInstruments.add(instrument);
metricInstruments[index] = instrument;
registeredMetricNames.add(name);
nextAvailableMetricIndex += 1;
return instrument;
}
}
Expand All @@ -214,26 +217,29 @@
public LongGaugeMetricInstrument registerLongGauge(String name, String description,
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
String unit, List<String> requiredLabelKeys, List<String> 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();

Check warning on line 226 in api/src/main/java/io/grpc/MetricInstrumentRegistry.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/io/grpc/MetricInstrumentRegistry.java#L226 

Added line #L226 was not covered by tests
}
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<MetricInstrument> 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;
}
}
50 changes: 7 additions & 43 deletions api/src/test/java/io/grpc/MetricInstrumentRegistryTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand All @@ -31,7 +30,6 @@
*/
@RunWith(JUnit4.class)
public class MetricInstrumentRegistryTest {
private static final int TEST_INSTRUMENT_CAPACITY = 20;
private static final ImmutableList<String> REQUIRED_LABEL_KEYS = ImmutableList.of("KEY1", "KEY2");
private static final ImmutableList<String> OPTIONAL_LABEL_KEYS = ImmutableList.of(
"OPTIONAL_KEY_1");
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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(
Expand All @@ -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);
}

}
9 changes: 2 additions & 7 deletions api/src/testFixtures/java/io/grpc/MetricInstrumentRegistryAccessor.java
View file
Open in desktop
Expand Up @@ -16,9 +16,6 @@

package io.grpc;

import java.util.List;
import java.util.Set;

/**
* Accesses test-only methods of {@link MetricInstrumentRegistry}.
*/
Expand All @@ -27,9 +24,7 @@ public final class MetricInstrumentRegistryAccessor {
private MetricInstrumentRegistryAccessor() {
}

public static MetricInstrumentRegistry createMetricInstrumentRegistry(
List<MetricInstrument> instruments,
Set<String> registeredNames) {
return new MetricInstrumentRegistry(instruments, registeredNames);
public static MetricInstrumentRegistry createMetricInstrumentRegistry() {
return new MetricInstrumentRegistry();
}
}
6 changes: 3 additions & 3 deletions core/src/main/java/io/grpc/internal/MetricRecorderImpl.java
View file
Open in desktop
Expand Up @@ -34,10 +34,10 @@
* <p>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<MetricSink> metricSinks;
private volatile MetricInstrumentRegistry registry;
private final MetricInstrumentRegistry registry;

@VisibleForTesting
MetricRecorderImpl(List<MetricSink> metricSinks, MetricInstrumentRegistry registry) {
Expand All @@ -57,6 +57,7 @@ public final class MetricRecorderImpl implements MetricRecorder {
public void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value,
List<String> requiredLabelValues, List<String> optionalLabelValues) {
for (MetricSink sink : metricSinks) {
// TODO(dnvindhya): Move updating measures logic from sink to here
List<Object> measures = sink.getMetricsMeasures();
if (measures.size() <= metricInstrument.getIndex()) {
// Measures may need updating in two cases:
Expand All @@ -68,7 +69,6 @@ public void recordDoubleCounter(DoubleCounterMetricInstrument metricInstrument,
}
}


/**
* Records a long counter value.
*
Expand Down