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

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/InstrumentDescriptor.java

@@ -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;
 
 /**
@@ -23,6 +23,7 @@ import javax.annotation.concurrent.Immutable;
 public abstract class InstrumentDescriptor {
 
   private final SourceInfo sourceInfo = SourceInfo.fromCurrentStack();
+  private int hashcode;
 
   public static InstrumentDescriptor create(
       String name,
@@ -46,6 +47,9 @@ public abstract class InstrumentDescriptor {
 
   public abstract InstrumentValueType getValueType();
 
+  /**
+   * Not part of instrument identity. Ignored from {@link #hashCode()} and {@link #equals(Object)}.
+   */
   public abstract Advice getAdvice();
 
   /**
@@ -56,7 +60,47 @@ public abstract class InstrumentDescriptor {
     return sourceInfo;
   }
 
-  @Memoized
+  /**
+   * 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 int hashCode() {
+    int result = hashcode;
+    if (result == 0) {
+      result = 1;
+      result *= 1000003;
+      result ^= getName().toLowerCase(Locale.ROOT).hashCode();
+      result *= 1000003;
+      result ^= getDescription().hashCode();
+      result *= 1000003;
+      result ^= getUnit().hashCode();
+      result *= 1000003;
+      result ^= getType().hashCode();
+      result *= 1000003;
+      result ^= getValueType().hashCode();
+      hashcode = result;
+    }
+    return result;
+  }
+
+  /**
+   * Uses case-insensitive version of {@link #getName()}, ignores {@link #getAdvice()} (not part of
+   * instrument identity}, ignores {@link #getSourceInfo()}.
+   */
+  @Override
+  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;
+  }
 }

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java

@@ -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;
 
