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

Instruments with names which are case-insensitive equal contribute to… #5701

Merged
merged 2 commits into from
Aug 11, 2023
Merged

Instruments with names which are case-insensitive equal contribute to… #5701

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6bbd9b
Instruments with names which are case-insensitive equal contribute to…
jack-berg Aug 10, 2023
b0b030b
Memoize hashcode
jack-berg Aug 11, 2023
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
46 changes: 43 additions & 3 deletions .../src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/InstrumentDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
package io.opentelemetry.sdk.metrics.internal.descriptor;

import com.google.auto.value.AutoValue;
import com.google.auto.value.extension.memoized.Memoized;
import io.opentelemetry.sdk.metrics.InstrumentType;
import io.opentelemetry.sdk.metrics.InstrumentValueType;
import io.opentelemetry.sdk.metrics.internal.debug.SourceInfo;
import java.util.Locale;
import javax.annotation.concurrent.Immutable;

/**
Expand Down Expand Up @@ -46,6 +46,9 @@ public static InstrumentDescriptor create(

public abstract InstrumentValueType getValueType();

/**
* Not part of instrument identity. Ignored from {@link #hashCode()} and {@link #equals(Object)}.
*/
public abstract Advice getAdvice();

/**
Expand All @@ -56,7 +59,44 @@ public final SourceInfo getSourceInfo() {
return sourceInfo;
}

@Memoized
/**
* Uses case-insensitive version of {@link #getName()}, ignores {@link #getAdvice()} (not part of
* instrument identity}, ignores {@link #getSourceInfo()}.
*/
@Override
public final int hashCode() {
// TODO: memoize
int h = 1;
h *= 1000003;
h ^= getName().toLowerCase(Locale.ROOT).hashCode();
h *= 1000003;
h ^= getDescription().hashCode();
h *= 1000003;
h ^= getUnit().hashCode();
h *= 1000003;
h ^= getType().hashCode();
h *= 1000003;
h ^= getValueType().hashCode();
return h;
}

/**
* Uses case-insensitive version of {@link #getName()}, ignores {@link #getAdvice()} (not part of
* instrument identity}, ignores {@link #getSourceInfo()}.
*/
@Override
public abstract int hashCode();
public final boolean equals(Object o) {
if (o == this) {
return true;
}
if (o instanceof InstrumentDescriptor) {
InstrumentDescriptor that = (InstrumentDescriptor) o;
return this.getName().equalsIgnoreCase(that.getName())
&& this.getDescription().equals(that.getDescription())
&& this.getUnit().equals(that.getUnit())
&& this.getType().equals(that.getType())
&& this.getValueType().equals(that.getValueType());
}
return false;
}
}
59 changes: 29 additions & 30 deletions ...rics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
package io.opentelemetry.sdk.metrics.internal.descriptor;

import com.google.auto.value.AutoValue;
import com.google.auto.value.extension.memoized.Memoized;
import io.opentelemetry.sdk.metrics.Aggregation;
import io.opentelemetry.sdk.metrics.InstrumentType;
import io.opentelemetry.sdk.metrics.InstrumentValueType;
import io.opentelemetry.sdk.metrics.View;
import io.opentelemetry.sdk.metrics.internal.aggregator.AggregationUtil;
import io.opentelemetry.sdk.metrics.internal.debug.SourceInfo;
import java.util.Locale;
import java.util.concurrent.atomic.AtomicReference;
import javax.annotation.concurrent.Immutable;

Expand Down Expand Up @@ -94,36 +94,35 @@ public String getAggregationName() {
return AggregationUtil.aggregationName(getView().getAggregation());
}

@Memoized
/** Uses case-insensitive version of {@link #getName()}. */
@Override
public abstract int hashCode();
public final int hashCode() {
// TODO: memoize
int h = 1;
h *= 1000003;
h ^= getName().toLowerCase(Locale.ROOT).hashCode();
h *= 1000003;
h ^= getDescription().hashCode();
h *= 1000003;
h ^= getView().hashCode();
h *= 1000003;
h ^= getSourceInstrument().hashCode();
return h;
}

