Skip to content
This repository was archived by the owner on Dec 23, 2023. It is now read-only.

feat: Allow users to register the same Meter multiple times without exception #2017

Merged
merged 3 commits into from
Mar 10, 2020
Merged

feat: Allow users to register the same Meter multiple times without exception #2017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f68d69
feat: Allow register same metric multiple times
mayurkale22 Mar 9, 2020
8dc91d2
add more test
mayurkale22 Mar 9, 2020
cc9073d
remove redundant test
mayurkale22 Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/DerivedDoubleCumulativeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ public synchronized void clear() {
registeredPoints = Collections.<List<LabelValue>, PointWithFunction<?>>emptyMap();
}

@Override
public MetricDescriptor getMetricDescriptor() {
return metricDescriptor;
}

@javax.annotation.Nullable
@Override
public Metric getMetric(Clock clock) {
Expand Down
5 changes: 5 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/DerivedDoubleGaugeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ public synchronized void clear() {
registeredPoints = Collections.<List<LabelValue>, PointWithFunction<?>>emptyMap();
}

@Override
public MetricDescriptor getMetricDescriptor() {
return metricDescriptor;
}

@javax.annotation.Nullable
@Override
public Metric getMetric(Clock clock) {
Expand Down
5 changes: 5 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/DerivedLongCumulativeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ public synchronized void clear() {
registeredPoints = Collections.<List<LabelValue>, PointWithFunction<?>>emptyMap();
}

@Override
public MetricDescriptor getMetricDescriptor() {
return metricDescriptor;
}

@javax.annotation.Nullable
@Override
public Metric getMetric(Clock clock) {
Expand Down
5 changes: 5 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/DerivedLongGaugeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ public synchronized void clear() {
registeredPoints = Collections.<List<LabelValue>, PointWithFunction<?>>emptyMap();
}

@Override
public MetricDescriptor getMetricDescriptor() {
return metricDescriptor;
}

@javax.annotation.Nullable
@Override
public Metric getMetric(Clock clock) {
Expand Down
5 changes: 5 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/DoubleCumulativeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ public synchronized void clear() {
registeredPoints = Collections.<List<LabelValue>, PointImpl>emptyMap();
}

@Override
public MetricDescriptor getMetricDescriptor() {
return metricDescriptor;
}

private synchronized DoublePoint registerTimeSeries(List<LabelValue> labelValues) {
PointImpl existingPoint = registeredPoints.get(labelValues);
if (existingPoint != null) {
Expand Down
5 changes: 5 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/DoubleGaugeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ public synchronized void clear() {
registeredPoints = Collections.<List<LabelValue>, PointImpl>emptyMap();
}

@Override
public MetricDescriptor getMetricDescriptor() {
return metricDescriptor;
}

private synchronized DoublePoint registerTimeSeries(List<LabelValue> labelValues) {
PointImpl existingPoint = registeredPoints.get(labelValues);
if (existingPoint != null) {
Expand Down
5 changes: 5 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/LongCumulativeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ public synchronized void clear() {
registeredPoints = Collections.<List<LabelValue>, PointImpl>emptyMap();
}

@Override
public MetricDescriptor getMetricDescriptor() {
return metricDescriptor;
}

private synchronized LongPoint registerTimeSeries(List<LabelValue> labelValues) {
PointImpl existingPoint = registeredPoints.get(labelValues);
if (existingPoint != null) {
Expand Down
5 changes: 5 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/LongGaugeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ public synchronized void clear() {
registeredPoints = Collections.<List<LabelValue>, PointImpl>emptyMap();
}

@Override
public MetricDescriptor getMetricDescriptor() {
return metricDescriptor;
}

private synchronized LongPoint registerTimeSeries(List<LabelValue> labelValues) {
PointImpl existingPoint = registeredPoints.get(labelValues);
if (existingPoint != null) {
Expand Down
8 changes: 8 additions & 0 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/Meter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import io.opencensus.common.Clock;
import io.opencensus.metrics.export.Metric;
import io.opencensus.metrics.export.MetricDescriptor;
import javax.annotation.Nullable;

interface Meter {
Expand All @@ -31,4 +32,11 @@ interface Meter {
*/
@Nullable
Metric getMetric(Clock clock);

/**
* Provides a {@link io.opencensus.metrics.export.MetricDescriptor}.
*
* @return a {@code MetricDescriptor}.
*/
MetricDescriptor getMetricDescriptor();
}
36 changes: 16 additions & 20 deletions impl_core/src/main/java/io/opencensus/implcore/metrics/MetricRegistryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ public LongGauge addLongGauge(String name, MetricOptions options) {
options.getUnit(),
options.getLabelKeys(),
options.getConstantLabels());
registeredMeters.registerMeter(name, longGaugeMetric);
return longGaugeMetric;
return (LongGauge) registeredMeters.registerMeter(name, longGaugeMetric);
}

@Override
Expand All @@ -72,8 +71,7 @@ public DoubleGauge addDoubleGauge(String name, MetricOptions options) {
options.getUnit(),
options.getLabelKeys(),
options.getConstantLabels());
registeredMeters.registerMeter(name, doubleGaugeMetric);
return doubleGaugeMetric;
return (DoubleGauge) registeredMeters.registerMeter(name, doubleGaugeMetric);
}

@Override
Expand All @@ -85,8 +83,7 @@ public DerivedLongGauge addDerivedLongGauge(String name, MetricOptions options)
options.getUnit(),
options.getLabelKeys(),
options.getConstantLabels());
registeredMeters.registerMeter(name, derivedLongGauge);
return derivedLongGauge;
return (DerivedLongGauge) registeredMeters.registerMeter(name, derivedLongGauge);
}

@Override
Expand All @@ -98,8 +95,7 @@ public DerivedDoubleGauge addDerivedDoubleGauge(String name, MetricOptions optio
options.getUnit(),
options.getLabelKeys(),
options.getConstantLabels());
registeredMeters.registerMeter(name, derivedDoubleGauge);
return derivedDoubleGauge;
return (DerivedDoubleGauge) registeredMeters.registerMeter(name, derivedDoubleGauge);
}

@Override
Expand All @@ -112,8 +108,7 @@ public LongCumulative addLongCumulative(String name, MetricOptions options) {
options.getLabelKeys(),
options.getConstantLabels(),
clock.now());
registeredMeters.registerMeter(name, longCumulativeMetric);
return longCumulativeMetric;
return (LongCumulative) registeredMeters.registerMeter(name, longCumulativeMetric);
}

@Override
Expand All @@ -126,8 +121,7 @@ public DoubleCumulative addDoubleCumulative(String name, MetricOptions options)
options.getLabelKeys(),
options.getConstantLabels(),
clock.now());
registeredMeters.registerMeter(name, longCumulativeMetric);
return longCumulativeMetric;
return (DoubleCumulative) registeredMeters.registerMeter(name, longCumulativeMetric);
}

@Override
Expand All @@ -140,8 +134,7 @@ public DerivedLongCumulative addDerivedLongCumulative(String name, MetricOptions
options.getLabelKeys(),
options.getConstantLabels(),
clock.now());
registeredMeters.registerMeter(name, derivedLongCumulative);
return derivedLongCumulative;
return (DerivedLongCumulative) registeredMeters.registerMeter(name, derivedLongCumulative);
}

@Override
Expand All @@ -154,8 +147,7 @@ public DerivedDoubleCumulative addDerivedDoubleCumulative(String name, MetricOpt
options.getLabelKeys(),
options.getConstantLabels(),
clock.now());
registeredMeters.registerMeter(name, derivedDoubleCumulative);
return derivedDoubleCumulative;
return (DerivedDoubleCumulative) registeredMeters.registerMeter(name, derivedDoubleCumulative);
}

private static final class RegisteredMeters {
Expand All @@ -165,17 +157,21 @@ private Map<String, Meter> getRegisteredMeters() {
return registeredMeters;
}

private synchronized void registerMeter(String meterName, Meter meter) {
private synchronized Meter registerMeter(String meterName, Meter meter) {
Meter existingMeter = registeredMeters.get(meterName);
if (existingMeter != null) {
// TODO(mayurkale): Allow users to register the same Meter multiple times without exception.
throw new IllegalArgumentException(
"A different metric with the same name already registered.");
if (!existingMeter.getMetricDescriptor().equals(meter.getMetricDescriptor())) {
throw new IllegalArgumentException(
Copy link

Choose a reason for hiding this comment

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

(Note, this comment is not relevant for this PR, and is only intended for discussing whether this fix would also fix this specific issue for the Spanner client library).

@mayurkale22 Can you confirm that registering multiple metrics with different label values (but equal label keys) will not go down this code path?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you confirm that registering multiple metrics with different label values (but equal label keys) will not go down this code path?

Yes, it is normal to have different label values - those will be represented as individual TimeSeries in the metric. i.e:

Metric:
     {name, label_keys, description, unit, type, ...}
TimeSeries
    [{labels_values1, value1}, {labels_values2, value2}, ...]

See: https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/metrics/export/Metric.java#L51

The exception will be raised only when you use the same metric name but with a different type or unit or label_keys.

Copy link

Choose a reason for hiding this comment

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

That sounds good 👍 Thanks for looking into this.

I think this fix is better than what I added to the Spanner client library, so I'll close the PR.

The only concern that I still have is what it looks like in the dashboards when there are multiple different instances of a SessionPool with the same label values. I understand that it won't cause any errors in the client library, but how is it presented to the user? This situation would occur when an application is opening two connections to the same database with different SessionPoolOptions. This would also mean different values for constant metrics like MaxSessions.

"A different metric with the same name already registered.");
} else {
return existingMeter;
}
}

Map<String, Meter> registeredMetersCopy = new LinkedHashMap<String, Meter>(registeredMeters);
registeredMetersCopy.put(meterName, meter);
registeredMeters = Collections.unmodifiableMap(registeredMetersCopy);
return meter;
}
}

Expand Down
39 changes: 38 additions & 1 deletion impl_core/src/test/java/io/opencensus/implcore/metrics/MetricRegistryImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,47 @@ public void getMetrics() {
}

@Test
public void registerDifferentMetricSameName() {
public void shouldReturnSameObjectOnMultipleRegisterCall() {
LongGauge longGauge = metricRegistry.addLongGauge(NAME, METRIC_OPTIONS);
LongPoint longPoint = longGauge.getOrCreateTimeSeries(LABEL_VALUES);
longPoint.set(200);

LongGauge longGauge1 = metricRegistry.addLongGauge(NAME, METRIC_OPTIONS);
LongPoint longPoint1 =
longGauge1.getOrCreateTimeSeries(Collections.singletonList(LABEL_VALUE_2));
longPoint1.set(300);

assertThat(longGauge).isEqualTo(longGauge1);
Collection<Metric> metricCollections = metricRegistry.getMetricProducer().getMetrics();
assertThat(metricCollections.size()).isEqualTo(1);
assertThat(metricCollections)
.containsExactly(
Metric.create(
LONG_METRIC_DESCRIPTOR,
Arrays.asList(
TimeSeries.createWithOnePoint(
Arrays.asList(LABEL_VALUE, LABEL_VALUE_2),
Point.create(Value.longValue(200), TEST_TIME),
null),
TimeSeries.createWithOnePoint(
Arrays.asList(LABEL_VALUE_2, LABEL_VALUE_2),
Point.create(Value.longValue(300), TEST_TIME),
null))));
}

@Test
public void shouldThrowWhenRegisterExistingMetricWithDiffType() {
metricRegistry.addLongGauge(NAME, DESCRIPTION, UNIT, LABEL_KEYS);
thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("A different metric with the same name already registered.");
metricRegistry.addDoubleGauge(NAME, DESCRIPTION, UNIT, LABEL_KEYS);
}

@Test
public void shouldThrowWhenRegisterExistingMetricWithDiffDesc() {
metricRegistry.addLongGauge(NAME, DESCRIPTION, UNIT, LABEL_KEYS);
thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("A different metric with the same name already registered.");
metricRegistry.addLongGauge(NAME, "duplicate", UNIT, LABEL_KEYS);
}
}