diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 256896d3c98..92bd21e51bd 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -25,6 +25,10 @@ Released 2021-Oct-08 * TracerProviderBuilder.AddLegacySource now supports wildcard activity names. ([#2183](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2183)) +* Instrument and View names are validated + [according with the spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument). + ([#2470](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2470)) + ## 1.2.0-alpha4 Released 2021-Sep-23 diff --git a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs index f7c75873016..dce53e8b32b 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs @@ -52,6 +52,11 @@ public static MeterProviderBuilder AddReader(this MeterProviderBuilder meterProv /// See View specification here : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#view. public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProviderBuilder, string instrumentName, string name) { + if (!MeterProviderBuilderSdk.IsValidInstrumentName(name)) + { + throw new ArgumentException($"Custom view name {name} is invalid.", nameof(name)); + } + if (meterProviderBuilder is MeterProviderBuilderBase meterProviderBuilderBase) { return meterProviderBuilderBase.AddView(instrumentName, name); @@ -71,6 +76,16 @@ public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProvid /// See View specification here : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#view. public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProviderBuilder, string instrumentName, MetricStreamConfiguration metricStreamConfiguration) { + if (metricStreamConfiguration == null) + { + throw new ArgumentNullException($"Metric stream configuration cannot be null.", nameof(metricStreamConfiguration)); + } + + if (!MeterProviderBuilderSdk.IsValidViewName(metricStreamConfiguration.Name)) + { + throw new ArgumentException($"Custom view name {metricStreamConfiguration.Name} is invalid.", nameof(metricStreamConfiguration.Name)); + } + if (meterProviderBuilder is MeterProviderBuilderBase meterProviderBuilderBase) { return meterProviderBuilderBase.AddView(instrumentName, metricStreamConfiguration); diff --git a/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs index 50572f0849b..47bd282cd96 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs @@ -14,10 +14,48 @@ // limitations under the License. // +using System.Text.RegularExpressions; + namespace OpenTelemetry.Metrics { internal class MeterProviderBuilderSdk : MeterProviderBuilderBase { + private static readonly Regex InstrumentNameRegex = new Regex( + @"^[a-zA-Z][-.\w]{0,62}$", RegexOptions.IgnoreCase | RegexOptions.Compiled); + + /// + /// Returns whether the given instrument name is valid according to the specification. + /// + /// See specification: . + /// The instrument name. + /// Boolean indicating if the instrument is valid. + internal static bool IsValidInstrumentName(string instrumentName) + { + if (string.IsNullOrWhiteSpace(instrumentName)) + { + return false; + } + + return InstrumentNameRegex.IsMatch(instrumentName); + } + + /// + /// Returns whether the given custom view name is valid according to the specification. + /// + /// See specification: . + /// The view name. + /// Boolean indicating if the instrument is valid. + internal static bool IsValidViewName(string customViewName) + { + // Only validate the view name in case it's not null. In case it's null, the view name will be the instrument name as per the spec. + if (customViewName == null) + { + return true; + } + + return InstrumentNameRegex.IsMatch(customViewName); + } + internal MeterProvider BuildSdk() => this.Build(); } } diff --git a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs index 6a3ab691e02..d39eec328fa 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs @@ -152,6 +152,18 @@ internal MeterProviderSdk( { var metricStreamConfig = metricStreamConfigs[i]; var metricStreamName = metricStreamConfig?.Name ?? instrument.Name; + + if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricStreamName)) + { + OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored( + metricStreamName, + instrument.Meter.Name, + "Metric name is invalid.", + "The name must comply with the OpenTelemetry specification."); + + continue; + } + if (this.metricStreamNames.Contains(metricStreamName)) { // TODO: Log that instrument is ignored @@ -218,6 +230,17 @@ internal MeterProviderSdk( try { + if (!MeterProviderBuilderSdk.IsValidInstrumentName(instrument.Name)) + { + OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored( + instrument.Name, + instrument.Meter.Name, + "Instrument name is invalid.", + "The name must comply with the OpenTelemetry specification"); + + return; + } + var metricName = instrument.Name; Metric metric = null; lock (this.instrumentCreationLock) diff --git a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs index 08d45e032d4..466d7d8a26e 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs @@ -599,6 +599,51 @@ void ProcessExport(Batch batch) Assert.True(difference <= 0.0001); } + [Theory] + [MemberData(nameof(MetricsTestData.InvalidInstrumentNames), MemberType = typeof(MetricsTestData))] + public void InstrumentWithInvalidNameIsIgnoredTest(string instrumentName) + { + var exportedItems = new List(); + + using var meter = new Meter("InstrumentWithInvalidNameIsIgnoredTest"); + + using var meterProvider = Sdk.CreateMeterProviderBuilder() + .AddMeter(meter.Name) + .AddInMemoryExporter(exportedItems) + .Build(); + + var counterLong = meter.CreateCounter(instrumentName); + counterLong.Add(10); + meterProvider.ForceFlush(MaxTimeToAllowForFlush); + + // instrument should have been ignored + // as its name does not comply with the specification + Assert.Empty(exportedItems); + } + + [Theory] + [MemberData(nameof(MetricsTestData.ValidInstrumentNames), MemberType = typeof(MetricsTestData))] + public void InstrumentWithValidNameIsExportedTest(string name) + { + var exportedItems = new List(); + + using var meter = new Meter("InstrumentValidNameIsExportedTest"); + + using var meterProvider = Sdk.CreateMeterProviderBuilder() + .AddMeter(meter.Name) + .AddInMemoryExporter(exportedItems) + .Build(); + + var counterLong = meter.CreateCounter(name); + counterLong.Add(10); + meterProvider.ForceFlush(MaxTimeToAllowForFlush); + + // Expecting one metric stream. + Assert.Single(exportedItems); + var metric = exportedItems[0]; + Assert.Equal(name, metric.Name); + } + private static long GetLongSum(List metrics) { long sum = 0; diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs index c426e84b6c9..cefed232b19 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs @@ -53,12 +53,76 @@ public void ViewToRenameMetric() Assert.Equal("renamed", metric.Name); } + [Theory] + [MemberData(nameof(MetricsTestData.InvalidInstrumentNames), MemberType = typeof(MetricsTestData))] + public void AddViewWithInvalidNameThrowsArgumentException(string viewNewName) + { + var exportedItems = new List(); + + using var meter1 = new Meter("AddViewWithInvalidNameThrowsArgumentException"); + + var ex = Assert.Throws(() => Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + .AddView("name1", viewNewName) + .AddInMemoryExporter(exportedItems) + .Build()); + + Assert.Contains($"Custom view name {viewNewName} is invalid.", ex.Message); + + ex = Assert.Throws(() => Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + .AddView("name1", new MetricStreamConfiguration { Name = viewNewName }) + .AddInMemoryExporter(exportedItems) + .Build()); + + Assert.Contains($"Custom view name {viewNewName} is invalid.", ex.Message); + } + + [Fact] + public void AddViewWithNullMetricStreamConfigurationThrowsArgumentnullException() + { + var exportedItems = new List(); + + using var meter1 = new Meter("AddViewWithInvalidNameThrowsArgumentException"); + + Assert.Throws(() => Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + .AddView("name1", (MetricStreamConfiguration)null) + .AddInMemoryExporter(exportedItems) + .Build()); + } + + [Theory] + [MemberData(nameof(MetricsTestData.ValidInstrumentNames), MemberType = typeof(MetricsTestData))] + public void ViewWithValidNameExported(string viewNewName) + { + var exportedItems = new List(); + + using var meter1 = new Meter("ViewWithInvalidNameIgnoredTest"); + using var meterProvider = Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + .AddView("name1", viewNewName) + .AddInMemoryExporter(exportedItems) + .Build(); + + var counterLong = meter1.CreateCounter("name1"); + counterLong.Add(10); + meterProvider.ForceFlush(MaxTimeToAllowForFlush); + + // Expecting one metric stream. + Assert.Single(exportedItems); + var metric = exportedItems[0]; + Assert.Equal(viewNewName, metric.Name); + } + [Fact] public void ViewToRenameMetricConditionally() { using var meter1 = new Meter("ViewToRenameMetricConditionallyTest"); using var meter2 = new Meter("ViewToRenameMetricConditionallyTest2"); + var exportedItems = new List(); + using var meterProvider = Sdk.CreateMeterProviderBuilder() .AddMeter(meter1.Name) .AddMeter(meter2.Name) @@ -93,6 +157,116 @@ public void ViewToRenameMetricConditionally() Assert.Equal("new description", exportedItems[1].Description); } + [Theory] + [MemberData(nameof(MetricsTestData.InvalidInstrumentNames), MemberType = typeof(MetricsTestData))] + public void ViewWithInvalidNameIgnoredConditionally(string viewNewName) + { + using var meter1 = new Meter("ViewToRenameMetricConditionallyTest"); + var exportedItems = new List(); + using var meterProvider = Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + + // since here it's a func, we can't validate the name right away + // so the view is allowed to be added, but upon instrument creation it's going to be ignored. + .AddView((instrument) => + { + if (instrument.Meter.Name.Equals(meter1.Name, StringComparison.OrdinalIgnoreCase) + && instrument.Name.Equals("name1", StringComparison.OrdinalIgnoreCase)) + { + // invalid instrument name as per the spec + return new MetricStreamConfiguration() { Name = viewNewName, Description = "new description" }; + } + else + { + return null; + } + }) + .AddInMemoryExporter(exportedItems) + .Build(); + + // We should expect 1 metric here, + // but because the MetricStreamName passed is invalid, the instrument is ignored + var counter1 = meter1.CreateCounter("name1", "unit", "original_description"); + counter1.Add(10); + + meterProvider.ForceFlush(MaxTimeToAllowForFlush); + + Assert.Empty(exportedItems); + } + + [Theory] + [MemberData(nameof(MetricsTestData.ValidInstrumentNames), MemberType = typeof(MetricsTestData))] + public void ViewWithValidNameConditionally(string viewNewName) + { + using var meter1 = new Meter("ViewToRenameMetricConditionallyTest"); + var exportedItems = new List(); + using var meterProvider = Sdk.CreateMeterProviderBuilder() + .AddMeter(meter1.Name) + .AddView((instrument) => + { + if (instrument.Meter.Name.Equals(meter1.Name, StringComparison.OrdinalIgnoreCase) + && instrument.Name.Equals("name1", StringComparison.OrdinalIgnoreCase)) + { + // invalid instrument name as per the spec + return new MetricStreamConfiguration() { Name = viewNewName, Description = "new description" }; + } + else + { + return null; + } + }) + .AddInMemoryExporter(exportedItems) + .Build(); + + // Expecting one metric stream. + var counter1 = meter1.CreateCounter("name1", "unit", "original_description"); + counter1.Add(10); + + meterProvider.ForceFlush(MaxTimeToAllowForFlush); + + // Expecting one metric stream. + Assert.Single(exportedItems); + var metric = exportedItems[0]; + Assert.Equal(viewNewName, metric.Name); + } + + [Fact] + public void ViewWithNullCustomNameTakesInstrumentName() + { + var exportedItems = new List(); + + using var meter = new Meter("ViewToRenameMetricConditionallyTest"); + + using var meterProvider = Sdk.CreateMeterProviderBuilder() + .AddMeter(meter.Name) + .AddView((instrument) => + { + if (instrument.Name.Equals("name1", StringComparison.OrdinalIgnoreCase)) + { + // null View name + return new MetricStreamConfiguration() { }; + } + else + { + return null; + } + }) + .AddInMemoryExporter(exportedItems) + .Build(); + + // Expecting one metric stream. + // Since the View name was null, the instrument name was used instead + var counter1 = meter.CreateCounter("name1", "unit", "original_description"); + counter1.Add(10); + + meterProvider.ForceFlush(MaxTimeToAllowForFlush); + + // Expecting one metric stream. + Assert.Single(exportedItems); + var metric = exportedItems[0]; + Assert.Equal(counter1.Name, metric.Name); + } + [Fact] public void ViewToRenameMetricWildCardMatch() { diff --git a/test/OpenTelemetry.Tests/Metrics/MetricsTestData.cs b/test/OpenTelemetry.Tests/Metrics/MetricsTestData.cs new file mode 100644 index 00000000000..5450eaa7ab3 --- /dev/null +++ b/test/OpenTelemetry.Tests/Metrics/MetricsTestData.cs @@ -0,0 +1,44 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +using System.Collections.Generic; + +namespace OpenTelemetry.Metrics.Tests +{ + public class MetricsTestData + { + public static IEnumerable InvalidInstrumentNames + => new List + { + new object[] { " " }, + new object[] { "-first-char-not-alphabetic" }, + new object[] { "1first-char-not-alphabetic" }, + new object[] { "invalid+separator" }, + new object[] { new string('m', 64) }, + }; + + public static IEnumerable ValidInstrumentNames + => new List + { + new object[] { "m" }, + new object[] { "first-char-alphabetic" }, + new object[] { "my-2-instrument" }, + new object[] { "my.metric" }, + new object[] { "my_metric2" }, + new object[] { new string('m', 63) }, + }; + } +}