From 14d6f22521acfd93457ae563dfb80ed5100b4dca Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Wed, 17 Nov 2021 14:08:42 -0800 Subject: [PATCH 1/5] Allow for metric to have the same name when they are from different meters --- src/OpenTelemetry/Metrics/MeterProviderSdk.cs | 16 ++++++----- .../Metrics/MetricAPITest.cs | 27 ++++++++++++------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs index fcea36be402..2771ded7a74 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs @@ -153,12 +153,14 @@ internal MeterProviderSdk( for (int i = 0; i < maxCountMetricsToBeCreated; i++) { var metricStreamConfig = metricStreamConfigs[i]; - var metricStreamName = metricStreamConfig?.Name ?? instrument.Name; + var meterName = instrument.Meter.Name; + var metricName = metricStreamConfig?.Name ?? instrument.Name; + var metricStreamName = $"{meterName}.{metricName}"; - if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricStreamName)) + if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricName)) { OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored( - metricStreamName, + metricName, instrument.Meter.Name, "Metric name is invalid.", "The name must comply with the OpenTelemetry specification."); @@ -195,7 +197,7 @@ internal MeterProviderSdk( string[] tagKeysInteresting = metricStreamConfig?.TagKeys; double[] histogramBucketBounds = (metricStreamConfig is HistogramConfiguration histogramConfig && histogramConfig.BucketBounds != null) ? histogramConfig.BucketBounds : null; - metric = new Metric(instrument, temporality, metricStreamName, metricDescription, histogramBucketBounds, tagKeysInteresting); + metric = new Metric(instrument, temporality, metricName, metricDescription, histogramBucketBounds, tagKeysInteresting); this.metrics[index] = metric; metrics.Add(metric); @@ -245,11 +247,13 @@ internal MeterProviderSdk( return; } + var meterName = instrument.Meter.Name; var metricName = instrument.Name; + var metricStreamName = $"{meterName}.{metricName}"; Metric metric = null; lock (this.instrumentCreationLock) { - if (this.metricStreamNames.Contains(metricName)) + if (this.metricStreamNames.Contains(metricStreamName)) { OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "Metric name conflicting with existing name.", "Either change the name of the instrument or change name using View."); return; @@ -265,7 +269,7 @@ internal MeterProviderSdk( { metric = new Metric(instrument, temporality, metricName, instrument.Description); this.metrics[index] = metric; - this.metricStreamNames.Add(metricName); + this.metricStreamNames.Add(metricStreamName); } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs index 630aef73826..eaa6bd12621 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs @@ -102,9 +102,11 @@ public void ObserverCallbackExceptionTest() } [Theory] - [InlineData(AggregationTemporality.Cumulative)] - [InlineData(AggregationTemporality.Delta)] - public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality temporality) + [InlineData(AggregationTemporality.Cumulative, true)] + [InlineData(AggregationTemporality.Cumulative, false)] + [InlineData(AggregationTemporality.Delta, true)] + [InlineData(AggregationTemporality.Delta, false)] + public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality temporality, bool hasView) { var metricItems = new List(); var metricExporter = new InMemoryExporter(metricItems); @@ -115,11 +117,17 @@ public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality tempor }; using var meter1 = new Meter($"{Utils.GetCurrentMethodName()}.1.{temporality}"); using var meter2 = new Meter($"{Utils.GetCurrentMethodName()}.2.{temporality}"); - using var meterProvider = Sdk.CreateMeterProviderBuilder() + var meterProviderBuilder = Sdk.CreateMeterProviderBuilder() .AddMeter(meter1.Name) .AddMeter(meter2.Name) - .AddReader(metricReader) - .Build(); + .AddReader(metricReader); + + if (hasView) + { + meterProviderBuilder.AddView("name1", new MetricStreamConfiguration() { Description = "description" }); + } + + using var meterProvider = meterProviderBuilder.Build(); // Expecting one metric stream. var counterLong = meter1.CreateCounter("name1"); @@ -137,15 +145,14 @@ public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality tempor metricReader.Collect(); Assert.Single(metricItems); - // The following will also be ignored - // as the name is same. - // (the Meter name is not part of stream name) + // The following will not be ignored + // as it is the same metric name but different meter. var anotherCounterSameNameDiffMeter = meter2.CreateCounter("name1"); anotherCounterSameNameDiffMeter.Add(10); counterLong.Add(10); metricItems.Clear(); metricReader.Collect(); - Assert.Single(metricItems); + Assert.Equal(2, metricItems.Count); } [Theory] From c9c193b77f3434ceefcd59ee520ff51439e68d1e Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Wed, 17 Nov 2021 14:13:08 -0800 Subject: [PATCH 2/5] Update changelog --- src/OpenTelemetry/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index b9f69f91f0d..b1c94701d5a 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +* Metrics with the same name but different from different meters is allowed. + ([#2634](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2634)) + * Metrics SDK will not provide inactive Metrics to delta exporter. ([#2629](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2629)) From e1ae5d08b4f1730a51ad94fb670f38a4cebe713c Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Wed, 17 Nov 2021 14:13:49 -0800 Subject: [PATCH 3/5] Fix changelog --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index b1c94701d5a..5f99c8f7593 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -* Metrics with the same name but different from different meters is allowed. +* Metrics with the same name but from different meters is allowed. ([#2634](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2634)) * Metrics SDK will not provide inactive Metrics to delta exporter. From c60b71e74ceeb7458f386cc37d4e3c49862e92a9 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Wed, 17 Nov 2021 15:31:23 -0800 Subject: [PATCH 4/5] Update src/OpenTelemetry/CHANGELOG.md Co-authored-by: Reiley Yang --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 5f99c8f7593..7b2edf66061 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -* Metrics with the same name but from different meters is allowed. +* Metrics with the same name but from different meters are allowed. ([#2634](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2634)) * Metrics SDK will not provide inactive Metrics to delta exporter. From 0507f3fc72b4c83bd7c2bbee19a2906401fd445c Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Wed, 17 Nov 2021 16:47:20 -0800 Subject: [PATCH 5/5] Split into two tests --- .../Metrics/MetricAPITest.cs | 47 ++++++++++++++++--- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs index eaa6bd12621..2ab58a86ce5 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs @@ -106,7 +106,7 @@ public void ObserverCallbackExceptionTest() [InlineData(AggregationTemporality.Cumulative, false)] [InlineData(AggregationTemporality.Delta, true)] [InlineData(AggregationTemporality.Delta, false)] - public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality temporality, bool hasView) + public void DuplicateInstrumentNamesFromSameMeterAreNotAllowed(AggregationTemporality temporality, bool hasView) { var metricItems = new List(); var metricExporter = new InMemoryExporter(metricItems); @@ -115,11 +115,9 @@ public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality tempor { PreferredAggregationTemporality = temporality, }; - using var meter1 = new Meter($"{Utils.GetCurrentMethodName()}.1.{temporality}"); - using var meter2 = new Meter($"{Utils.GetCurrentMethodName()}.2.{temporality}"); + using var meter = new Meter($"{Utils.GetCurrentMethodName()}.{temporality}"); var meterProviderBuilder = Sdk.CreateMeterProviderBuilder() - .AddMeter(meter1.Name) - .AddMeter(meter2.Name) + .AddMeter(meter.Name) .AddReader(metricReader); if (hasView) @@ -130,7 +128,7 @@ public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality tempor using var meterProvider = meterProviderBuilder.Build(); // Expecting one metric stream. - var counterLong = meter1.CreateCounter("name1"); + var counterLong = meter.CreateCounter("name1"); counterLong.Add(10); metricReader.Collect(); Assert.Single(metricItems); @@ -138,12 +136,47 @@ public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality tempor // The following will be ignored as // metric of same name exists. // Metric stream will remain one. - var anotherCounterSameName = meter1.CreateCounter("name1"); + var anotherCounterSameName = meter.CreateCounter("name1"); anotherCounterSameName.Add(10); counterLong.Add(10); metricItems.Clear(); metricReader.Collect(); Assert.Single(metricItems); + } + + [Theory] + [InlineData(AggregationTemporality.Cumulative, true)] + [InlineData(AggregationTemporality.Cumulative, false)] + [InlineData(AggregationTemporality.Delta, true)] + [InlineData(AggregationTemporality.Delta, false)] + public void DuplicateInstrumentNamesFromDifferentMetersAreAllowed(AggregationTemporality temporality, bool hasView) + { + var metricItems = new List(); + var metricExporter = new InMemoryExporter(metricItems); + + var metricReader = new BaseExportingMetricReader(metricExporter) + { + PreferredAggregationTemporality = temporality, + }; + using var meter1 = new Meter($"{Utils.GetCurrentMethodName()}.1.{temporality}"); + using var meter2 = new Meter($"{Utils.GetCurrentMethodName()}.2.{temporality}"); + var meterProviderBuilder = Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + .AddMeter(meter2.Name) + .AddReader(metricReader); + + if (hasView) + { + meterProviderBuilder.AddView("name1", new MetricStreamConfiguration() { Description = "description" }); + } + + using var meterProvider = meterProviderBuilder.Build(); + + // Expecting one metric stream. + var counterLong = meter1.CreateCounter("name1"); + counterLong.Add(10); + metricReader.Collect(); + Assert.Single(metricItems); // The following will not be ignored // as it is the same metric name but different meter.