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

Fix view bugs where views create conflicting metric streams at runtime #3129

Closed
wants to merge 16 commits into from
Closed

Fix view bugs where views create conflicting metric streams at runtime #3129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
19f4443
Add view tests
alanwest Mar 23, 2022
521fc6d
Fix up tests
alanwest Mar 23, 2022
1688884
Revise tests
alanwest Mar 23, 2022
866535e
Two instruments with view matching only one of them
alanwest Mar 23, 2022
b15fb65
Assert.Single
alanwest Mar 23, 2022
e90644d
Ignore an instrument registration if a view causes a conflict with it
alanwest Mar 23, 2022
403e572
Make most tests pass
alanwest Mar 23, 2022
ee7ab4c
Pass through identity conflict when instrument w/ no view conflicts w…
alanwest Apr 1, 2022
cfee469
Merge branch 'main' into alanwest/views-are-hard
alanwest Apr 1, 2022
b44cb72
Named parameter
alanwest Apr 1, 2022
363b98a
Use different AddView overload
alanwest Apr 1, 2022
3bf47f8
Fix log messages
alanwest Apr 1, 2022
2d47c2d
Remove incorrect log message
alanwest Apr 1, 2022
462d1af
Move check for drop configuration
alanwest Apr 1, 2022
d8efbce
Reintroduce message in correct spot
alanwest Apr 1, 2022
7a9a450
Merge branch 'main' into alanwest/views-are-hard
cijothomas Apr 2, 2022
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
9 changes: 7 additions & 2 deletions src/OpenTelemetry/Metrics/InstrumentIdentity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,15 @@ namespace OpenTelemetry.Metrics
{
private readonly int hashCode;

public InstrumentIdentity(Meter meter, string instrumentName, string unit, string description, Type instrumentType)
public InstrumentIdentity(Meter meter, string instrumentName, string unit, string description, Type instrumentType, bool derivedFromView)
{
this.MeterName = meter.Name;
this.MeterVersion = meter.Version ?? string.Empty;
this.InstrumentName = instrumentName;
this.Unit = unit ?? string.Empty;
this.Description = description ?? string.Empty;
this.InstrumentType = instrumentType;
this.DerivedFromView = derivedFromView;

unchecked
{
Expand All @@ -41,6 +42,7 @@ public InstrumentIdentity(Meter meter, string instrumentName, string unit, strin
hash = (hash * 31) + this.InstrumentName.GetHashCode();
hash = this.Unit == null ? hash : (hash * 31) + this.Unit.GetHashCode();
hash = this.Description == null ? hash : (hash * 31) + this.Description.GetHashCode();
hash = (hash * 31) + this.DerivedFromView.GetHashCode();
this.hashCode = hash;
}
}
Expand All @@ -57,6 +59,8 @@ public InstrumentIdentity(Meter meter, string instrumentName, string unit, strin

public readonly Type InstrumentType { get; }

public readonly bool DerivedFromView { get; }

public static bool operator ==(InstrumentIdentity metricIdentity1, InstrumentIdentity metricIdentity2) => metricIdentity1.Equals(metricIdentity2);

public static bool operator !=(InstrumentIdentity metricIdentity1, InstrumentIdentity metricIdentity2) => !metricIdentity1.Equals(metricIdentity2);
Expand All @@ -73,7 +77,8 @@ public bool Equals(InstrumentIdentity other)
&& this.MeterVersion == other.MeterVersion
&& this.InstrumentName == other.InstrumentName
&& this.Unit == other.Unit
&& this.Description == other.Description;
&& this.Description == other.Description
&& this.DerivedFromView == other.DerivedFromView;
}

public override readonly int GetHashCode() => this.hashCode;
Expand Down
30 changes: 21 additions & 9 deletions src/OpenTelemetry/Metrics/MetricReaderExt.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ internal Metric AddMetricWithNoViews(Instrument instrument)
var meterVersion = instrument.Meter.Version;
var metricName = instrument.Name;
var metricStreamName = $"{meterName}.{meterVersion}.{metricName}";
var instrumentIdentity = new InstrumentIdentity(instrument.Meter, metricName, instrument.Unit, instrument.Description, instrument.GetType());
var instrumentIdentity = new InstrumentIdentity(instrument.Meter, metricName, instrument.Unit, instrument.Description, instrument.GetType(), derivedFromView: false);
lock (this.instrumentCreationLock)
{
if (this.instrumentIdentityToMetric.TryGetValue(instrumentIdentity, out var existingMetric))
Expand Down Expand Up @@ -119,7 +119,7 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
var metricName = metricStreamConfig?.Name ?? instrument.Name;
var metricStreamName = $"{meterName}.{meterVersion}.{metricName}";
var metricDescription = metricStreamConfig?.Description ?? instrument.Description;
var instrumentIdentity = new InstrumentIdentity(instrument.Meter, metricName, instrument.Unit, metricDescription, instrument.GetType());
var instrumentIdentity = new InstrumentIdentity(instrument.Meter, metricName, instrument.Unit, metricDescription, instrument.GetType(), derivedFromView: metricStreamConfig != null);

if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricName))
{
Expand All @@ -132,9 +132,27 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
continue;
}

if (metricStreamConfig == MetricStreamConfiguration.Drop)
{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "View configuration asks to drop this instrument.", "Modify view configuration to allow this instrument, if desired.");
continue;
}

