From 88a5ce2644430f0e34261dba230bbf37a6458c92 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Thu, 13 Jan 2022 13:08:15 -0800 Subject: [PATCH 1/3] Fail-fast when using AddView with guaranteed conflict --- src/OpenTelemetry/CHANGELOG.md | 3 ++ .../Metrics/MeterProviderBuilderExtensions.cs | 16 +++++++++++ .../Metrics/MetricViewTests.cs | 28 +++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index d08f572cb45..db0d25f5b90 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -5,6 +5,9 @@ * Make `MetricPoint` of `MetricPointAccessor` readonly. ([2736](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2736)) +* Fail-fast when using AddView with guaranteed conflict. + ([2751](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2751)) + ## 1.2.0-rc1 Released 2021-Nov-29 diff --git a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs index 8e5485350fa..4bbd93d2143 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs @@ -57,6 +57,14 @@ public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProvid throw new ArgumentException($"Custom view name {name} is invalid.", nameof(name)); } + if (instrumentName.IndexOf('*') != -1) + { + throw new ArgumentException( + $"When view is used to provide name of metric stream, the instrument name" + + $"{instrumentName} should not contain wildcard selection, as it'll lead to conflict.", + nameof(instrumentName)); + } + if (meterProviderBuilder is MeterProviderBuilderBase meterProviderBuilderBase) { return meterProviderBuilderBase.AddView(instrumentName, name); @@ -86,6 +94,14 @@ public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProvid throw new ArgumentException($"Custom view name {metricStreamConfiguration.Name} is invalid.", nameof(metricStreamConfiguration.Name)); } + if (metricStreamConfiguration.Name != null && instrumentName.IndexOf('*') != -1) + { + throw new ArgumentException( + $"When view is used to provide name of metric stream, the instrument name" + + $"{instrumentName} should not contain wildcard selection, as it'll lead to conflict.", + nameof(instrumentName)); + } + if (metricStreamConfiguration is ExplicitBucketHistogramConfiguration histogramConfiguration) { // Validate histogram boundaries diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs index 2f76de70efc..ca9ec0012bb 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs @@ -92,6 +92,34 @@ public void AddViewWithNullMetricStreamConfigurationThrowsArgumentnullException( .Build()); } + [Fact] + public void AddViewWithNameThrowsInvalidArgumentExceptionWhenConflict() + { + var exportedItems = new List(); + + using var meter1 = new Meter("AddViewWithGuaranteedConflictThrowsInvalidArgumentException"); + + Assert.Throws(() => Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + .AddView("instrumenta.*", name: "newname") + .AddInMemoryExporter(exportedItems) + .Build()); + } + + [Fact] + public void AddViewWithNameInMetricStreamConfigurationThrowsInvalidArgumentExceptionWhenConflict() + { + var exportedItems = new List(); + + using var meter1 = new Meter("AddViewWithGuaranteedConflictThrowsInvalidArgumentException"); + + Assert.Throws(() => Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + .AddView("instrumenta.*", new MetricStreamConfiguration() { Name = "newname" }) + .AddInMemoryExporter(exportedItems) + .Build()); + } + [Theory] [MemberData(nameof(MetricTestData.InvalidHistogramBoundaries), MemberType = typeof(MetricTestData))] public void AddViewWithInvalidHistogramBoundsThrowsArgumentException(double[] boundaries) From eb628041241b660e2d6f525d0d32ac74c618185b Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Thu, 13 Jan 2022 16:20:15 -0800 Subject: [PATCH 2/3] better msg --- .../Metrics/MeterProviderBuilderExtensions.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs index 4bbd93d2143..199a11bb499 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs @@ -60,8 +60,9 @@ public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProvid if (instrumentName.IndexOf('*') != -1) { throw new ArgumentException( - $"When view is used to provide name of metric stream, the instrument name" + - $"{instrumentName} should not contain wildcard selection, as it'll lead to conflict.", + $"Instrument selection criteria is invalid. Instrument name '{instrumentName}' " + + $"contains a wildcard character. This is not allowed when using a view to " + + $"rename a metric stream as it would lead to conflicting metric stream names.", nameof(instrumentName)); } @@ -97,8 +98,9 @@ public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProvid if (metricStreamConfiguration.Name != null && instrumentName.IndexOf('*') != -1) { throw new ArgumentException( - $"When view is used to provide name of metric stream, the instrument name" + - $"{instrumentName} should not contain wildcard selection, as it'll lead to conflict.", + $"Instrument selection criteria is invalid. Instrument name '{instrumentName}' " + + $"contains a wildcard character. This is not allowed when using a view to " + + $"rename a metric stream as it would lead to conflicting metric stream names.", nameof(instrumentName)); } From 1cbeb27189786d3c981f861d802b7403acd93d55 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Thu, 13 Jan 2022 16:26:40 -0800 Subject: [PATCH 3/3] remove test which is invalid with new behavior --- .../Metrics/MetricViewTests.cs | 27 ------------------- 1 file changed, 27 deletions(-) diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs index ca9ec0012bb..2d75c883ac5 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs @@ -305,33 +305,6 @@ public void ViewWithNullCustomNameTakesInstrumentName() Assert.Equal(counter1.Name, metric.Name); } - [Fact] - public void ViewToRenameMetricWildCardMatch() - { - using var meter = new Meter(Utils.GetCurrentMethodName()); - var exportedItems = new List(); - using var meterProvider = Sdk.CreateMeterProviderBuilder() - .AddMeter(meter.Name) - .AddView("counter*", "renamed") - .AddInMemoryExporter(exportedItems) - .Build(); - - // Expecting one metric stream. - var counter1 = meter.CreateCounter("counterA"); - counter1.Add(10); - var counter2 = meter.CreateCounter("counterB"); - counter2.Add(10); - var counter3 = meter.CreateCounter("counterC"); - counter3.Add(10); - meterProvider.ForceFlush(MaxTimeToAllowForFlush); - - // counter* matches all 3 instruments which all - // becomes "renamed" and only 1st one is exported. - Assert.Single(exportedItems); - var metric = exportedItems[0]; - Assert.Equal("renamed", metric.Name); - } - [Fact] public void ViewToProduceMultipleStreamsFromInstrument() {