@@ -29,6 +29,7 @@ import javax.annotation.concurrent.Immutable;
 public abstract class MetricDescriptor {
 
   private final AtomicReference<SourceInfo> viewSourceInfo = new AtomicReference<>();
+  private int hashcode;
 
   /**
    * Constructs a metric descriptor with no instrument and default view.
@@ -94,36 +95,38 @@ public abstract class MetricDescriptor {
     return AggregationUtil.aggregationName(getView().getAggregation());
   }
 
-  @Memoized
+  /** Uses case-insensitive version of {@link #getName()}. */
   @Override
-  public abstract int hashCode();
+  public final int hashCode() {
+    int result = hashcode;
+    if (result == 0) {
+      result = 1;
+      result *= 1000003;
+      result ^= getName().toLowerCase(Locale.ROOT).hashCode();
+      result *= 1000003;
+      result ^= getDescription().hashCode();
+      result *= 1000003;
+      result ^= getView().hashCode();
+      result *= 1000003;
+      result ^= getSourceInstrument().hashCode();
+      hashcode = result;
+    }
+    return result;
+  }
 
-  /**
+  /** Uses case-insensitive version of {@link #getName()}. */
-   * Returns true if another metric descriptor is compatible with this one.
+  @Override
-   *
+  public final boolean equals(Object o) {
-   * <p>A metric descriptor is compatible with another if the following are true:
+    if (o == this) {
-   *
+      return true;
-   * <ul>
+    }
-   *   <li>{@link #getName()} is equal
+    if (o instanceof MetricDescriptor) {
-   *   <li>{@link #getDescription()} is equal
+      MetricDescriptor that = (MetricDescriptor) o;
-   *   <li>{@link #getAggregationName()} is equal
+      return this.getName().equalsIgnoreCase(that.getName())
-   *   <li>{@link InstrumentDescriptor#getName()} is equal
+          && this.getDescription().equals(that.getDescription())
-   *   <li>{@link InstrumentDescriptor#getDescription()} is equal
+          && this.getView().equals(that.getView())
-   *   <li>{@link InstrumentDescriptor#getUnit()} is equal
+          && this.getSourceInstrument().equals(that.getSourceInstrument());
-   *   <li>{@link InstrumentDescriptor#getType()} is equal
+    }
-   *   <li>{@link InstrumentDescriptor#getValueType()} is equal
+    return false;
-   * </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());
   }
 }

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/DebugUtils.java

@@ -22,7 +22,7 @@ public final class 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.

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistry.java

@@ -22,9 +22,10 @@ import java.util.logging.Logger;
  *
  * <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
+ * descriptors share the same name, but have some difference in identifying fields, an identity
- * conflicts on {@link #register(MetricStorage)} and logs diagnostic information when they occur.
+ * conflict has occurred. The registry detects identity conflicts on {@link
- * See {@link MetricDescriptor#isCompatibleWith(MetricDescriptor)} for definition of compatibility.
+ * #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.
@@ -75,9 +76,9 @@ public class MetricStorageRegistry {
         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())
+      if (existing.getName().equalsIgnoreCase(descriptor.getName())) {
-          && !existing.isCompatibleWith(descriptor)) {
         logger.log(Level.WARNING, DebugUtils.duplicateMetricErrorMessage(existing, descriptor));
         break; // Only log information about the first conflict found to reduce noise
       }

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/IdentityTest.java

@@ -8,11 +8,13 @@ package io.opentelemetry.sdk.metrics;
 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;
@@ -106,7 +108,7 @@ class IdentityTest {
   }
 
   @Test
-  void sameMeterDifferentInstrumentNoViews() {
+  void sameMeterDifferentInstrumentNameNoViews() {
     SdkMeterProvider meterProvider = builder.build();
 
     meterProvider.get("meter1").counterBuilder("counter1").build().add(10);
@@ -130,6 +132,139 @@ class IdentityTest {
     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

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptorTest.java

@@ -12,6 +12,7 @@ import io.opentelemetry.sdk.metrics.InstrumentType;
 import io.opentelemetry.sdk.metrics.InstrumentValueType;
 import io.opentelemetry.sdk.metrics.View;
 import io.opentelemetry.sdk.metrics.internal.debug.SourceInfo;
+import java.util.Arrays;
 import org.junit.jupiter.api.Test;
 
 class MetricDescriptorTest {
@@ -57,7 +58,7 @@ class MetricDescriptorTest {
   }
 
   @Test
-  void metricDescriptor_isCompatible() {
+  void metricDescriptor_equals() {
     View view = View.builder().build();
     InstrumentDescriptor instrument =
         InstrumentDescriptor.create(
@@ -67,50 +68,44 @@ class MetricDescriptorTest {
             InstrumentType.COUNTER,
             InstrumentValueType.DOUBLE,
             Advice.empty());
-    MetricDescriptor descriptor =
+    MetricDescriptor descriptor1 =
         MetricDescriptor.create(view, SourceInfo.fromCurrentStack(), instrument);
-    // Same name, description, source name, source description, source unit, source type, and source
+    // Same name (case-insensitive), description, view, source name (case-insensitive), source unit,
-    // value type is compatible
+    // source description, source type, and source value type is equal. Advice is not part of
-    assertThat(
+    // equals.
-            descriptor.isCompatibleWith(
+    MetricDescriptor descriptor2 =
         MetricDescriptor.create(
             View.builder().build(),
             SourceInfo.fromCurrentStack(),
             InstrumentDescriptor.create(
-                        "name",
+                "Name",
                 "description",
                 "unit",
                 InstrumentType.COUNTER,
                 InstrumentValueType.DOUBLE,
-                        Advice.empty()))))
+                Advice.create(Arrays.asList(1.0, 2.0))));
-        .isTrue();
+    assertThat(descriptor1).isEqualTo(descriptor2).hasSameHashCodeAs(descriptor2);
-    // Different name overridden by view is not compatible
+    // Different name overridden by view is not equal
-    assertThat(
+    descriptor2 =
-            descriptor.isCompatibleWith(
         MetricDescriptor.create(
-                    View.builder().setName("bar").build(),
+            View.builder().setName("bar").build(), SourceInfo.fromCurrentStack(), instrument);
-                    SourceInfo.fromCurrentStack(),
+    assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
-                    instrument)))
+    // Different description overridden by view is not equal
-        .isFalse();
+    descriptor2 =
-    // Different description overridden by view is not compatible
-    assertThat(
-            descriptor.isCompatibleWith(
         MetricDescriptor.create(
             View.builder().setDescription("foo").build(),
             SourceInfo.fromCurrentStack(),
-                    instrument)))
+            instrument);
-        .isFalse();
+    assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
-    // Different aggregation overridden by view is not compatible
+    // Different aggregation overridden by view is not equal
-    assertThat(
+    descriptor2 =
-            descriptor.isCompatibleWith(
         MetricDescriptor.create(
             View.builder().setAggregation(Aggregation.lastValue()).build(),
             SourceInfo.fromCurrentStack(),
-                    instrument)))
+            instrument);
-        .isFalse();
+    assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
-    // Different instrument source name is not compatible
+    // Different instrument source name is not equal
-    assertThat(
+    descriptor2 =
-            descriptor.isCompatibleWith(
         MetricDescriptor.create(
             view,
             SourceInfo.fromCurrentStack(),
@@ -120,11 +115,10 @@ class MetricDescriptorTest {
                 "unit",
                 InstrumentType.COUNTER,
                 InstrumentValueType.DOUBLE,
-                        Advice.empty()))))
+                Advice.empty()));
-        .isFalse();
+    assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
-    // Different instrument source description is not compatible
+    // Different instrument source description is not equal
-    assertThat(
+    descriptor2 =
-            descriptor.isCompatibleWith(
         MetricDescriptor.create(
             view,
             SourceInfo.fromCurrentStack(),
@@ -134,11 +128,10 @@ class MetricDescriptorTest {
                 "unit",
                 InstrumentType.COUNTER,
                 InstrumentValueType.DOUBLE,
-                        Advice.empty()))))
+                Advice.empty()));
-        .isFalse();
+    assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
-    // Different instrument source unit is not compatible
+    // Different instrument source unit is not equal
-    assertThat(
+    descriptor2 =
-            descriptor.isCompatibleWith(
         MetricDescriptor.create(
             view,
             SourceInfo.fromCurrentStack(),
@@ -148,11 +141,10 @@ class MetricDescriptorTest {
                 "foo",
                 InstrumentType.COUNTER,
                 InstrumentValueType.DOUBLE,
-                        Advice.empty()))))
+                Advice.empty()));
-        .isFalse();
+    assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
-    // Different instrument source type is not compatible
+    // Different instrument source type is not equal
-    assertThat(
+    descriptor2 =
-            descriptor.isCompatibleWith(
         MetricDescriptor.create(
             view,
             SourceInfo.fromCurrentStack(),
@@ -162,11 +154,10 @@ class MetricDescriptorTest {
                 "unit",
                 InstrumentType.HISTOGRAM,
                 InstrumentValueType.DOUBLE,
-                        Advice.empty()))))
+                Advice.empty()));
-        .isFalse();
+    assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
-    // Different instrument source value type is not compatible
+    // Different instrument source value type is not equal
-    assertThat(
+    descriptor2 =
-            descriptor.isCompatibleWith(
         MetricDescriptor.create(
             view,
             SourceInfo.fromCurrentStack(),
@@ -176,7 +167,7 @@ class MetricDescriptorTest {
                 "unit",
                 InstrumentType.COUNTER,
                 InstrumentValueType.LONG,
-                        Advice.empty()))))
+                Advice.empty()));
-        .isFalse();
+    assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
   }
 }