diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 05cd8c96315..9c02cd74ea1 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -15,6 +15,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..98ff6fb6f70 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs @@ -52,6 +52,12 @@ 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) { + // we only need to validate the custom view name in case something was actually provided + if (!string.IsNullOrWhiteSpace(name) && !MeterProviderBuilderSdk.IsValidViewName(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 /// 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) { + // we only need to validate the custom view name in case something was actually provided + if (!string.IsNullOrWhiteSpace(metricStreamConfiguration.Name) && !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..6d655578cc6 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs @@ -14,10 +14,42 @@ // 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) + { + return InstrumentNameRegex.IsMatch(customViewName); + } + internal MeterProvider BuildSdk() => this.Build(); } } diff --git a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs index e11a75697e8..eb31c395b18 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs @@ -153,6 +153,18 @@ internal MeterProviderSdk( { var metricStreamConfig = metricStreamConfigs[i]; var metricStreamName = metricStreamConfig?.Name ?? instrument.Name; + + if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricStreamName)) + { + OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored( + instrument.Name, + instrument.Meter.Name, + "Metric name is invalid.", + "The name must comply with the OpenTelemetry specification."); + + continue; + } + if (this.metricStreamNames.ContainsKey(metricStreamName)) { // TODO: Log that instrument is ignored @@ -219,6 +231,17 @@ internal MeterProviderSdk( try { + if (!MeterProviderBuilderSdk.IsValidInstrumentName(instrument.Name)) + { + OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored( + instrument.Name, + instrument.Meter.Name, + "Metric 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 73dcb3df392..d12f84ec616 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs @@ -586,6 +586,51 @@ void ProcessExport(Batch batch) Assert.True(difference <= 0.0001); } + [Theory] + [MemberData(nameof(MetricsTestData.InvalidInstrumentNames), MemberType = typeof(MetricsTestData))] + public void InstrumentWithInvalidNameIsIgnoredTest(string name) + { + 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(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(); + + 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 7a825252513..493563f406d 100644 --- 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(); + + 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); + } + + [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 +143,80 @@ public void ViewToRenameMetricConditionally() Assert.Equal("new description", exportedItems[1].Description); } + [Theory] + [MemberData(nameof(MetricsTestData.InvalidViewNames), 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 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("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(); + + // We should expect 1 metric here, + // but because the MetricStreamName passed is invalid, the view is ignored + 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 ViewToRenameMetricWildCardMatch() { diff --git a/test/OpenTelemetry.Tests/Metrics/MetricsTestData.cs b/test/OpenTelemetry.Tests/Metrics/MetricsTestData.cs new file mode 100644 index 00000000000..d248cac8b56 --- /dev/null +++ b/test/OpenTelemetry.Tests/Metrics/MetricsTestData.cs @@ -0,0 +1,53 @@ +// +// 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 InvalidViewNames + => new List + { + 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) }, + }; + } +}