From af86f5ee6bdb4ff6c99c9942b3d215ca633a9f3e Mon Sep 17 00:00:00 2001 From: Joao Grassi Date: Sat, 9 Oct 2021 18:02:50 +0200 Subject: [PATCH] PR Suggestions --- .../Metrics/MeterProviderBuilderExtensions.cs | 4 +-- .../Metrics/MeterProviderBuilderSdk.cs | 8 +++--- src/OpenTelemetry/Metrics/MeterProviderSdk.cs | 26 +++++++++++++------ 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs index 4e14eb3b276..98ff6fb6f70 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs @@ -53,7 +53,7 @@ public static MeterProviderBuilder AddReader(this MeterProviderBuilder meterProv 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.IsViewNameValid(name)) + if (!string.IsNullOrWhiteSpace(name) && !MeterProviderBuilderSdk.IsValidViewName(name)) { throw new ArgumentException($"Custom view name {name} is invalid.", nameof(name)); } @@ -78,7 +78,7 @@ public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProvid 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.IsViewNameValid(metricStreamConfiguration.Name)) + if (!string.IsNullOrWhiteSpace(metricStreamConfiguration.Name) && !MeterProviderBuilderSdk.IsValidViewName(metricStreamConfiguration.Name)) { throw new ArgumentException($"Custom view name {metricStreamConfiguration.Name} is invalid.", nameof(metricStreamConfiguration.Name)); } diff --git a/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs index 58d993df746..6d655578cc6 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs @@ -24,12 +24,12 @@ internal class MeterProviderBuilderSdk : MeterProviderBuilderBase @"^[a-zA-Z][-.\w]{0,62}$", RegexOptions.IgnoreCase | RegexOptions.Compiled); /// - /// Returns whether the given instrument name is valid according with the specification. + /// 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 IsInstrumentNameValid(string instrumentName) + internal static bool IsValidInstrumentName(string instrumentName) { if (string.IsNullOrWhiteSpace(instrumentName)) { @@ -40,12 +40,12 @@ internal static bool IsInstrumentNameValid(string instrumentName) } /// - /// Returns whether the given custom view name is valid according with the specification. + /// 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 IsViewNameValid(string customViewName) + internal static bool IsValidViewName(string customViewName) { return InstrumentNameRegex.IsMatch(customViewName); } diff --git a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs index 9777564ecb9..dfbc20a520d 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs @@ -139,9 +139,14 @@ internal MeterProviderSdk( var metricStreamConfig = metricStreamConfigs[i]; var metricStreamName = metricStreamConfig?.Name ?? instrument.Name; - if (!MeterProviderBuilderSdk.IsInstrumentNameValid(metricStreamName)) + if (!MeterProviderBuilderSdk.IsValidInstrumentName(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"); + OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored( + instrument.Name, + instrument.Meter.Name, + "Metric name is invalid.", + "The name must comply with the OpenTelemetry specification."); + continue; } @@ -206,14 +211,19 @@ internal MeterProviderSdk( { try { - if (!MeterProviderBuilderSdk.IsInstrumentNameValid(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)) { + 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)