From 30b1b3b70efc599e443520b469b3b5939c1482b4 Mon Sep 17 00:00:00 2001 From: jack-berg Date: Tue, 21 Dec 2021 12:29:17 -0600 Subject: [PATCH 1/2] Log warning when duplicate async instruments are registered --- .../opentelemetry/testing/TestSourceInfo.java | 11 ++- .../internal/descriptor/MetricDescriptor.java | 16 ++++ .../metrics/internal/state/DebugUtils.java | 9 ++- .../internal/state/MetricStorageRegistry.java | 12 ++- .../state/MetricStorageRegistryTest.java | 78 ++++++++----------- 5 files changed, 67 insertions(+), 59 deletions(-) diff --git a/sdk/metrics/src/debugEnabledTest/java/io/opentelemetry/testing/TestSourceInfo.java b/sdk/metrics/src/debugEnabledTest/java/io/opentelemetry/testing/TestSourceInfo.java index 7e80e9878df..5eb8f26a2da 100644 --- a/sdk/metrics/src/debugEnabledTest/java/io/opentelemetry/testing/TestSourceInfo.java +++ b/sdk/metrics/src/debugEnabledTest/java/io/opentelemetry/testing/TestSourceInfo.java @@ -38,7 +38,7 @@ void testDuplicateExceptionMessage_pureInstruments() { "name", "description", "unit", - InstrumentType.HISTOGRAM, + InstrumentType.OBSERVABLE_SUM, InstrumentValueType.DOUBLE)); MetricDescriptor simpleWithNewDescription = MetricDescriptor.create( @@ -49,11 +49,11 @@ void testDuplicateExceptionMessage_pureInstruments() { .contains("Found duplicate metric definition: name") .contains("- Unit [unit2] does not match [unit]") .contains("- Description [description2] does not match [description]") - .contains("- InstrumentType [COUNTER] does not match [HISTOGRAM]") + .contains("- InstrumentType [COUNTER] does not match [OBSERVABLE_SUM]") .contains("- InstrumentValueType [LONG] does not match [DOUBLE]") + .contains("- InstrumentType [OBSERVABLE_SUM] is async and already registered") .contains(simple.getSourceInstrument().getSourceInfo().multiLineDebugString()) - .contains( - "Original instrument registered with same name but different description, unit, instrument type, or instrument value type.") + .contains("Original instrument registered with same name but is incompatible.") .contains( simpleWithNewDescription.getSourceInstrument().getSourceInfo().multiLineDebugString()); } @@ -118,8 +118,7 @@ void testDuplicateExceptionMessage_viewBasedConflict2() { .contains("FROM instrument name2") .contains(simple.getSourceInstrument().getSourceInfo().multiLineDebugString()) .contains("- Unit [unit] does not match [unit2]") - .contains( - "Original instrument registered with same name but different description, unit, instrument type, or instrument value type.") + .contains("Original instrument registered with same name but is incompatible.") .contains( simpleWithNewDescription.getSourceInstrument().getSourceInfo().multiLineDebugString()); } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java index 142fda3282d..48d65c38049 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java @@ -86,4 +86,20 @@ public boolean isCompatibleWith(MetricDescriptor other) { && Objects.equals( getSourceInstrument().getValueType(), other.getSourceInstrument().getValueType()); } + + /** Returns whether the descriptor describes an async {@link InstrumentType}. */ + public boolean isAsync() { + switch (getSourceInstrument().getType()) { + case OBSERVABLE_UP_DOWN_SUM: + case OBSERVABLE_GAUGE: + case OBSERVABLE_SUM: + return true; + case HISTOGRAM: + case COUNTER: + case UP_DOWN_COUNTER: + return false; + } + throw new IllegalStateException( + "Unrecognized instrument type " + getSourceInstrument().getType()); + } } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/DebugUtils.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/DebugUtils.java index 2dcf60d9e51..36bd1ea36a8 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/DebugUtils.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/DebugUtils.java @@ -96,13 +96,18 @@ public static String duplicateMetricErrorMessage( .append(existing.getSourceInstrument().getValueType()) .append("]\n"); } + if (existing.isAsync()) { + result + .append("- InstrumentType [") + .append(existing.getSourceInstrument().getType()) + .append("] is async and already registered\n"); + } // Next we write out where the existing metric descriptor came from, either a raw instrument // or a view on a raw instrument. if (existing.getName().equals(existing.getSourceInstrument().getName())) { result - .append( - "Original instrument registered with same name but different description, unit, instrument type, or instrument value type.\n") + .append("Original instrument registered with same name but is incompatible.\n") .append(existing.getSourceInstrument().getSourceInfo().multiLineDebugString()) .append("\n"); } else { diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistry.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistry.java index 690332b5e09..9041cdfe556 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistry.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistry.java @@ -58,6 +58,10 @@ public I register(I storage) { MetricDescriptor descriptor = storage.getMetricDescriptor(); MetricStorage oldOrNewStorage = registry.computeIfAbsent(descriptor.getName().toLowerCase(), key -> storage); + // Metric didn't already exist in registry, return. + if (storage == oldOrNewStorage) { + return (I) oldOrNewStorage; + } // Make sure the storage is compatible. if (!oldOrNewStorage.getMetricDescriptor().isCompatibleWith(descriptor)) { throw new DuplicateMetricStorageException( @@ -65,14 +69,14 @@ public I register(I storage) { descriptor, "Metric with same name and different descriptor already created."); } - // Make sure we aren't mixing sync + async. - if (!storage.getClass().equals(oldOrNewStorage.getClass())) { + // Descriptors are compatible, but can't register async instruments multiple times. + if (descriptor.isAsync()) { throw new DuplicateMetricStorageException( oldOrNewStorage.getMetricDescriptor(), descriptor, - "Metric with same name and different instrument already created."); + "Async metric with same name has already been created."); } - + // Metric already existed, and is compatible with new storage. return (I) oldOrNewStorage; } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistryTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistryTest.java index c474d3a51ae..988ef909cb7 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistryTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistryTest.java @@ -10,23 +10,31 @@ import io.opentelemetry.api.common.Attributes; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.InstrumentType; +import io.opentelemetry.sdk.metrics.common.InstrumentValueType; import io.opentelemetry.sdk.metrics.data.MetricData; +import io.opentelemetry.sdk.metrics.internal.descriptor.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.internal.descriptor.MetricDescriptor; import io.opentelemetry.sdk.metrics.internal.export.CollectionInfo; +import io.opentelemetry.sdk.metrics.view.View; import io.opentelemetry.sdk.resources.Resource; import org.junit.jupiter.api.Test; /** Unit tests for {@link MetricStorageRegistry}. */ class MetricStorageRegistryTest { - private static final MetricDescriptor METRIC_DESCRIPTOR = - MetricDescriptor.create("name", "description", "1"); - private static final MetricDescriptor OTHER_METRIC_DESCRIPTOR = - MetricDescriptor.create("name", "other_description", "1"); + private static final MetricDescriptor SYNC_DESCRIPTOR = + descriptor("sync", "description", InstrumentType.COUNTER); + private static final MetricDescriptor OTHER_SYNC_DESCRIPTOR = + descriptor("sync", "other_description", InstrumentType.COUNTER); + private static final MetricDescriptor ASYNC_DESCRIPTOR = + descriptor("async", "description", InstrumentType.OBSERVABLE_GAUGE); + + private final MeterSharedState meterSharedState = + MeterSharedState.create(InstrumentationLibraryInfo.empty()); @Test - void register() { - MeterSharedState meterSharedState = MeterSharedState.create(InstrumentationLibraryInfo.empty()); - TestMetricStorage testInstrument = new TestMetricStorage(METRIC_DESCRIPTOR); + void register_Sync() { + TestMetricStorage testInstrument = new TestMetricStorage(SYNC_DESCRIPTOR); assertThat(meterSharedState.getMetricStorageRegistry().register(testInstrument)) .isSameAs(testInstrument); assertThat(meterSharedState.getMetricStorageRegistry().register(testInstrument)) @@ -34,14 +42,13 @@ void register() { assertThat( meterSharedState .getMetricStorageRegistry() - .register(new TestMetricStorage(METRIC_DESCRIPTOR))) + .register(new TestMetricStorage(SYNC_DESCRIPTOR))) .isSameAs(testInstrument); } @Test - void register_OtherDescriptor() { - MeterSharedState meterSharedState = MeterSharedState.create(InstrumentationLibraryInfo.empty()); - TestMetricStorage testInstrument = new TestMetricStorage(METRIC_DESCRIPTOR); + void register_SyncIncompatibleDescriptor() { + TestMetricStorage testInstrument = new TestMetricStorage(SYNC_DESCRIPTOR); assertThat(meterSharedState.getMetricStorageRegistry().register(testInstrument)) .isSameAs(testInstrument); @@ -49,15 +56,14 @@ void register_OtherDescriptor() { () -> meterSharedState .getMetricStorageRegistry() - .register(new TestMetricStorage(OTHER_METRIC_DESCRIPTOR))) + .register(new TestMetricStorage(OTHER_SYNC_DESCRIPTOR))) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Metric with same name and different descriptor already created."); } @Test - void register_OtherInstance() { - MeterSharedState meterSharedState = MeterSharedState.create(InstrumentationLibraryInfo.empty()); - TestMetricStorage testInstrument = new TestMetricStorage(METRIC_DESCRIPTOR); + void register_Async() { + TestMetricStorage testInstrument = new TestMetricStorage(ASYNC_DESCRIPTOR); assertThat(meterSharedState.getMetricStorageRegistry().register(testInstrument)) .isSameAs(testInstrument); @@ -65,45 +71,23 @@ void register_OtherInstance() { () -> meterSharedState .getMetricStorageRegistry() - .register(new OtherTestMetricStorage(METRIC_DESCRIPTOR))) + .register(new TestMetricStorage(ASYNC_DESCRIPTOR))) .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Metric with same name and different instrument already created."); + .hasMessageContaining("Async metric with same name has already been created."); } - private static final class TestMetricStorage implements MetricStorage, WriteableMetricStorage { - private final MetricDescriptor descriptor; - - TestMetricStorage(MetricDescriptor descriptor) { - this.descriptor = descriptor; - } - - @Override - public MetricDescriptor getMetricDescriptor() { - return descriptor; - } - - @Override - public MetricData collectAndReset( - CollectionInfo collectionInfo, - Resource resource, - InstrumentationLibraryInfo instrumentationLibraryInfo, - long startEpochNanos, - long epochNanos, - boolean suppressSynchronousCollection) { - return null; - } - - @Override - public BoundStorageHandle bind(Attributes attributes) { - return null; - } + private static MetricDescriptor descriptor( + String name, String description, InstrumentType instrumentType) { + return MetricDescriptor.create( + View.builder().build(), + InstrumentDescriptor.create( + name, description, "1", instrumentType, InstrumentValueType.DOUBLE)); } - private static final class OtherTestMetricStorage - implements MetricStorage, WriteableMetricStorage { + private static final class TestMetricStorage implements MetricStorage, WriteableMetricStorage { private final MetricDescriptor descriptor; - OtherTestMetricStorage(MetricDescriptor descriptor) { + TestMetricStorage(MetricDescriptor descriptor) { this.descriptor = descriptor; } From e78286290a8b08990023b99e08c299690a95a31d Mon Sep 17 00:00:00 2001 From: jack-berg Date: Mon, 27 Dec 2021 14:20:37 -0600 Subject: [PATCH 2/2] Improve test coverage --- .../descriptor/MetricDescriptorTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptorTest.java index ebb78c86ded..6bd2d44e23d 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptorTest.java @@ -123,4 +123,21 @@ void metricDescriptor_isCompatible() { InstrumentValueType.LONG)))) .isFalse(); } + + @Test + void isAsync() { + assertThat(descriptorForInstrument(InstrumentType.OBSERVABLE_UP_DOWN_SUM).isAsync()).isTrue(); + assertThat(descriptorForInstrument(InstrumentType.OBSERVABLE_GAUGE).isAsync()).isTrue(); + assertThat(descriptorForInstrument(InstrumentType.OBSERVABLE_SUM).isAsync()).isTrue(); + assertThat(descriptorForInstrument(InstrumentType.HISTOGRAM).isAsync()).isFalse(); + assertThat(descriptorForInstrument(InstrumentType.COUNTER).isAsync()).isFalse(); + assertThat(descriptorForInstrument(InstrumentType.UP_DOWN_COUNTER).isAsync()).isFalse(); + } + + private static MetricDescriptor descriptorForInstrument(InstrumentType instrumentType) { + InstrumentDescriptor instrument = + InstrumentDescriptor.create( + "name", "description", "unit", instrumentType, InstrumentValueType.DOUBLE); + return MetricDescriptor.create(View.builder().build(), instrument); + } }