Add validation to instrument and view names

open-telemetry · Oct 8, 2021 · a456557 · a456557
1 parent db8f0e7
commit a456557
Show file tree

Hide file tree

Showing 7 changed files with 284 additions and 0 deletions.
diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -11,6 +11,10 @@
 * 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
@@ -52,6 +52,12 @@ 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)
         {
+            // we only need to validate the custom view name in case something was actually provided
+            if (!string.IsNullOrWhiteSpace(name) && !MeterProviderBuilderSdk.ViewNameValid(name))
+            {
+                throw new ArgumentException($"Custom view name {name} is invalid.", nameof(name));
+            }
+
             if (meterProviderBuilder is MeterProviderBuilderBase meterProviderBuilderBase)
             {
                 return meterProviderBuilderBase.AddView(instrumentName, name);
@@ -71,6 +77,12 @@ 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)
         {
+            // we only need to validate the custom view name in case something was actually provided
+            if (!string.IsNullOrWhiteSpace(metricStreamConfiguration.Name) && !MeterProviderBuilderSdk.ViewNameValid(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
@@ -14,10 +14,42 @@
 // 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 with 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 InstrumentNameValid(string instrumentName)
+        {
+            if (string.IsNullOrWhiteSpace(instrumentName))
+            {
+                return false;
+            }
+
+            return InstrumentNameRegex.IsMatch(instrumentName);
+        }
+
+        /// <summary>
+        /// Returns whether the given custom view name is valid according with 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 ViewNameValid(string customViewName)
+        {
+            return InstrumentNameRegex.IsMatch(customViewName);
+        }
+
         internal MeterProvider BuildSdk() => this.Build();
     }
 }
diff --git a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs
@@ -19,6 +19,7 @@
 using System.Diagnostics;
 using System.Diagnostics.Metrics;
 using System.Linq;
+using System.Text.RegularExpressions;
 using OpenTelemetry.Internal;
 using OpenTelemetry.Resources;
 
@@ -138,6 +139,13 @@ internal MeterProviderSdk(
                             {
                                 var metricStreamConfig = metricStreamConfigs[i];
                                 var metricStreamName = metricStreamConfig?.Name ?? instrument.Name;
+
+                                if (!MeterProviderBuilderSdk.InstrumentNameValid(metricStreamName))
+                                {
+                                    OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(instrument.Name, instrument.Meter.Name, "Metric name is invalid.", "The name must comply with the OpenTelemetry specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument");
+                                    continue;
+                                }
+
                                 if (this.metricStreamNames.ContainsKey(metricStreamName))
                                 {
                                     // TODO: Log that instrument is ignored
@@ -199,6 +207,12 @@ internal MeterProviderSdk(
                 {
                     try
                     {
+                        if (!MeterProviderBuilderSdk.InstrumentNameValid(instrument.Name))
+                        {
+                            OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(instrument.Name, instrument.Meter.Name, "Metric name is invalid.", "The name must comply with the OpenTelemetry specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument");
+                            return;
+                        }
+
                         if (meterSourcesToSubscribe.ContainsKey(instrument.Meter.Name))
                         {
                             var metricName = instrument.Name;

diff --git a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs
@@ -503,6 +503,51 @@ void ProcessExport(Batch<Metric> batch)
             Assert.True(difference <= 0.0001);
         }
 
+        [Theory]
+        [MemberData(nameof(MetricsTestData.InvalidInstrumentNames), MemberType = typeof(MetricsTestData))]
+        public void InstrumentWithInvalidNameIsIgnoredTest(string name)
+        {
+            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>(name);
+            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;

diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs
@@ -53,12 +53,62 @@ public void ViewToRenameMetric()
             Assert.Equal("renamed", metric.Name);
         }
 
+        [Theory]
+        [MemberData(nameof(MetricsTestData.InvalidViewNames), 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);
+        }
+
+        [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 +143,80 @@ 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 export 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 view 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();
+
+            // We should expect 1 metric here,
+            // but because the MetricStreamName passed is invalid, the view is ignored
+            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 ViewToRenameMetricWildCardMatch()
         {

diff --git a/test/OpenTelemetry.Tests/Metrics/MetricsTestData.cs b/test/OpenTelemetry.Tests/Metrics/MetricsTestData.cs
@@ -0,0 +1,53 @@
+// <copyright file="MetricsTestData.cs" company="OpenTelemetry Authors">
+// 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.
+// </copyright>
+
+using System.Collections.Generic;
+
+namespace OpenTelemetry.Metrics.Tests
+{
+    public class MetricsTestData
+    {
+        public static IEnumerable<object[]> InvalidInstrumentNames
+           => new List<object[]>
+           {
+                    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<object[]> InvalidViewNames
+           => new List<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<object[]> ValidInstrumentNames
+           => new List<object[]>
+           {
+                    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) },
+           };
+    }
+}