open-telemetry
/
opentelemetry-dotnet
mirror of https://github.com/open-telemetry/opentelemetry-dotnet.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix bug with multiple views mapping to a single metric stream (#3006) 

* Add test demonstrating bug with multiple views selecting one instrument

* Prevent duplicate metric point updates

* Fix formatting

* Update changelog

* No ranges

* Add comment

* Unused using

* Expand instrument registration to consider tags from view

* Fix logic and copy tag keys array

* Fix null check

* Fix formatting

* Include histogram bounds in metric stream identity

* Rename InstrumentIdentity -> MetricStreamIdentity

* Rename instrumentIdentity

* Another test. Two views. Same tags.

* Remove readonly modifier

* Fix hash computation

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
This commit is contained in:
Alan West 2022-04-06 15:30:24 -07:00 committed by GitHub
parent d294140a33
commit 23609730dd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 411 additions and 96 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/OpenTelemetry/CHANGELOG.md
View File

@ -17,6 +17,10 @@ Released 2022-Mar-30
period.
([#2982](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2982))
* Fix bug where multiple views selecting a single instrument can result in
duplicate updates to a single metric point.
([#3006](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3006))
* Added the `PeriodicExportingMetricReaderOptions.ExportTimeoutMilliseconds`
option.
([#3038](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3038))

81
src/OpenTelemetry/Metrics/InstrumentIdentity.cs
View File

@ -1,81 +0,0 @@
// <copyright file="InstrumentIdentity.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System;
using System.Diagnostics.Metrics;
namespace OpenTelemetry.Metrics
{
internal readonly struct InstrumentIdentity : IEquatable<InstrumentIdentity>
{
private readonly int hashCode;
public InstrumentIdentity(Meter meter, string instrumentName, string unit, string description, Type instrumentType)
{
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;
unchecked
{
var hash = 17;
hash = (hash * 31) + this.InstrumentType.GetHashCode();
hash = (hash * 31) + this.MeterName.GetHashCode();
hash = (hash * 31) + this.MeterVersion.GetHashCode();
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();
this.hashCode = hash;
}
}
public readonly string MeterName { get; }
public readonly string MeterVersion { get; }
public readonly string InstrumentName { get; }
public readonly string Unit { get; }
public readonly string Description { get; }
public readonly Type InstrumentType { get; }
public static bool operator ==(InstrumentIdentity metricIdentity1, InstrumentIdentity metricIdentity2) => metricIdentity1.Equals(metricIdentity2);
public static bool operator !=(InstrumentIdentity metricIdentity1, InstrumentIdentity metricIdentity2) => !metricIdentity1.Equals(metricIdentity2);
public override readonly bool Equals(object obj)
{
return obj is InstrumentIdentity other && this.Equals(other);
}
public bool Equals(InstrumentIdentity other)
{
return this.InstrumentType == other.InstrumentType
&& this.MeterName == other.MeterName
&& this.MeterVersion == other.MeterVersion
&& this.InstrumentName == other.InstrumentName
&& this.Unit == other.Unit
&& this.Description == other.Description;
}
public override readonly int GetHashCode() => this.hashCode;
}
}

4
src/OpenTelemetry/Metrics/Metric.cs
View File

@ -30,7 +30,7 @@ namespace OpenTelemetry.Metrics
private readonly AggregatorStore aggStore;
internal Metric(
InstrumentIdentity instrumentIdentity,
MetricStreamIdentity instrumentIdentity,
AggregationTemporality temporality,
int maxMetricPointsPerMetricStream,
double[] histogramBounds = null,
@ -124,7 +124,7 @@ namespace OpenTelemetry.Metrics
public string MeterVersion => this.InstrumentIdentity.MeterVersion;
internal InstrumentIdentity InstrumentIdentity { get; private set; }
internal MetricStreamIdentity InstrumentIdentity { get; private set; }
internal bool InstrumentDisposed { get; set; }

32
src/OpenTelemetry/Metrics/MetricReaderExt.cs
View File

@ -28,7 +28,7 @@ namespace OpenTelemetry.Metrics
public abstract partial class MetricReader
{
private readonly HashSet<string> metricStreamNames = new(StringComparer.OrdinalIgnoreCase);
private readonly ConcurrentDictionary<InstrumentIdentity, Metric> instrumentIdentityToMetric = new();
private readonly ConcurrentDictionary<MetricStreamIdentity, Metric> instrumentIdentityToMetric = new();
private readonly object instrumentCreationLock = new();
private int maxMetricStreams;
private int maxMetricPointsPerMetricStream;
@ -42,10 +42,10 @@ namespace OpenTelemetry.Metrics
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 metricStreamIdentity = new MetricStreamIdentity(instrument.Meter, metricName, instrument.Unit, instrument.Description, instrument.GetType(), null, null);
lock (this.instrumentCreationLock)
{
if (this.instrumentIdentityToMetric.TryGetValue(instrumentIdentity, out var existingMetric))
if (this.instrumentIdentityToMetric.TryGetValue(metricStreamIdentity, out var existingMetric))
{
return existingMetric;
}
@ -70,7 +70,7 @@ namespace OpenTelemetry.Metrics
Metric metric = null;
try
{
metric = new Metric(instrumentIdentity, this.Temporality, this.maxMetricPointsPerMetricStream);
metric = new Metric(metricStreamIdentity, this.Temporality, this.maxMetricPointsPerMetricStream);
}
catch (NotSupportedException nse)
{
@ -82,7 +82,7 @@ namespace OpenTelemetry.Metrics
return null;
}
this.instrumentIdentityToMetric[instrumentIdentity] = metric;
this.instrumentIdentityToMetric[metricStreamIdentity] = metric;
this.metrics[index] = metric;
this.metricStreamNames.Add(metricStreamName);
return metric;
@ -119,7 +119,10 @@ namespace OpenTelemetry.Metrics
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 tagKeysInteresting = metricStreamConfig?.CopiedTagKeys;
var histogramBucketBounds = (metricStreamConfig is ExplicitBucketHistogramConfiguration histogramConfig
&& histogramConfig.CopiedBoundaries != null) ? histogramConfig.CopiedBoundaries : null;
var metricStreamIdentity = new MetricStreamIdentity(instrument.Meter, metricName, instrument.Unit, metricDescription, instrument.GetType(), tagKeysInteresting, histogramBucketBounds);
if (!MeterProviderBuilderSdk.IsValidInstrumentName(metricName))
{
@ -132,9 +135,15 @@ namespace OpenTelemetry.Metrics
continue;
}
if (this.instrumentIdentityToMetric.TryGetValue(instrumentIdentity, out var existingMetric))
if (this.instrumentIdentityToMetric.TryGetValue(metricStreamIdentity, out var existingMetric))
{
metrics.Add(existingMetric);
// The list of metrics may already contain a matching metric with the same
// identity when a single instrument is selected by multiple views.
if (!metrics.Contains(existingMetric))
{
metrics.Add(existingMetric);
}
continue;
}
@ -161,12 +170,9 @@ namespace OpenTelemetry.Metrics
else
{
Metric metric;
string[] tagKeysInteresting = metricStreamConfig?.CopiedTagKeys;
double[] histogramBucketBounds = (metricStreamConfig is ExplicitBucketHistogramConfiguration histogramConfig
&& histogramConfig.CopiedBoundaries != null) ? histogramConfig.CopiedBoundaries : null;
metric = new Metric(instrumentIdentity, this.Temporality, this.maxMetricPointsPerMetricStream, histogramBucketBounds, tagKeysInteresting);
metric = new Metric(metricStreamIdentity, this.Temporality, this.maxMetricPointsPerMetricStream, histogramBucketBounds, tagKeysInteresting);
this.instrumentIdentityToMetric[instrumentIdentity] = metric;
this.instrumentIdentityToMetric[metricStreamIdentity] = metric;
this.metrics[index] = metric;
metrics.Add(metric);
this.metricStreamNames.Add(metricStreamName);

148
src/OpenTelemetry/Metrics/MetricStreamIdentity.cs Normal file
View File

@ -0,0 +1,148 @@
// <copyright file="MetricStreamIdentity.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System;
using System.Diagnostics.Metrics;
namespace OpenTelemetry.Metrics
{
internal readonly struct MetricStreamIdentity : IEquatable<MetricStreamIdentity>
{
private static readonly StringArrayEqualityComparer StringArrayComparer = new StringArrayEqualityComparer();
private readonly int hashCode;
public MetricStreamIdentity(Meter meter, string instrumentName, string unit, string description, Type instrumentType, string[] tagKeys, double[] histogramBucketBounds)
{
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;
if (tagKeys != null && tagKeys.Length > 0)
{
this.TagKeys = new string[tagKeys.Length];
tagKeys.CopyTo(this.TagKeys, 0);
}
else
{
this.TagKeys = null;
}
if (histogramBucketBounds != null && histogramBucketBounds.Length > 0)
{
this.HistogramBucketBounds = new double[histogramBucketBounds.Length];
histogramBucketBounds.CopyTo(this.HistogramBucketBounds, 0);
}
else
{
this.HistogramBucketBounds = null;
}
unchecked
{
var hash = 17;
hash = (hash * 31) + this.InstrumentType.GetHashCode();
hash = (hash * 31) + this.MeterName.GetHashCode();
hash = (hash * 31) + this.MeterVersion.GetHashCode();
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 = this.TagKeys == null ? hash : (hash * 31) + StringArrayComparer.GetHashCode(this.TagKeys);
if (this.HistogramBucketBounds != null)
{
var len = this.HistogramBucketBounds.Length;
for (var i = 0; i < len; ++i)
{
hash = (hash * 31) + this.HistogramBucketBounds[i].GetHashCode();
}
}
this.hashCode = hash;
}
}
public string MeterName { get; }
public string MeterVersion { get; }
public string InstrumentName { get; }
public string Unit { get; }
public string Description { get; }
public Type InstrumentType { get; }
public string[] TagKeys { get; }
public double[] HistogramBucketBounds { get; }
public static bool operator ==(MetricStreamIdentity metricIdentity1, MetricStreamIdentity metricIdentity2) => metricIdentity1.Equals(metricIdentity2);
public static bool operator !=(MetricStreamIdentity metricIdentity1, MetricStreamIdentity metricIdentity2) => !metricIdentity1.Equals(metricIdentity2);
public readonly override bool Equals(object obj)
{
return obj is MetricStreamIdentity other && this.Equals(other);
}
public bool Equals(MetricStreamIdentity other)
{
return this.InstrumentType == other.InstrumentType
&& this.MeterName == other.MeterName
&& this.MeterVersion == other.MeterVersion
&& this.InstrumentName == other.InstrumentName
&& this.Unit == other.Unit
&& this.Description == other.Description
&& StringArrayComparer.Equals(this.TagKeys, other.TagKeys)
&& HistogramBoundsEqual(this.HistogramBucketBounds, other.HistogramBucketBounds);
}
public readonly override int GetHashCode() => this.hashCode;
private static bool HistogramBoundsEqual(double[] bounds1, double[] bounds2)
{
if (ReferenceEquals(bounds1, bounds2))
{
return true;
}
if (ReferenceEquals(bounds1, null) || ReferenceEquals(bounds2, null))
{
return false;
}
var len1 = bounds1.Length;
if (len1 != bounds2.Length)
{
return false;
}
for (int i = 0; i < len1; i++)
{
if (!bounds1[i].Equals(bounds2[i]))
{
return false;
}
}
return true;
}
}
}

69
src/OpenTelemetry/Metrics/StringArrayEqualityComparer.cs Normal file
View File

@ -0,0 +1,69 @@
// <copyright file="StringArrayEqualityComparer.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System;
using System.Collections.Generic;
namespace OpenTelemetry.Metrics
{
internal class StringArrayEqualityComparer : IEqualityComparer<string[]>
{
public bool Equals(string[] strings1, string[] strings2)
{
if (ReferenceEquals(strings1, strings2))
{
return true;
}
if (ReferenceEquals(strings1, null) || ReferenceEquals(strings2, null))
{
return false;
}
var len1 = strings1.Length;
if (len1 != strings2.Length)
{
return false;
}
for (int i = 0; i < len1; i++)
{
if (!strings1[i].Equals(strings2[i], StringComparison.Ordinal))
{
return false;
}
}
return true;
}
public int GetHashCode(string[] strings)
{
int hash = 17;
unchecked
{
for (int i = 0; i < strings.Length; i++)
{
hash = (hash * 31) + strings[i].GetHashCode();
}
}
return hash;
}
}
}

169
test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs
View File

@ -469,6 +469,175 @@ namespace OpenTelemetry.Metrics.Tests
}
}
[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);
Assert.Equal(2, exportedItems.Count);
var metric1 = new List<Metric>() { exportedItems[0] };
var metric2 = new List<Metric>() { exportedItems[1] };
var tag1 = new List<KeyValuePair<string, object>> { tags[0] };
var tag2 = new List<KeyValuePair<string, object>> { tags[1] };
Assert.Equal("name", exportedItems[0].Name);
Assert.Equal("name", exportedItems[1].Name);
Assert.Equal(20, GetLongSum(metric1));
Assert.Equal(20, GetLongSum(metric2));
CheckTagsForNthMetricPoint(metric1, tag1, 1);
CheckTagsForNthMetricPoint(metric2, tag2, 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);
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);
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);
Assert.Equal(2, exportedItems.Count);
var metric1 = exportedItems[0];
var metric2 = exportedItems[1];
Assert.Equal("name", exportedItems[0].Name);
Assert.Equal("name", exportedItems[1].Name);
var metricPoints = new List<MetricPoint>();
foreach (ref readonly var mp in metric1.GetMetricPoints())
{
metricPoints.Add(mp);
}
Assert.Single(metricPoints);
var metricPoint = metricPoints[0];
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++;
}
metricPoints = new List<MetricPoint>();
foreach (ref readonly var mp in metric2.GetMetricPoints())
{
metricPoints.Add(mp);
}
Assert.Single(metricPoints);
metricPoint = metricPoints[0];
Assert.Equal(2, metricPoint.GetHistogramCount());
Assert.Equal(30, metricPoint.GetHistogramSum());
index = 0;
actualCount = 0;
expectedBucketCounts = new long[] { 0, 2, 0 };
foreach (var histogramMeasurement in metricPoint.GetHistogramBuckets())
{
Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount);
index++;
actualCount++;
}
}
[Fact]
public void DuplicateInstrumentNamesFromDifferentMetersWithSameNameDifferentVersion()
{