Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow duplicate metric name when meter is different #2634

Merged
merged 7 commits into from
Nov 18, 2021
Merged

Allow duplicate metric name when meter is different #2634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
14d6f22
Allow for metric to have the same name when they are from different m…
alanwest Nov 17, 2021
c9c193b
Update changelog
alanwest Nov 17, 2021
e1ae5d0
Fix changelog
alanwest Nov 17, 2021
c60b71e
Update src/OpenTelemetry/CHANGELOG.md
alanwest Nov 17, 2021
0507f3f
Split into two tests
alanwest Nov 18, 2021
aff87a3
Merge branch 'alanwest/allow-same-metric-name-different-meter' of git…
alanwest Nov 18, 2021
66c9fcd
Merge branch 'main' into alanwest/allow-same-metric-name-different-meter
cijothomas Nov 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Metrics with the same name but from different meters are allowed.
([#2634](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2634))

* Metrics SDK will not provide inactive Metrics to delta exporter.
([#2629](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2629))

Expand Down
16 changes: 10 additions & 6 deletions src/OpenTelemetry/Metrics/MeterProviderSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,14 @@ internal MeterProviderSdk(
for (int i = 0; i < maxCountMetricsToBeCreated; i++)
{
var metricStreamConfig = metricStreamConfigs[i];
var metricStreamName = metricStreamConfig?.Name ?? instrument.Name;
var meterName = instrument.Meter.Name;
var metricName = metricStreamConfig?.Name ?? instrument.Name;
var metricStreamName = $"{meterName}.{metricName}";

if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricStreamName))
if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be metricStreamName still? As it is now, it again uses the instrument name, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do want to use the instrument name as the validation check is only for the instrument name. metricStreamName is now used to ensure the uniqueness of the combination of meter name and instrument name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍, thanks for tackling this so quickly!

{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(
metricStreamName,
metricName,
instrument.Meter.Name,
"Metric name is invalid.",
"The name must comply with the OpenTelemetry specification.");
Expand Down Expand Up @@ -195,7 +197,7 @@ internal MeterProviderSdk(
string[] tagKeysInteresting = metricStreamConfig?.TagKeys;
double[] histogramBucketBounds = (metricStreamConfig is HistogramConfiguration histogramConfig
&& histogramConfig.BucketBounds != null) ? histogramConfig.BucketBounds : null;
metric = new Metric(instrument, temporality, metricStreamName, metricDescription, histogramBucketBounds, tagKeysInteresting);
metric = new Metric(instrument, temporality, metricName, metricDescription, histogramBucketBounds, tagKeysInteresting);

this.metrics[index] = metric;
metrics.Add(metric);
Expand Down Expand Up @@ -245,11 +247,13 @@ internal MeterProviderSdk(
return;
}

var meterName = instrument.Meter.Name;
var metricName = instrument.Name;
var metricStreamName = $"{meterName}.{metricName}";
Metric metric = null;
lock (this.instrumentCreationLock)
{
if (this.metricStreamNames.Contains(metricName))
if (this.metricStreamNames.Contains(metricStreamName))
{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "Metric name conflicting with existing name.", "Either change the name of the instrument or change name using View.");
return;
Expand All @@ -265,7 +269,7 @@ internal MeterProviderSdk(
{
metric = new Metric(instrument, temporality, metricName, instrument.Description);
this.metrics[index] = metric;
this.metricStreamNames.Add(metricName);
this.metricStreamNames.Add(metricStreamName);
}
}

Expand Down
72 changes: 56 additions & 16 deletions test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,11 @@ public void ObserverCallbackExceptionTest()
}

[Theory]
[InlineData(AggregationTemporality.Cumulative)]
[InlineData(AggregationTemporality.Delta)]
public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality temporality)
[InlineData(AggregationTemporality.Cumulative, true)]
[InlineData(AggregationTemporality.Cumulative, false)]
[InlineData(AggregationTemporality.Delta, true)]
[InlineData(AggregationTemporality.Delta, false)]
public void DuplicateInstrumentNamesFromSameMeterAreNotAllowed(AggregationTemporality temporality, bool hasView)
{
var metricItems = new List<Metric>();
var metricExporter = new InMemoryExporter<Metric>(metricItems);
Expand All @@ -113,39 +115,77 @@ public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality tempor
{
PreferredAggregationTemporality = temporality,
};
using var meter1 = new Meter($"{Utils.GetCurrentMethodName()}.1.{temporality}");
using var meter2 = new Meter($"{Utils.GetCurrentMethodName()}.2.{temporality}");
using var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter1.Name)
.AddMeter(meter2.Name)
.AddReader(metricReader)
.Build();
using var meter = new Meter($"{Utils.GetCurrentMethodName()}.{temporality}");
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddReader(metricReader);

if (hasView)
{
meterProviderBuilder.AddView("name1", new MetricStreamConfiguration() { Description = "description" });
}

using var meterProvider = meterProviderBuilder.Build();

// Expecting one metric stream.
var counterLong = meter1.CreateCounter<long>("name1");
var counterLong = meter.CreateCounter<long>("name1");
counterLong.Add(10);
metricReader.Collect();
Assert.Single(metricItems);

// The following will be ignored as
// metric of same name exists.
// Metric stream will remain one.
var anotherCounterSameName = meter1.CreateCounter<long>("name1");
var anotherCounterSameName = meter.CreateCounter<long>("name1");
anotherCounterSameName.Add(10);
counterLong.Add(10);
metricItems.Clear();
metricReader.Collect();
Assert.Single(metricItems);
}

[Theory]
[InlineData(AggregationTemporality.Cumulative, true)]
[InlineData(AggregationTemporality.Cumulative, false)]
[InlineData(AggregationTemporality.Delta, true)]
[InlineData(AggregationTemporality.Delta, false)]
public void DuplicateInstrumentNamesFromDifferentMetersAreAllowed(AggregationTemporality temporality, bool hasView)
{
var metricItems = new List<Metric>();
var metricExporter = new InMemoryExporter<Metric>(metricItems);

// The following will also be ignored
// as the name is same.
// (the Meter name is not part of stream name)
var metricReader = new BaseExportingMetricReader(metricExporter)
{
PreferredAggregationTemporality = temporality,
};
using var meter1 = new Meter($"{Utils.GetCurrentMethodName()}.1.{temporality}");
using var meter2 = new Meter($"{Utils.GetCurrentMethodName()}.2.{temporality}");
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter1.Name)
.AddMeter(meter2.Name)
.AddReader(metricReader);

if (hasView)
{
meterProviderBuilder.AddView("name1", new MetricStreamConfiguration() { Description = "description" });
}

using var meterProvider = meterProviderBuilder.Build();

// Expecting one metric stream.
var counterLong = meter1.CreateCounter<long>("name1");
counterLong.Add(10);
metricReader.Collect();
Assert.Single(metricItems);

// The following will not be ignored
// as it is the same metric name but different meter.
var anotherCounterSameNameDiffMeter = meter2.CreateCounter<long>("name1");
anotherCounterSameNameDiffMeter.Add(10);
counterLong.Add(10);
metricItems.Clear();
metricReader.Collect();
Assert.Single(metricItems);
Assert.Equal(2, metricItems.Count);
}

[Theory]
Expand Down