if (this.instrumentIdentityToMetric.TryGetValue(instrumentIdentity, out var existingMetric))
{
metrics.Add(existingMetric);
if (metrics.Count == 0)
{
metrics.Add(existingMetric);
}
else
{
OpenTelemetrySdkEventSource.Log.MetricViewIgnored(
metricName,
meterName,
"A view applied to this instrument would result in a conflicting metric stream. The view will be ignored.",
"Use MeterProviderBuilder.AddView to resolve the conflict.");
}

alanwest marked this conversation as resolved.
Show resolved Hide resolved
continue;
}

Expand All @@ -147,12 +165,6 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
"Either change the name of the instrument or use MeterProviderBuilder.AddView to resolve the conflict.");
}

if (metricStreamConfig == MetricStreamConfiguration.Drop)
{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "View configuration asks to drop this instrument.", "Modify view configuration to allow this instrument, if desired.");
continue;
}

var index = ++this.metricIndex;
if (index >= this.maxMetricStreams)
{
Expand Down
276 changes: 276 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,282 @@ long GetAggregatedValue(Metric metric)
}
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

Nit: move these tests to the MetricViewTests.cs

public void DuplicateInstrumentRegistration_WithViews_TwoIdenticalInstruments_TwoViews_SameRename()
{
var exportedItems = new List<Metric>();

using var meter = new Meter($"{Utils.GetCurrentMethodName()}");
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView((instrument) => new MetricStreamConfiguration { Name = "newname" })
.AddView((instrument) => new MetricStreamConfiguration { Name = "newname" })
.AddInMemoryExporter(exportedItems);

using var meterProvider = meterProviderBuilder.Build();

var instrument1 = meter.CreateCounter<long>("name");
var instrument2 = meter.CreateCounter<long>("name");

instrument1.Add(10);
instrument2.Add(10);

meterProvider.ForceFlush(MaxTimeToAllowForFlush);

// The second view results in a name conflict, so it is ignored
Assert.Single(exportedItems);
var metric1 = new List<Metric>() { exportedItems[0] };
Assert.Equal("newname", exportedItems[0].Name);

// Both instruments record
Assert.Equal(20, GetLongSum(metric1));
}

[Fact]
public void DuplicateInstrumentRegistration_WithViews_TwoIdenticalInstruments_TwoViews_DifferentTags()
{
var exportedItems = new List<Metric>();

using var meter = new Meter($"{Utils.GetCurrentMethodName()}");
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView((instrument) =>
{
return new MetricStreamConfiguration { TagKeys = new[] { "key1" } };
})
.AddView((instrument) =>
{
return new MetricStreamConfiguration { TagKeys = new[] { "key2" } };
})
.AddInMemoryExporter(exportedItems);

using var meterProvider = meterProviderBuilder.Build();

var instrument1 = meter.CreateCounter<long>("name");
var instrument2 = meter.CreateCounter<long>("name");

var tags = new KeyValuePair<string, object>[]
{
new("key1", "value"),
new("key2", "value"),
};

instrument1.Add(10, tags);
instrument2.Add(10, tags);

meterProvider.ForceFlush(MaxTimeToAllowForFlush);

// The second view results in a name conflict, so it is ignored
Assert.Single(exportedItems);
var metric1 = new List<Metric>() { exportedItems[0] };
var tag1 = new List<KeyValuePair<string, object>> { tags[0] };

Assert.Equal("name", exportedItems[0].Name);

// Both instruments record
Assert.Equal(20, GetLongSum(metric1));

// Only key1 is kept not key2
CheckTagsForNthMetricPoint(metric1, tag1, 1);
}

[Fact]
public void DuplicateInstrumentRegistration_WithViews_TwoIdenticalInstruments_TwoViews_SameTags()
{
var exportedItems = new List<Metric>();

using var meter = new Meter($"{Utils.GetCurrentMethodName()}");
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView((instrument) =>
{
return new MetricStreamConfiguration { TagKeys = new[] { "key1" } };
})
.AddView((instrument) =>
{
return new MetricStreamConfiguration { TagKeys = new[] { "key1" } };
})
.AddInMemoryExporter(exportedItems);

using var meterProvider = meterProviderBuilder.Build();

var instrument1 = meter.CreateCounter<long>("name");
var instrument2 = meter.CreateCounter<long>("name");

var tags = new KeyValuePair<string, object>[]
{
new("key1", "value"),
new("key2", "value"),
};

instrument1.Add(10, tags);
instrument2.Add(10, tags);

meterProvider.ForceFlush(MaxTimeToAllowForFlush);

// The second view results in a name conflict, so it is ignored
Assert.Single(exportedItems);
var metric1 = new List<Metric>() { exportedItems[0] };

var tag1 = new List<KeyValuePair<string, object>> { tags[0] };

Assert.Equal("name", exportedItems[0].Name);

// Both instruments record
Assert.Equal(20, GetLongSum(metric1));
CheckTagsForNthMetricPoint(metric1, tag1, 1);
}

