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 9 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
18 changes: 10 additions & 8 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(), false);
alanwest marked this conversation as resolved.
Show resolved Hide resolved
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(), metricStreamConfig != null);
alanwest marked this conversation as resolved.
Show resolved Hide resolved

if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricName))
{
Expand All @@ -133,20 +133,22 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
}

if (this.instrumentIdentityToMetric.TryGetValue(instrumentIdentity, out var existingMetric))
{
metrics.Add(existingMetric);
continue;
}

if (this.metricStreamNames.Contains(metricStreamName))
Copy link
Member

Choose a reason for hiding this comment

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

we still need this check, and warning, for the following test

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

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

            using var meterProvider = meterProviderBuilder.Build();

            var instrument1 = meter.CreateCounter<long>("name", description: "desc1");
            var instrument2 = meter.CreateCounter<long>("name", description: "desc2");

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

            meterProvider.ForceFlush(MaxTimeToAllowForFlush);

            Assert.Equal(2, exportedItems.Count);
            Assert.Equal("name", exportedItems[0].Name);
            Assert.Equal("desc1", exportedItems[0].Description);
            Assert.Equal("name", exportedItems[1].Name);
            Assert.Equal("desc2", exportedItems[1].Description);
        }

{
OpenTelemetrySdkEventSource.Log.DuplicateMetricInstrument(
Copy link
Member

Choose a reason for hiding this comment

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

This log must only be inside the if (metrics.Count==0) part.

Copy link
Member Author

@alanwest alanwest Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

On second thought, I removed this log message. I think it is unnecessary. It is not an issue of a duplicate conflicting metric instrument. The if (metrics.Count == 0) case is meant to handle instruments mapping to identical metric streams (i.e., same name, kind, description, etc) - so it is safe to aggregate together.

For example, this test covers this case

opentelemetry-dotnet/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs

Lines 503 to 524 in 3bf47f8

[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");

Both instrument1 and instrument2 match the first view, the metric stream is identical, so they can be aggregated together. The second view is ignored, and a log message is produced in this case.

Update: Actually this log message does still belong. I incorrectly moved it. Moved it back per your comment here #3129 (comment)

metricName,
meterName,
"Metric instrument has the same name as an existing one but differs by description, unit, or instrument type. Measurements from this instrument will still be exported but may result in conflicts.",
"Either change the name of the instrument or use MeterProviderBuilder.AddView to resolve the conflict.");

if (metrics.Count == 0)
{
metrics.Add(existingMetric);
}

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

// TODO: I think this check needs to be moved up...
Copy link
Member

Choose a reason for hiding this comment

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

yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved and added a test for this scenario 462d1af

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.");
Expand Down
238 changes: 238 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,244 @@ 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("name", "newname")
.AddView("name", "newname")
reyang marked this conversation as resolved.
Show resolved Hide resolved
.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 DuplicateInstrumentNamesFromDifferentMetersWithSameNameDifferentVersion()
{
Expand Down