Add validation to instrument and view name

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

src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs

@@ -52,6 +52,11 @@ public static MeterProviderBuilder AddReader(this MeterProviderBuilder meterProv
         /// <remarks>See View specification here : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#view.</remarks>
         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
         /// <remarks>See View specification here : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#view.</remarks>
         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);

src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs

@@ -14,10 +14,48 @@
 // limitations under the License.
 // </copyright>
 
+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);
+
+        /// <summary>
+        /// Returns whether the given instrument name is valid according to the specification.
+        /// </summary>
+        /// <remarks>See specification: <see href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument"/>.</remarks>
+        /// <param name="instrumentName">The instrument name.</param>
+        /// <returns>Boolean indicating if the instrument is valid.</returns>
+        internal static bool IsValidInstrumentName(string instrumentName)
+        {
+            if (string.IsNullOrWhiteSpace(instrumentName))
+            {
+                return false;
+            }
+
+            return InstrumentNameRegex.IsMatch(instrumentName);
+        }
+
+        /// <summary>
+        /// Returns whether the given custom view name is valid according to the specification.
+        /// </summary>
+        /// <remarks>See specification: <see href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument"/>.</remarks>
+        /// <param name="customViewName">The view name.</param>
+        /// <returns>Boolean indicating if the instrument is valid.</returns>
+        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();
     }
 }

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)

test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs

@@ -599,6 +599,51 @@ void ProcessExport(Batch<Metric> batch)
             Assert.True(difference <= 0.0001);
         }
 
+        [Theory]
+        [MemberData(nameof(MetricsTestData.InvalidInstrumentNames), MemberType = typeof(MetricsTestData))]
+        public void InstrumentWithInvalidNameIsIgnoredTest(string instrumentName)
+        {
+            var exportedItems = new List<Metric>();
+
+            using var meter = new Meter("InstrumentWithInvalidNameIsIgnoredTest");
+
+            using var meterProvider = Sdk.CreateMeterProviderBuilder()
+                .AddMeter(meter.Name)
+                .AddInMemoryExporter(exportedItems)
+                .Build();
+
+            var counterLong = meter.CreateCounter<long>(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<Metric>();
+
+            using var meter = new Meter("InstrumentValidNameIsExportedTest");
+
+            using var meterProvider = Sdk.CreateMeterProviderBuilder()
+                .AddMeter(meter.Name)
+                .AddInMemoryExporter(exportedItems)
+                .Build();
+
+            var counterLong = meter.CreateCounter<long>(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<Metric> metrics)
         {
             long sum = 0;

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<Metric>();
+
+            using var meter1 = new Meter("AddViewWithInvalidNameThrowsArgumentException");
+
+            var ex = Assert.Throws<ArgumentException>(() => Sdk.CreateMeterProviderBuilder()
+                .AddMeter(meter1.Name)
+                .AddView("name1", viewNewName)
+                .AddInMemoryExporter(exportedItems)
+                .Build());
+
+            Assert.Contains($"Custom view name {viewNewName} is invalid.", ex.Message);
+
+            ex = Assert.Throws<ArgumentException>(() => 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<Metric>();
+
+            using var meter1 = new Meter("AddViewWithInvalidNameThrowsArgumentException");
+
+            Assert.Throws<ArgumentNullException>(() => 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<Metric>();
+
+            using var meter1 = new Meter("ViewWithInvalidNameIgnoredTest");
+            using var meterProvider = Sdk.CreateMeterProviderBuilder()
+                .AddMeter(meter1.Name)
+                .AddView("name1", viewNewName)
+                .AddInMemoryExporter(exportedItems)
+                .Build();
+
+            var counterLong = meter1.CreateCounter<long>("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<Metric>();
+
             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<Metric>();
+            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<long>("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<Metric>();
+            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<long>("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<Metric>();
+
+            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<long>("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()
         {