Skip to content

Commit

Permalink
Add validation to instrument and view name (#2470)
Browse files Browse the repository at this point in the history
  • Loading branch information
@joaopgrassi
joaopgrassi authored Nov 2, 2021
1 parent d9b2ea4 commit 1d16b0a
Show file tree
Hide file tree
Showing 7 changed files with 343 additions and 0 deletions.
4 changes: 4 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
38 changes: 38 additions & 0 deletions src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
23 changes: 23 additions & 0 deletions src/OpenTelemetry/Metrics/MeterProviderSdk.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
45 changes: 45 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
174 changes: 174 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()
{
Expand Down
Loading

0 comments on commit 1d16b0a

Please sign in to comment.