/**
* Returns true if another metric descriptor is compatible with this one.
*
* <p>A metric descriptor is compatible with another if the following are true:
*
* <ul>
* <li>{@link #getName()} is equal
* <li>{@link #getDescription()} is equal
* <li>{@link #getAggregationName()} is equal
* <li>{@link InstrumentDescriptor#getName()} is equal
* <li>{@link InstrumentDescriptor#getDescription()} is equal
* <li>{@link InstrumentDescriptor#getUnit()} is equal
* <li>{@link InstrumentDescriptor#getType()} is equal
* <li>{@link InstrumentDescriptor#getValueType()} is equal
* </ul>
*/
public boolean isCompatibleWith(MetricDescriptor other) {
return getName().equals(other.getName())
&& getDescription().equals(other.getDescription())
&& getAggregationName().equals(other.getAggregationName())
&& getSourceInstrument().getName().equals(other.getSourceInstrument().getName())
&& getSourceInstrument()
.getDescription()
.equals(other.getSourceInstrument().getDescription())
&& getSourceInstrument().getUnit().equals(other.getSourceInstrument().getUnit())
&& getSourceInstrument().getType().equals(other.getSourceInstrument().getType())
&& getSourceInstrument().getValueType().equals(other.getSourceInstrument().getValueType());
/** Uses case-insensitive version of {@link #getName()}. */
@Override
public final boolean equals(Object o) {
if (o == this) {
return true;
}
if (o instanceof MetricDescriptor) {
MetricDescriptor that = (MetricDescriptor) o;
return this.getName().equalsIgnoreCase(that.getName())
&& this.getDescription().equals(that.getDescription())
&& this.getView().equals(that.getView())
&& this.getSourceInstrument().equals(that.getSourceInstrument());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The aggregationName used in the previous implementation is part of the view, so that's why it's not needed here. Just remarking because it gave me a small pause.

return false;
}
}
2 changes: 1 addition & 1 deletion sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/DebugUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ private DebugUtils() {}
* Creates a detailed error message comparing two {@link MetricDescriptor}s.
*
* <p>Called when the metrics with the descriptors have the same name, but {@link
* MetricDescriptor#isCompatibleWith(MetricDescriptor)} is {@code false}.
* MetricDescriptor#equals(Object)} is {@code false}.
*
* <p>This should identify all issues between the descriptor and log information on where they are
* defined. Users should be able to find/fix issues based on this error.
Expand Down
11 changes: 6 additions & 5 deletions ...rics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@
*
* <p>Each descriptor in the registry results in an exported metric stream. Under normal
* circumstances each descriptor shares a unique {@link MetricDescriptor#getName()}. When multiple
* descriptors share the same name, an identity conflict has occurred. The registry detects identity
* conflicts on {@link #register(MetricStorage)} and logs diagnostic information when they occur.
* See {@link MetricDescriptor#isCompatibleWith(MetricDescriptor)} for definition of compatibility.
* descriptors share the same name, but have some difference in identifying fields, an identity
* conflict has occurred. The registry detects identity conflicts on {@link
* #register(MetricStorage)} and logs diagnostic information when they occur. See {@link
* MetricDescriptor#equals(Object)} for definition of identity equality.
*
* <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
* at any time.
Expand Down Expand Up @@ -75,9 +76,9 @@ public <I extends MetricStorage> I register(I newStorage) {
continue;
}
MetricDescriptor existing = storage.getMetricDescriptor();
// TODO: consider this alternative
// Check compatibility of metrics which share the same case-insensitive name
if (existing.getName().equalsIgnoreCase(descriptor.getName())
&& !existing.isCompatibleWith(descriptor)) {
if (existing.getName().equalsIgnoreCase(descriptor.getName())) {
logger.log(Level.WARNING, DebugUtils.duplicateMetricErrorMessage(existing, descriptor));
break; // Only log information about the first conflict found to reduce noise
}
Expand Down
137 changes: 136 additions & 1 deletion sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/IdentityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;

import io.github.netmikey.logunit.api.LogCapturer;
import io.opentelemetry.extension.incubator.metrics.ExtendedDoubleHistogramBuilder;
import io.opentelemetry.internal.testing.slf4j.SuppressLogger;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.metrics.internal.state.MetricStorageRegistry;
import io.opentelemetry.sdk.metrics.internal.view.ViewRegistry;
import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader;
import java.util.Arrays;
import java.util.concurrent.atomic.AtomicLong;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -106,7 +108,7 @@ void sameMeterSameInstrumentNoViews() {
}

@Test
void sameMeterDifferentInstrumentNoViews() {
void sameMeterDifferentInstrumentNameNoViews() {
SdkMeterProvider meterProvider = builder.build();

meterProvider.get("meter1").counterBuilder("counter1").build().add(10);
Expand All @@ -130,6 +132,139 @@ void sameMeterDifferentInstrumentNoViews() {
assertThat(metricStorageRegistryLogs.getEvents()).hasSize(0);
}

@Test
void sameMeterDifferentInstrumentNameCaseNoViews() {
SdkMeterProvider meterProvider = builder.build();

meterProvider.get("meter1").counterBuilder("Counter1").build().add(10);
meterProvider.get("meter1").counterBuilder("counter1").build().add(10);

assertThat(reader.collectAllMetrics())
.satisfiesExactlyInAnyOrder(
metricData ->
assertThat(metricData)
.hasInstrumentationScope(forMeter("meter1"))
.hasName("Counter1")
.hasLongSumSatisfying(
sum -> sum.hasPointsSatisfying(point -> point.hasValue(20))));

assertThat(metricStorageRegistryLogs.getEvents()).hasSize(0);
}

@Test
@SuppressLogger(MetricStorageRegistry.class)
void sameMeterSameInstrumentNameDifferentIdentifyingFieldsNoViews() {
SdkMeterProvider meterProvider = builder.build();

meterProvider.get("meter1").counterBuilder("counter1").build().add(10);
// Same name, different unit
meterProvider.get("meter1").counterBuilder("counter1").setUnit("unit1").build().add(10);
// Same name, different description
meterProvider
.get("meter1")
.counterBuilder("counter1")
.setDescription("description1")
.build()
.add(10);
// Same name, different value type
meterProvider.get("meter1").counterBuilder("counter1").ofDoubles().build().add(10);
// Same name, different instrument type
meterProvider.get("meter1").upDownCounterBuilder("counter1").build().add(10);

// When name is the same, but some identifying field is different (unit, description, value
// type, instrument type) we produce different metric streams are produced and log a warning
assertThat(reader.collectAllMetrics())
.satisfiesExactlyInAnyOrder(
metricData ->
assertThat(metricData)
.hasInstrumentationScope(forMeter("meter1"))
.hasName("counter1")
.hasLongSumSatisfying(
sum -> sum.isMonotonic().hasPointsSatisfying(point -> point.hasValue(10))),
metricData ->
assertThat(metricData)
.hasInstrumentationScope(forMeter("meter1"))
.hasName("counter1")
.hasUnit("unit1")
.hasLongSumSatisfying(
sum -> sum.isMonotonic().hasPointsSatisfying(point -> point.hasValue(10))),
metricData ->
assertThat(metricData)
.hasInstrumentationScope(forMeter("meter1"))
.hasName("counter1")
.hasDescription("description1")
.hasLongSumSatisfying(
sum -> sum.isMonotonic().hasPointsSatisfying(point -> point.hasValue(10))),
metricData ->
assertThat(metricData)
.hasInstrumentationScope(forMeter("meter1"))
.hasName("counter1")
.hasDoubleSumSatisfying(
sum -> sum.isMonotonic().hasPointsSatisfying(point -> point.hasValue(10))),
metricData ->
assertThat(metricData)
.hasInstrumentationScope(forMeter("meter1"))
.hasName("counter1")
.hasLongSumSatisfying(
sum ->
sum.isNotMonotonic().hasPointsSatisfying(point -> point.hasValue(10))));

assertThat(metricStorageRegistryLogs.getEvents())
.allSatisfy(
logEvent ->
assertThat(logEvent.getMessage()).contains("Found duplicate metric definition"))
.hasSize(4);
}

@Test
void sameMeterSameInstrumentNameDifferentNonIdentifyingFieldsNoViews() {
SdkMeterProvider meterProvider = builder.build();

// Register histogram1, with and without advice. First registration without advice wins.
meterProvider.get("meter1").histogramBuilder("histogram1").build().record(8);
((ExtendedDoubleHistogramBuilder) meterProvider.get("meter1").histogramBuilder("histogram1"))
.setAdvice(advice -> advice.setExplicitBucketBoundaries(Arrays.asList(10.0, 20.0, 30.0)))
.build()
.record(8);

// Register histogram2, with and without advice. First registration with advice wins.
((ExtendedDoubleHistogramBuilder) meterProvider.get("meter1").histogramBuilder("histogram2"))
.setAdvice(advice -> advice.setExplicitBucketBoundaries(Arrays.asList(10.0, 20.0, 30.0)))
.build()
.record(8);
meterProvider.get("meter1").histogramBuilder("histogram2").build().record(8);

assertThat(reader.collectAllMetrics())
.satisfiesExactlyInAnyOrder(
metricData ->
assertThat(metricData)
.hasInstrumentationScope(forMeter("meter1"))
.hasName("histogram1")
.hasHistogramSatisfying(
histogram ->
histogram.hasPointsSatisfying(
point ->
point
.hasBucketCounts(
0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasBucketBoundaries(
0d, 5d, 10d, 25d, 50d, 75d, 100d, 250d, 500d, 750d,
1_000d, 2_500d, 5_000d, 7_500d, 10_000d))),
metricData ->
assertThat(metricData)
.hasInstrumentationScope(forMeter("meter1"))
.hasName("histogram2")
.hasHistogramSatisfying(
histogram ->
histogram.hasPointsSatisfying(
point ->
point
.hasBucketCounts(2, 0, 0, 0)
.hasBucketBoundaries(10.0, 20.0, 30.0))));

assertThat(metricStorageRegistryLogs.getEvents()).hasSize(0);
}

@Test
void differentMeterSameInstrumentNoViews() {
// Meters are the same if their name, version, and scope are all equals
Expand Down
Loading