[Fact]
public void DuplicateInstrumentRegistration_WithViews_TwoIdenticalInstruments_TwoViews_DifferentHistogramBounds()
{
var exportedItems = new List<Metric>();

using var meter = new Meter($"{Utils.GetCurrentMethodName()}");
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView((instrument) =>
{
return new ExplicitBucketHistogramConfiguration { Boundaries = new[] { 5.0, 10.0 } };
})
.AddView((instrument) =>
{
return new ExplicitBucketHistogramConfiguration { Boundaries = new[] { 10.0, 20.0 } };
})
.AddInMemoryExporter(exportedItems);

using var meterProvider = meterProviderBuilder.Build();

var instrument1 = meter.CreateHistogram<long>("name");
var instrument2 = meter.CreateHistogram<long>("name");

instrument1.Record(15);
instrument2.Record(15);

meterProvider.ForceFlush(MaxTimeToAllowForFlush);

// The second view results in a name conflict, so it is ignored
Assert.Single(exportedItems);
var metric1 = exportedItems[0];

Assert.Equal("name", exportedItems[0].Name);

var metricPoints = new List<MetricPoint>();
foreach (ref readonly var mp in metric1.GetMetricPoints())
{
metricPoints.Add(mp);
}

Assert.Single(metricPoints);
var metricPoint = metricPoints[0];

// Both instruments record
Assert.Equal(2, metricPoint.GetHistogramCount());
Assert.Equal(30, metricPoint.GetHistogramSum());

var index = 0;
var actualCount = 0;
var expectedBucketCounts = new long[] { 0, 0, 2 };
foreach (var histogramMeasurement in metricPoint.GetHistogramBuckets())
{
Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount);
index++;
actualCount++;
}
}

[Fact]
public void DuplicateInstrumentRegistration_WithViews_TwoInstruments_OneView_ResultsInConflict()
{
var exportedItems = new List<Metric>();

using var meter = new Meter($"{Utils.GetCurrentMethodName()}");
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView((instrument) =>
{
if (instrument.Name == "name")
{
return new MetricStreamConfiguration { Name = "othername", TagKeys = new[] { "key1" } };
}
else
{
return null;
}
})
.AddInMemoryExporter(exportedItems);

using var meterProvider = meterProviderBuilder.Build();

var instrument1 = meter.CreateCounter<long>("name");
var instrument2 = meter.CreateCounter<long>("othername");

var tags = new KeyValuePair<string, object>[]
{
new("key1", "value"),
new("key2", "value"),
};

instrument1.Add(10, tags);
instrument2.Add(10, tags);

meterProvider.ForceFlush(MaxTimeToAllowForFlush);

Assert.Equal(2, exportedItems.Count);
var metric1 = new List<Metric>() { exportedItems[0] };
var metric2 = new List<Metric>() { exportedItems[1] };

var tags1 = new List<KeyValuePair<string, object>> { tags[0] };
var tags2 = new List<KeyValuePair<string, object>> { tags[0], tags[1] };

Assert.Equal("othername", exportedItems[0].Name);
Assert.Equal("othername", exportedItems[1].Name);

Assert.Equal(10, GetLongSum(metric1));
Assert.Equal(10, GetLongSum(metric2));

CheckTagsForNthMetricPoint(metric1, tags1, 1);
CheckTagsForNthMetricPoint(metric2, tags2, 1);
}

[Fact]
public void DuplicateInstrumentRegistration_WithViews_TwoInstruments_WouldResultInConflictButSecondInstrumentIsDropped()
{
var exportedItems = new List<Metric>();

using var meter = new Meter($"{Utils.GetCurrentMethodName()}");
var meterProviderBuilder = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView((instrument) =>
{
if (instrument.Name == "name")
{
return new MetricStreamConfiguration { Name = "othername" };
}
else
{
return MetricStreamConfiguration.Drop;
}
})
.AddInMemoryExporter(exportedItems);

using var meterProvider = meterProviderBuilder.Build();

var instrument1 = meter.CreateCounter<long>("name");
var instrument2 = meter.CreateCounter<long>("othername");

instrument1.Add(10);
instrument2.Add(20);

meterProvider.ForceFlush(MaxTimeToAllowForFlush);

Assert.Single(exportedItems);
var metric1 = new List<Metric>() { exportedItems[0] };

Assert.Equal("othername", exportedItems[0].Name);
Assert.Equal(10, GetLongSum(metric1));
}

[Fact]
public void DuplicateInstrumentNamesFromDifferentMetersWithSameNameDifferentVersion()
{
Expand Down