From bf8b6dd07b2723b18121c4be132df1c9dc8f158b Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 13 Feb 2024 11:33:26 -0800 Subject: [PATCH 01/29] WIP to update exmplar code. --- ...AlignedHistogramBucketExemplarReservoir.cs | 106 +++++++++--------- .../Metrics/Exemplar/Exemplar.cs | 93 ++++++++++++++- .../Metrics/Exemplar/ExemplarReservoir.cs | 17 +-- .../Exemplar/SimpleExemplarReservoir.cs | 4 +- src/OpenTelemetry/ReadOnlyTagCollection.cs | 41 +++++-- 5 files changed, 180 insertions(+), 81 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/AlignedHistogramBucketExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/AlignedHistogramBucketExemplarReservoir.cs index 4f10f3e2527..0ae7324e13c 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/AlignedHistogramBucketExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/AlignedHistogramBucketExemplarReservoir.cs @@ -10,90 +10,88 @@ namespace OpenTelemetry.Metrics; /// internal sealed class AlignedHistogramBucketExemplarReservoir : ExemplarReservoir { - private readonly Exemplar[] runningExemplars; - private readonly Exemplar[] tempExemplars; + private readonly AggregatorStore parentAggregatorStore; + private readonly Exemplar[] bufferA; + private readonly Exemplar[] bufferB; + private Exemplar[] activeBuffer; - public AlignedHistogramBucketExemplarReservoir(int length) + public AlignedHistogramBucketExemplarReservoir(AggregatorStore parentAggregatorStore, int length) { - this.runningExemplars = new Exemplar[length + 1]; - this.tempExemplars = new Exemplar[length + 1]; + Debug.Assert(parentAggregatorStore != null, "parentAggregatorStore was null"); + + this.parentAggregatorStore = parentAggregatorStore; + this.bufferA = new Exemplar[length + 1]; + this.bufferB = new Exemplar[length + 1]; + this.activeBuffer = this.bufferA; } - public override void Offer(long value, ReadOnlySpan> tags, int index = default) + public override void Offer(long value, ReadOnlySpan> tags) { - this.OfferAtBoundary(value, tags, index); + this.OfferAtBoundary(value, tags, this.FindBucketIndexForValue(value)); } - public override void Offer(double value, ReadOnlySpan> tags, int index = default) + public override void Offer(double value, ReadOnlySpan> tags) { - this.OfferAtBoundary(value, tags, index); + this.OfferAtBoundary(value, tags, this.FindBucketIndexForValue(value)); } - public override Exemplar[] Collect(ReadOnlyTagCollection actualTags, bool reset) + public override ReadOnlyExemplarCollection Collect() { - for (int i = 0; i < this.runningExemplars.Length; i++) - { - this.tempExemplars[i] = this.runningExemplars[i]; - if (this.runningExemplars[i].FilteredTags != null) - { - // TODO: Better data structure to avoid this Linq. - // This is doing filtered = alltags - storedtags. - // TODO: At this stage, this logic is done inside Reservoir. - // Kinda hard for end users who write own reservoirs. - // Evaluate if this logic can be moved elsewhere. - // TODO: The cost is paid irrespective of whether the - // Exporter supports Exemplar or not. One idea is to - // defer this until first exporter attempts read. - this.tempExemplars[i].FilteredTags = this.runningExemplars[i].FilteredTags!.Except(actualTags.KeyAndValues.ToList()).ToList(); - } + var currentBuffer = this.activeBuffer; - if (reset) + this.activeBuffer = currentBuffer == this.bufferA + ? this.bufferB + : this.bufferA; + + if (this.parentAggregatorStore.OutputDelta) + { + for (int i = 0; i < this.activeBuffer.Length; i++) { - this.runningExemplars[i].Timestamp = default; + this.activeBuffer[i].Reset(); } } - return this.tempExemplars; + return new(currentBuffer); + } + + protected int FindBucketIndexForValue(double value) + { + } - private void OfferAtBoundary(double value, ReadOnlySpan> tags, int index) + private void OfferAtBoundary(T value, ReadOnlySpan> tags, int bucketIndex) + where T : notnull { - ref var exemplar = ref this.runningExemplars[index]; + ref var exemplar = ref this.activeBuffer[bucketIndex]; + exemplar.Timestamp = DateTimeOffset.UtcNow; - exemplar.DoubleValue = value; - exemplar.TraceId = Activity.Current?.TraceId; - exemplar.SpanId = Activity.Current?.SpanId; - if (tags == default) + if (typeof(T) == typeof(long)) { - // default tag is used to indicate - // the special case where all tags provided at measurement - // recording time are stored. - // In this case, Exemplars does not have to store any tags. - // In other words, FilteredTags will be empty. - return; + exemplar.LongValue = (long)(object)value; } - - if (exemplar.FilteredTags == null) + else if (typeof(T) == typeof(double)) { - exemplar.FilteredTags = new List>(tags.Length); + exemplar.DoubleValue = (double)(object)value; } else { - // Keep the list, but clear contents. - exemplar.FilteredTags.Clear(); + Debug.Fail("Invalid value type"); + exemplar.DoubleValue = Convert.ToDouble((object)value); } - // Though only those tags that are filtered need to be - // stored, finding filtered list from the full tag list - // is expensive. So all the tags are stored in hot path (this). - // During snapshot, the filtered list is calculated. - // TODO: Evaluate alternative approaches based on perf. - // TODO: This is not user friendly to Reservoir authors - // and must be handled as transparently as feasible. - foreach (var tag in tags) + var currentActivity = Activity.Current; + if (currentActivity != null) { - exemplar.FilteredTags.Add(tag); + exemplar.TraceId = currentActivity.TraceId; + exemplar.SpanId = currentActivity.SpanId; } + else + { + exemplar.TraceId = default; + exemplar.SpanId = default; + } + + exemplar.StoreFilteredTags(tags); } } diff --git a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs index d635a1a4ad4..3b80d13badf 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs @@ -28,6 +28,11 @@ namespace OpenTelemetry.Metrics; #endif struct Exemplar { + private readonly HashSet keyFilter; + private int tagCount; + private KeyValuePair[]? tagStorage; + private MetricPointValueStorage valueStorage; + /// /// Gets the timestamp. /// @@ -43,16 +48,96 @@ struct Exemplar /// public ActivitySpanId? SpanId { get; internal set; } - // TODO: Leverage MetricPointValueStorage - // and allow double/long instead of double only. + /// + /// Gets the long value. + /// + public long LongValue + { + get => this.valueStorage.AsLong; + internal set => this.valueStorage.AsLong = value; + } /// /// Gets the double value. /// - public double DoubleValue { get; internal set; } + public double DoubleValue + { + get => this.valueStorage.AsDouble; + internal set => this.valueStorage.AsDouble = value; + } /// /// Gets the FilteredTags (i.e any tags that were dropped during aggregation). /// - public List>? FilteredTags { get; internal set; } + public ReadOnlyTagCollection FilteredTags + => new(this.keyFilter, this.tagStorage ?? Array.Empty>(), this.tagCount); + + internal void StoreFilteredTags(ReadOnlySpan> tags) + { + // todo: We can't share a pointer to array when collecting hmm + + this.tagCount = tags.Length; + if (tags.Length == 0) + { + return; + } + + if (this.tagStorage == null || this.tagStorage.Length < this.tagCount) + { + this.tagStorage = new KeyValuePair[this.tagCount]; + } + + tags.CopyTo(this.tagStorage); + } + + internal void Reset() + { + this.Timestamp = default; + } +} + +public readonly ref struct ReadOnlyExemplarCollection +{ + private readonly Exemplar[] exemplars; + + internal ReadOnlyExemplarCollection(Exemplar[] exemplars) + { + Debug.Assert(exemplars != null, "exemplars was null"); + + this.exemplars = exemplars; + } + + public Enumerator GetEnumerator() => new(this.exemplars); + + /// + /// Enumerates the elements of a . + /// + public struct Enumerator + { + private readonly Exemplar[] exemplars; + private int index; + + internal Enumerator(Exemplar[] exemplars) + { + this.exemplars = exemplars; + this.index = -1; + } + + /// + /// Gets the at the current position of the enumerator. + /// + public readonly ref readonly Exemplar Current + => ref this.exemplars[this.index]; + + /// + /// Advances the enumerator to the next element of the . + /// + /// if the enumerator was + /// successfully advanced to the next element; if the enumerator has passed the end of the + /// collection. + public bool MoveNext() + => ++this.index < this.exemplars.Length; + } } diff --git a/src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs index d5b944ff656..c73c1290b25 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs @@ -13,28 +13,17 @@ internal abstract class ExemplarReservoir /// /// The value of the measurement. /// The complete set of tags provided with the measurement. - /// The histogram bucket index where this measurement is going to be stored. - /// This is optional and is only relevant for Histogram with buckets. - public abstract void Offer(long value, ReadOnlySpan> tags, int index = default); + public abstract void Offer(long value, ReadOnlySpan> tags); /// /// Offers measurement to the reservoir. /// /// The value of the measurement. /// The complete set of tags provided with the measurement. - /// The histogram bucket index where this measurement is going to be stored. - /// This is optional and is only relevant for Histogram with buckets. - public abstract void Offer(double value, ReadOnlySpan> tags, int index = default); + public abstract void Offer(double value, ReadOnlySpan> tags); /// /// Collects all the exemplars accumulated by the Reservoir. /// - /// The actual tags that are part of the metric. Exemplars are - /// only expected to contain any filtered tags, so this will allow the reservoir - /// to prepare the filtered tags from all the tags it is given by doing the - /// equivalent of filtered tags = all tags - actual tags. - /// - /// Flag to indicate if the reservoir should be reset after this call. - /// Array of Exemplars. - public abstract Exemplar[] Collect(ReadOnlyTagCollection actualTags, bool reset); + public abstract ReadOnlyExemplarCollection Collect(); } diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleExemplarReservoir.cs index b719c902edc..bc8324fabd8 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleExemplarReservoir.cs @@ -26,12 +26,12 @@ public SimpleExemplarReservoir(int poolSize) this.random = new Random(); } - public override void Offer(long value, ReadOnlySpan> tags, int index = default) + public override void Offer(long value, ReadOnlySpan> tags) { this.Offer(value, tags); } - public override void Offer(double value, ReadOnlySpan> tags, int index = default) + public override void Offer(double value, ReadOnlySpan> tags) { this.Offer(value, tags); } diff --git a/src/OpenTelemetry/ReadOnlyTagCollection.cs b/src/OpenTelemetry/ReadOnlyTagCollection.cs index 3c7dc59d770..73102b35546 100644 --- a/src/OpenTelemetry/ReadOnlyTagCollection.cs +++ b/src/OpenTelemetry/ReadOnlyTagCollection.cs @@ -1,6 +1,8 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +using System.Diagnostics; + namespace OpenTelemetry; /// @@ -11,16 +13,31 @@ namespace OpenTelemetry; public readonly struct ReadOnlyTagCollection { internal readonly KeyValuePair[] KeyAndValues; + private readonly HashSet? keyFilter; + private readonly int count; internal ReadOnlyTagCollection(KeyValuePair[]? keyAndValues) { this.KeyAndValues = keyAndValues ?? Array.Empty>(); + this.keyFilter = null; + this.count = this.KeyAndValues.Length; + } + + internal ReadOnlyTagCollection(HashSet keyFilter, KeyValuePair[] keyAndValues, int count) + { + Debug.Assert(keyFilter != null, "keyFilter was null"); + Debug.Assert(keyAndValues != null, "keyAndValues was null"); + Debug.Assert(count <= keyAndValues.Length, "count was invalid"); + + this.keyFilter = keyFilter; + this.KeyAndValues = keyAndValues; + this.count = count; } /// /// Gets the number of tags in the collection. /// - public int Count => this.KeyAndValues.Length; + public int Count => this.count; /// /// Returns an enumerator that iterates through the tags. @@ -59,14 +76,24 @@ internal Enumerator(ReadOnlyTagCollection source) /// collection. public bool MoveNext() { - int index = this.index; - - if (index < this.source.Count) + while (true) { - this.Current = this.source.KeyAndValues[index]; + int index = this.index; + if (index < this.source.Count) + { + var item = this.source.KeyAndValues[index++]; + this.index = index; + + if (this.source.keyFilter?.Contains(item.Key) == true) + { + continue; + } + + this.Current = item; + return true; + } - this.index++; - return true; + break; } return false; From daf57cded5e03e4f7a82ceb92688e73cad112408 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 13 Feb 2024 16:26:23 -0800 Subject: [PATCH 02/29] First compiling version. --- .../ConsoleMetricExporter.cs | 20 +-- .../Implementation/MetricItemExtensions.cs | 22 ++- src/OpenTelemetry/Metrics/AggregatorStore.cs | 8 +- ...AlignedHistogramBucketExemplarReservoir.cs | 92 +++--------- .../Metrics/Exemplar/Exemplar.cs | 58 +------- .../Metrics/Exemplar/ExemplarMeasurement.cs | 57 +++++++ .../Metrics/Exemplar/ExemplarReservoir.cs | 23 ++- .../Exemplar/FixedSizeExemplarReservoir.cs | 94 ++++++++++++ .../Exemplar/ReadOnlyExemplarCollection.cs | 98 ++++++++++++ .../Exemplar/SimpleExemplarReservoir.cs | 140 ------------------ .../SimpleFixedSizeExemplarReservoir.cs | 65 ++++++++ src/OpenTelemetry/Metrics/MetricPoint.cs | 67 +++++---- .../Metrics/MetricPointOptionalComponents.cs | 9 +- .../ReadOnlyFilteredTagCollection.cs | 125 ++++++++++++++++ src/OpenTelemetry/ReadOnlyTagCollection.cs | 43 +----- .../Metrics/MetricExemplarTests.cs | 15 +- .../Metrics/MetricTestsBase.cs | 9 +- 17 files changed, 564 insertions(+), 381 deletions(-) create mode 100644 src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs create mode 100644 src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs create mode 100644 src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs delete mode 100644 src/OpenTelemetry/Metrics/Exemplar/SimpleExemplarReservoir.cs create mode 100644 src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs create mode 100644 src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs index 4899f0d923e..939990d6955 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs @@ -188,9 +188,9 @@ public override ExportResult Export(in Batch batch) } var exemplarString = new StringBuilder(); - foreach (var exemplar in metricPoint.GetExemplars()) + if (metricPoint.TryGetExemplars(out var exemplars)) { - if (exemplar.Timestamp != default) + foreach (var exemplar in exemplars) { exemplarString.Append("Value: "); exemplarString.Append(exemplar.DoubleValue); @@ -201,17 +201,19 @@ public override ExportResult Export(in Batch batch) exemplarString.Append(" SpanId: "); exemplarString.Append(exemplar.SpanId); - if (exemplar.FilteredTags != null && exemplar.FilteredTags.Count > 0) + bool appendedTagString = false; + foreach (var tag in exemplar.FilteredTags) { - exemplarString.Append(" Filtered Tags : "); - - foreach (var tag in exemplar.FilteredTags) + if (ConsoleTagTransformer.Instance.TryTransformTag(tag, out var result)) { - if (ConsoleTagTransformer.Instance.TryTransformTag(tag, out var result)) + if (!appendedTagString) { - exemplarString.Append(result); - exemplarString.Append(' '); + exemplarString.Append(" Filtered Tags : "); + appendedTagString = true; } + + exemplarString.Append(result); + exemplarString.Append(' '); } } diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs index d93105e3dfb..63f78bdfb81 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs @@ -267,33 +267,29 @@ internal static OtlpMetrics.Metric ToOtlpMetric(this Metric metric) } } - var exemplars = metricPoint.GetExemplars(); - foreach (var examplar in exemplars) + if (metricPoint.TryGetExemplars(out var exemplars)) { - if (examplar.Timestamp != default) + foreach (ref readonly var exemplar in exemplars) { byte[] traceIdBytes = new byte[16]; - examplar.TraceId?.CopyTo(traceIdBytes); + exemplar.TraceId?.CopyTo(traceIdBytes); byte[] spanIdBytes = new byte[8]; - examplar.SpanId?.CopyTo(spanIdBytes); + exemplar.SpanId?.CopyTo(spanIdBytes); var otlpExemplar = new OtlpMetrics.Exemplar { - TimeUnixNano = (ulong)examplar.Timestamp.ToUnixTimeNanoseconds(), + TimeUnixNano = (ulong)exemplar.Timestamp.ToUnixTimeNanoseconds(), TraceId = UnsafeByteOperations.UnsafeWrap(traceIdBytes), SpanId = UnsafeByteOperations.UnsafeWrap(spanIdBytes), - AsDouble = examplar.DoubleValue, + AsDouble = exemplar.DoubleValue, }; - if (examplar.FilteredTags != null) + foreach (var tag in exemplar.FilteredTags) { - foreach (var tag in examplar.FilteredTags) + if (OtlpKeyValueTransformer.Instance.TryTransformTag(tag, out var result)) { - if (OtlpKeyValueTransformer.Instance.TryTransformTag(tag, out var result)) - { - otlpExemplar.FilteredAttributes.Add(result); - } + otlpExemplar.FilteredAttributes.Add(result); } } diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index a5d603589ad..fa4cefc7221 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -11,6 +11,7 @@ namespace OpenTelemetry.Metrics; internal sealed class AggregatorStore { + internal readonly HashSet? TagKeysInteresting; internal readonly bool OutputDelta; internal readonly bool OutputDeltaWithUnusedMetricPointReclaimEnabled; internal readonly int CardinalityLimit; @@ -24,7 +25,6 @@ internal sealed class AggregatorStore private readonly object lockZeroTags = new(); private readonly object lockOverflowTag = new(); - private readonly HashSet? tagKeysInteresting; private readonly int tagsKeysInterestingCount; // This holds the reclaimed MetricPoints that are available for reuse. @@ -84,7 +84,7 @@ internal AggregatorStore( this.updateLongCallback = this.UpdateLongCustomTags; this.updateDoubleCallback = this.UpdateDoubleCustomTags; var hs = new HashSet(metricStreamIdentity.TagKeys, StringComparer.Ordinal); - this.tagKeysInteresting = hs; + this.TagKeysInteresting = hs; this.tagsKeysInterestingCount = hs.Count; } @@ -1122,9 +1122,9 @@ private int FindMetricAggregatorsCustomTag(ReadOnlySpan /// The AlignedHistogramBucketExemplarReservoir implementation. /// -internal sealed class AlignedHistogramBucketExemplarReservoir : ExemplarReservoir +internal sealed class AlignedHistogramBucketExemplarReservoir : FixedSizeExemplarReservoir { - private readonly AggregatorStore parentAggregatorStore; - private readonly Exemplar[] bufferA; - private readonly Exemplar[] bufferB; - private Exemplar[] activeBuffer; - - public AlignedHistogramBucketExemplarReservoir(AggregatorStore parentAggregatorStore, int length) + public AlignedHistogramBucketExemplarReservoir(int numberOfBuckets) + : base(numberOfBuckets) { - Debug.Assert(parentAggregatorStore != null, "parentAggregatorStore was null"); - - this.parentAggregatorStore = parentAggregatorStore; - this.bufferA = new Exemplar[length + 1]; - this.bufferB = new Exemplar[length + 1]; - this.activeBuffer = this.bufferA; } - public override void Offer(long value, ReadOnlySpan> tags) + public override void Offer(in ExemplarMeasurement measurement) { - this.OfferAtBoundary(value, tags, this.FindBucketIndexForValue(value)); - } + Debug.Assert( + measurement.ExplicitBucketHistogramBucketIndex != -1, + "ExplicitBucketHistogramBucketIndex was -1"); - public override void Offer(double value, ReadOnlySpan> tags) - { - this.OfferAtBoundary(value, tags, this.FindBucketIndexForValue(value)); + this.UpdateExemplar( + measurement.ExplicitBucketHistogramBucketIndex, + in measurement); } - public override ReadOnlyExemplarCollection Collect() + public override void Offer(in ExemplarMeasurement measurement) { - var currentBuffer = this.activeBuffer; - - this.activeBuffer = currentBuffer == this.bufferA - ? this.bufferB - : this.bufferA; - - if (this.parentAggregatorStore.OutputDelta) - { - for (int i = 0; i < this.activeBuffer.Length; i++) - { - this.activeBuffer[i].Reset(); - } - } - - return new(currentBuffer); - } - - protected int FindBucketIndexForValue(double value) - { - - } - - private void OfferAtBoundary(T value, ReadOnlySpan> tags, int bucketIndex) - where T : notnull - { - ref var exemplar = ref this.activeBuffer[bucketIndex]; - - exemplar.Timestamp = DateTimeOffset.UtcNow; - - if (typeof(T) == typeof(long)) - { - exemplar.LongValue = (long)(object)value; - } - else if (typeof(T) == typeof(double)) - { - exemplar.DoubleValue = (double)(object)value; - } - else - { - Debug.Fail("Invalid value type"); - exemplar.DoubleValue = Convert.ToDouble((object)value); - } - - var currentActivity = Activity.Current; - if (currentActivity != null) - { - exemplar.TraceId = currentActivity.TraceId; - exemplar.SpanId = currentActivity.SpanId; - } - else - { - exemplar.TraceId = default; - exemplar.SpanId = default; - } + Debug.Assert( + measurement.ExplicitBucketHistogramBucketIndex != -1, + "ExplicitBucketHistogramBucketIndex was -1"); - exemplar.StoreFilteredTags(tags); + this.UpdateExemplar( + measurement.ExplicitBucketHistogramBucketIndex, + in measurement); } } diff --git a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs index 3b80d13badf..c73e6cd24ce 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs @@ -28,7 +28,7 @@ namespace OpenTelemetry.Metrics; #endif struct Exemplar { - private readonly HashSet keyFilter; + internal HashSet? KeyFilter; private int tagCount; private KeyValuePair[]? tagStorage; private MetricPointValueStorage valueStorage; @@ -53,7 +53,7 @@ struct Exemplar /// public long LongValue { - get => this.valueStorage.AsLong; + readonly get => this.valueStorage.AsLong; internal set => this.valueStorage.AsLong = value; } @@ -62,20 +62,18 @@ public long LongValue /// public double DoubleValue { - get => this.valueStorage.AsDouble; + readonly get => this.valueStorage.AsDouble; internal set => this.valueStorage.AsDouble = value; } /// /// Gets the FilteredTags (i.e any tags that were dropped during aggregation). /// - public ReadOnlyTagCollection FilteredTags - => new(this.keyFilter, this.tagStorage ?? Array.Empty>(), this.tagCount); + public readonly ReadOnlyFilteredTagCollection FilteredTags + => new(this.KeyFilter, this.tagStorage ?? Array.Empty>(), this.tagCount); internal void StoreFilteredTags(ReadOnlySpan> tags) { - // todo: We can't share a pointer to array when collecting hmm - this.tagCount = tags.Length; if (tags.Length == 0) { @@ -95,49 +93,3 @@ internal void Reset() this.Timestamp = default; } } - -public readonly ref struct ReadOnlyExemplarCollection -{ - private readonly Exemplar[] exemplars; - - internal ReadOnlyExemplarCollection(Exemplar[] exemplars) - { - Debug.Assert(exemplars != null, "exemplars was null"); - - this.exemplars = exemplars; - } - - public Enumerator GetEnumerator() => new(this.exemplars); - - /// - /// Enumerates the elements of a . - /// - public struct Enumerator - { - private readonly Exemplar[] exemplars; - private int index; - - internal Enumerator(Exemplar[] exemplars) - { - this.exemplars = exemplars; - this.index = -1; - } - - /// - /// Gets the at the current position of the enumerator. - /// - public readonly ref readonly Exemplar Current - => ref this.exemplars[this.index]; - - /// - /// Advances the enumerator to the next element of the . - /// - /// if the enumerator was - /// successfully advanced to the next element; if the enumerator has passed the end of the - /// collection. - public bool MoveNext() - => ++this.index < this.exemplars.Length; - } -} diff --git a/src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs b/src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs new file mode 100644 index 00000000000..4b0bfd777b8 --- /dev/null +++ b/src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs @@ -0,0 +1,57 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#if EXPOSE_EXPERIMENTAL_FEATURES && NET8_0_OR_GREATER +using System.Diagnostics.CodeAnalysis; +using OpenTelemetry.Internal; +#endif + +namespace OpenTelemetry.Metrics; + +#if EXPOSE_EXPERIMENTAL_FEATURES +/// +/// Represents an Exemplar measurement. +/// +/// +/// Measurement type. +#if NET8_0_OR_GREATER +[Experimental(DiagnosticDefinitions.ExemplarExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)] +#endif +public +#else +internal +#endif + readonly ref struct ExemplarMeasurement + where T : struct +{ + internal ExemplarMeasurement( + T value, + ReadOnlySpan> tags) + { + this.Value = value; + this.Tags = tags; + this.ExplicitBucketHistogramBucketIndex = -1; + } + + internal ExemplarMeasurement( + T value, + ReadOnlySpan> tags, + int explicitBucketHistogramIndex) + { + this.Value = value; + this.Tags = tags; + this.ExplicitBucketHistogramBucketIndex = explicitBucketHistogramIndex; + } + + /// + /// Gets the measurement value. + /// + public T Value { get; } + + /// + /// Gets the measurement tags. + /// + public ReadOnlySpan> Tags { get; } + + internal int ExplicitBucketHistogramBucketIndex { get; } +} diff --git a/src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs index c73c1290b25..b8cf161e30b 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/ExemplarReservoir.cs @@ -8,22 +8,33 @@ namespace OpenTelemetry.Metrics; /// internal abstract class ExemplarReservoir { + /// + /// Gets a value indicating whether or not the should reset its state when performing + /// collection. + /// + public bool ResetOnCollect { get; private set; } + /// /// Offers measurement to the reservoir. /// - /// The value of the measurement. - /// The complete set of tags provided with the measurement. - public abstract void Offer(long value, ReadOnlySpan> tags); + /// . + public abstract void Offer(in ExemplarMeasurement measurement); /// /// Offers measurement to the reservoir. /// - /// The value of the measurement. - /// The complete set of tags provided with the measurement. - public abstract void Offer(double value, ReadOnlySpan> tags); + /// . + public abstract void Offer(in ExemplarMeasurement measurement); /// /// Collects all the exemplars accumulated by the Reservoir. /// + /// . public abstract ReadOnlyExemplarCollection Collect(); + + internal virtual void Initialize(AggregatorStore aggregatorStore) + { + this.ResetOnCollect = aggregatorStore.OutputDelta; + } } diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs new file mode 100644 index 00000000000..98e4e8bbcca --- /dev/null +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -0,0 +1,94 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +using System.Diagnostics; + +namespace OpenTelemetry.Metrics; + +internal abstract class FixedSizeExemplarReservoir : ExemplarReservoir +{ + private Exemplar[] bufferA = Array.Empty(); + private Exemplar[] bufferB = Array.Empty(); + private Exemplar[] activeBuffer = Array.Empty(); + + protected FixedSizeExemplarReservoir(int exemplarCount) + { + this.ExemplarCount = exemplarCount; + } + + internal int ExemplarCount { get; } + + /// + /// Collects all the exemplars accumulated by the Reservoir. + /// + /// . + public override ReadOnlyExemplarCollection Collect() + { + var currentBuffer = this.activeBuffer; + + this.activeBuffer = currentBuffer == this.bufferA + ? this.bufferB + : this.bufferA; + + if (this.ResetOnCollect) + { + for (int i = 0; i < this.activeBuffer.Length; i++) + { + this.activeBuffer[i].Reset(); + } + } + + return new(currentBuffer); + } + + internal sealed override void Initialize(AggregatorStore aggregatorStore) + { + this.bufferA = new Exemplar[this.ExemplarCount]; + this.bufferB = new Exemplar[this.ExemplarCount]; + this.activeBuffer = this.bufferA; + + for (int i = 0; i < this.ExemplarCount; i++) + { + this.bufferA[i].KeyFilter = aggregatorStore.TagKeysInteresting; + this.bufferB[i].KeyFilter = aggregatorStore.TagKeysInteresting; + } + + base.Initialize(aggregatorStore); + } + + internal void UpdateExemplar(int exemplarIndex, in ExemplarMeasurement measurement) + where T : struct + { + ref var exemplar = ref this.activeBuffer[exemplarIndex]; + + exemplar.Timestamp = DateTimeOffset.UtcNow; + + if (typeof(T) == typeof(long)) + { + exemplar.LongValue = (long)(object)measurement.Value; + } + else if (typeof(T) == typeof(double)) + { + exemplar.DoubleValue = (double)(object)measurement.Value; + } + else + { + Debug.Fail("Invalid value type"); + exemplar.DoubleValue = Convert.ToDouble(measurement.Value); + } + + var currentActivity = Activity.Current; + if (currentActivity != null) + { + exemplar.TraceId = currentActivity.TraceId; + exemplar.SpanId = currentActivity.SpanId; + } + else + { + exemplar.TraceId = default; + exemplar.SpanId = default; + } + + exemplar.StoreFilteredTags(measurement.Tags); + } +} diff --git a/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs b/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs new file mode 100644 index 00000000000..081501ab8cd --- /dev/null +++ b/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs @@ -0,0 +1,98 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +using System.Diagnostics; +#if EXPOSE_EXPERIMENTAL_FEATURES && NET8_0_OR_GREATER +using System.Diagnostics.CodeAnalysis; +using OpenTelemetry.Internal; +#endif + +namespace OpenTelemetry.Metrics; + +#if EXPOSE_EXPERIMENTAL_FEATURES +/// +/// A read-only collection of s. +/// +/// +#if NET8_0_OR_GREATER +[Experimental(DiagnosticDefinitions.ExemplarExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)] +#endif +public +#else +internal +#endif + readonly struct ReadOnlyExemplarCollection +{ + private readonly Exemplar[] exemplars; + + internal ReadOnlyExemplarCollection(Exemplar[] exemplars) + { + Debug.Assert(exemplars != null, "exemplars was null"); + + this.exemplars = exemplars!; + } + + /// + /// Returns an enumerator that iterates through the s. + /// + /// . + public Enumerator GetEnumerator() + => new(this.exemplars); + + internal ReadOnlyExemplarCollection Copy() + { + var copy = new Exemplar[this.exemplars.Length]; + this.exemplars.CopyTo(copy, 0); + return new ReadOnlyExemplarCollection(copy); + } + + /// + /// Enumerates the elements of a . + /// + public struct Enumerator + { + private readonly Exemplar[] exemplars; + private int index; + + internal Enumerator(Exemplar[] exemplars) + { + this.exemplars = exemplars; + this.index = -1; + } + + /// + /// Gets the at the current position of the enumerator. + /// + public readonly ref readonly Exemplar Current + => ref this.exemplars[this.index]; + + /// + /// Advances the enumerator to the next element of the . + /// + /// if the enumerator was + /// successfully advanced to the next element; if the enumerator has passed the end of the + /// collection. + public bool MoveNext() + { + while (true) + { + var index = ++this.index; + if (index < this.exemplars.Length) + { + if (this.exemplars[index].Timestamp == default) + { + continue; + } + + return true; + } + + break; + } + + return false; + } + } +} diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleExemplarReservoir.cs deleted file mode 100644 index bc8324fabd8..00000000000 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleExemplarReservoir.cs +++ /dev/null @@ -1,140 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -using System.Diagnostics; - -namespace OpenTelemetry.Metrics; - -/// -/// The SimpleExemplarReservoir implementation. -/// -internal sealed class SimpleExemplarReservoir : ExemplarReservoir -{ - private readonly int poolSize; - private readonly Random random; - private readonly Exemplar[] runningExemplars; - private readonly Exemplar[] tempExemplars; - - private long measurementsSeen; - - public SimpleExemplarReservoir(int poolSize) - { - this.poolSize = poolSize; - this.runningExemplars = new Exemplar[poolSize]; - this.tempExemplars = new Exemplar[poolSize]; - this.measurementsSeen = 0; - this.random = new Random(); - } - - public override void Offer(long value, ReadOnlySpan> tags) - { - this.Offer(value, tags); - } - - public override void Offer(double value, ReadOnlySpan> tags) - { - this.Offer(value, tags); - } - - public override Exemplar[] Collect(ReadOnlyTagCollection actualTags, bool reset) - { - for (int i = 0; i < this.runningExemplars.Length; i++) - { - this.tempExemplars[i] = this.runningExemplars[i]; - if (this.runningExemplars[i].FilteredTags != null) - { - // TODO: Better data structure to avoid this Linq. - // This is doing filtered = alltags - storedtags. - // TODO: At this stage, this logic is done inside Reservoir. - // Kinda hard for end users who write own reservoirs. - // Evaluate if this logic can be moved elsewhere. - // TODO: The cost is paid irrespective of whether the - // Exporter supports Exemplar or not. One idea is to - // defer this until first exporter attempts read. - this.tempExemplars[i].FilteredTags = this.runningExemplars[i].FilteredTags!.Except(actualTags.KeyAndValues.ToList()).ToList(); - } - - if (reset) - { - this.runningExemplars[i].Timestamp = default; - } - } - - // Reset internal state irrespective of temporality. - // This ensures incoming measurements have fair chance - // of making it to the reservoir. - this.measurementsSeen = 0; - - return this.tempExemplars; - } - - private void Offer(double value, ReadOnlySpan> tags) - { - if (this.measurementsSeen < this.poolSize) - { - ref var exemplar = ref this.runningExemplars[this.measurementsSeen]; - exemplar.Timestamp = DateTimeOffset.UtcNow; - exemplar.DoubleValue = value; - exemplar.TraceId = Activity.Current?.TraceId; - exemplar.SpanId = Activity.Current?.SpanId; - this.StoreTags(ref exemplar, tags); - } - else - { - // TODO: RandomNext64 is only available in .NET 6 or newer. - int upperBound = 0; - unchecked - { - upperBound = (int)this.measurementsSeen; - } - - var index = this.random.Next(0, upperBound); - if (index < this.poolSize) - { - ref var exemplar = ref this.runningExemplars[index]; - exemplar.Timestamp = DateTimeOffset.UtcNow; - exemplar.DoubleValue = value; - exemplar.TraceId = Activity.Current?.TraceId; - exemplar.SpanId = Activity.Current?.SpanId; - this.StoreTags(ref exemplar, tags); - } - } - - this.measurementsSeen++; - } - - private void StoreTags(ref Exemplar exemplar, ReadOnlySpan> tags) - { - if (tags == default) - { - // default tag is used to indicate - // the special case where all tags provided at measurement - // recording time are stored. - // In this case, Exemplars does not have to store any tags. - // In other words, FilteredTags will be empty. - return; - } - - if (exemplar.FilteredTags == null) - { - exemplar.FilteredTags = new List>(tags.Length); - } - else - { - // Keep the list, but clear contents. - exemplar.FilteredTags.Clear(); - } - - // Though only those tags that are filtered need to be - // stored, finding filtered list from the full tag list - // is expensive. So all the tags are stored in hot path (this). - // During snapshot, the filtered list is calculated. - // TODO: Evaluate alternative approaches based on perf. - // TODO: This is not user friendly to Reservoir authors - // and must be handled as transparently as feasible. - foreach (var tag in tags) - { - exemplar.FilteredTags.Add(tag); - } - } -} diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs new file mode 100644 index 00000000000..4852fe07995 --- /dev/null +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs @@ -0,0 +1,65 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +namespace OpenTelemetry.Metrics; + +/// +/// The SimpleExemplarReservoir implementation. +/// +internal sealed class SimpleFixedSizeExemplarReservoir : FixedSizeExemplarReservoir +{ + private readonly Random random; + + private int measurementsSeen; + + public SimpleFixedSizeExemplarReservoir(int poolSize) + : base(poolSize) + { + this.measurementsSeen = 0; + this.random = new Random(); + } + + public override void Offer(in ExemplarMeasurement measurement) + { + this.Offer(in measurement); + } + + public override void Offer(in ExemplarMeasurement measurement) + { + this.Offer(in measurement); + } + + public override ReadOnlyExemplarCollection Collect() + { + // Reset internal state irrespective of temporality. + // This ensures incoming measurements have fair chance + // of making it to the reservoir. + this.measurementsSeen = 0; + + return base.Collect(); + } + + private void Offer(in ExemplarMeasurement measurement) + where T : struct + { + if (this.measurementsSeen < this.ExemplarCount) + { + this.UpdateExemplar(this.measurementsSeen, in measurement); + } + else + { + int index; + lock (this.random) + { + index = this.random.Next(0, this.measurementsSeen); + } + + if (index < this.ExemplarCount) + { + this.UpdateExemplar(index, in measurement); + } + } + + Interlocked.Increment(ref this.measurementsSeen); + } +} diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index 349afd51bd9..7842c13b650 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; namespace OpenTelemetry.Metrics; @@ -91,7 +92,7 @@ internal MetricPoint( if (aggregatorStore!.IsExemplarEnabled() && reservoir == null) { - reservoir = new SimpleExemplarReservoir(DefaultSimpleReservoirPoolSize); + reservoir = new SimpleFixedSizeExemplarReservoir(DefaultSimpleReservoirPoolSize); } if (reservoir != null) @@ -101,6 +102,8 @@ internal MetricPoint( this.mpComponents = new MetricPointOptionalComponents(); } + reservoir.Initialize(aggregatorStore); + this.mpComponents.ExemplarReservoir = reservoir; } @@ -345,21 +348,18 @@ public readonly bool TryGetHistogramMinMaxValues(out double min, out double max) /// Gets the exemplars associated with the metric point. /// /// - /// . + /// . + /// if exemplars exist; otherwise. [MethodImpl(MethodImplOptions.AggressiveInlining)] public #else - /// - /// Gets the exemplars associated with the metric point. - /// - /// . [MethodImpl(MethodImplOptions.AggressiveInlining)] internal #endif - readonly Exemplar[] GetExemplars() + readonly bool TryGetExemplars([NotNullWhen(true)] out ReadOnlyExemplarCollection? exemplars) { - // TODO: Do not expose Exemplar data structure (array now) - return this.mpComponents?.Exemplars ?? Array.Empty(); + exemplars = this.mpComponents?.Exemplars; + return exemplars.HasValue; } internal readonly MetricPoint Copy() @@ -468,7 +468,8 @@ internal void UpdateWithExemplar(long number, ReadOnlySpan(number, tags)); } ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -488,7 +489,8 @@ internal void UpdateWithExemplar(long number, ReadOnlySpan(number, tags)); } ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -508,7 +510,8 @@ internal void UpdateWithExemplar(long number, ReadOnlySpan(number, tags)); } ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -689,7 +692,8 @@ internal void UpdateWithExemplar(double number, ReadOnlySpan(number, tags)); } ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -712,7 +716,8 @@ internal void UpdateWithExemplar(double number, ReadOnlySpan(number, tags)); } ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -735,7 +740,8 @@ internal void UpdateWithExemplar(double number, ReadOnlySpan(number, tags)); } ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -926,8 +932,6 @@ internal void TakeSnapshot(bool outputDelta) } } - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); @@ -990,7 +994,6 @@ internal void TakeSnapshot(bool outputDelta) } } - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); this.MetricPointStatus = MetricPointStatus.NoCollectPending; ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); @@ -1107,7 +1110,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) this.snapshotValue.AsLong = this.runningValue.AsLong; } - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); + this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -1131,7 +1134,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) this.snapshotValue.AsDouble = this.runningValue.AsDouble; } - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); + this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -1144,7 +1147,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) this.snapshotValue.AsLong = this.runningValue.AsLong; this.MetricPointStatus = MetricPointStatus.NoCollectPending; - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); + this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -1157,7 +1160,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) this.snapshotValue.AsDouble = this.runningValue.AsDouble; this.MetricPointStatus = MetricPointStatus.NoCollectPending; - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); + this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); @@ -1192,7 +1195,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) } } - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); + this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); this.MetricPointStatus = MetricPointStatus.NoCollectPending; @@ -1218,7 +1221,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) histogramBuckets.RunningSum = 0; } - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); + this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); this.MetricPointStatus = MetricPointStatus.NoCollectPending; ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); @@ -1258,7 +1261,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) } } - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); + this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); this.MetricPointStatus = MetricPointStatus.NoCollectPending; ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); @@ -1287,7 +1290,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta) histogramBuckets.RunningMax = double.NegativeInfinity; } - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta); + this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); this.MetricPointStatus = MetricPointStatus.NoCollectPending; ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); @@ -1387,7 +1390,8 @@ private void UpdateHistogram(double number, ReadOnlySpan(number, tags)); } ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); @@ -1415,7 +1419,8 @@ private void UpdateHistogramWithMinMax(double number, ReadOnlySpan(number, tags)); } ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); @@ -1445,7 +1450,8 @@ private void UpdateHistogramWithBuckets(double number, ReadOnlySpan(number, tags, explicitBucketHistogramIndex: i)); } } @@ -1476,7 +1482,8 @@ private void UpdateHistogramWithBucketsAndMinMax(double number, ReadOnlySpan(number, tags, explicitBucketHistogramIndex: i)); } histogramBuckets.RunningMin = Math.Min(histogramBuckets.RunningMin, number); diff --git a/src/OpenTelemetry/Metrics/MetricPointOptionalComponents.cs b/src/OpenTelemetry/Metrics/MetricPointOptionalComponents.cs index 9b3221869c9..6c44229761a 100644 --- a/src/OpenTelemetry/Metrics/MetricPointOptionalComponents.cs +++ b/src/OpenTelemetry/Metrics/MetricPointOptionalComponents.cs @@ -18,7 +18,7 @@ internal sealed class MetricPointOptionalComponents public ExemplarReservoir? ExemplarReservoir; - public Exemplar[]? Exemplars; + public ReadOnlyExemplarCollection? Exemplars; public int IsCriticalSectionOccupied = 0; @@ -28,14 +28,9 @@ internal MetricPointOptionalComponents Copy() { HistogramBuckets = this.HistogramBuckets?.Copy(), Base2ExponentialBucketHistogram = this.Base2ExponentialBucketHistogram?.Copy(), + Exemplars = this.Exemplars?.Copy(), }; - if (this.Exemplars != null) - { - copy.Exemplars = new Exemplar[this.Exemplars.Length]; - Array.Copy(this.Exemplars, copy.Exemplars, this.Exemplars.Length); - } - return copy; } } diff --git a/src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs b/src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs new file mode 100644 index 00000000000..85f74ae626f --- /dev/null +++ b/src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs @@ -0,0 +1,125 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +using System.Diagnostics; +#if EXPOSE_EXPERIMENTAL_FEATURES && NET8_0_OR_GREATER +using System.Diagnostics.CodeAnalysis; +using OpenTelemetry.Internal; +#endif + +namespace OpenTelemetry; + +#if EXPOSE_EXPERIMENTAL_FEATURES +/// +/// A read-only collection of tag key/value pairs which returns a filtered +/// subset of tags when enumerated. +/// +// Note: Does not implement IReadOnlyCollection<> or IEnumerable<> to +// prevent accidental boxing. +#if NET8_0_OR_GREATER +[Experimental(DiagnosticDefinitions.ExemplarExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)] +#endif +public +#else +internal +#endif + readonly struct ReadOnlyFilteredTagCollection +{ + private readonly HashSet? keyFilter; + private readonly KeyValuePair[] tags; + private readonly int count; + + internal ReadOnlyFilteredTagCollection( + HashSet? keyFilter, + KeyValuePair[] tags, + int count) + { + Debug.Assert(tags != null, "tags was null"); + Debug.Assert(count <= tags!.Length, "count was invalid"); + + this.keyFilter = keyFilter; + this.tags = tags; + this.count = count; + } + + /// + /// Gets the maximum number of tags in the collection. + /// + /// + /// Note: Enumerating the collection may return fewer results depending on + /// the filter. + /// + public int MaximumCount => this.count; + + /// + /// Returns an enumerator that iterates through the tags. + /// + /// . + public Enumerator GetEnumerator() => new(this); + + internal IReadOnlyList> ToReadOnlyList() + { + var list = new List>(this.MaximumCount); + + foreach (var item in this) + { + list.Add(item); + } + + return list; + } + + /// + /// Enumerates the elements of a . + /// + // Note: Does not implement IEnumerator<> to prevent accidental boxing. + public struct Enumerator + { + private readonly ReadOnlyFilteredTagCollection source; + private int index; + + internal Enumerator(ReadOnlyFilteredTagCollection source) + { + this.source = source; + this.index = -1; + this.Current = default; + } + + /// + /// Gets the tag at the current position of the enumerator. + /// + public KeyValuePair Current { readonly get; private set; } + + /// + /// Advances the enumerator to the next element of the . + /// + /// if the enumerator was + /// successfully advanced to the next element; if the enumerator has passed the end of the + /// collection. + public bool MoveNext() + { + while (true) + { + int index = ++this.index; + if (index < this.source.MaximumCount) + { + var item = this.source.tags[index]; + + if (this.source.keyFilter?.Contains(item.Key) == true) + { + continue; + } + + this.Current = item; + return true; + } + + break; + } + + return false; + } + } +} diff --git a/src/OpenTelemetry/ReadOnlyTagCollection.cs b/src/OpenTelemetry/ReadOnlyTagCollection.cs index 73102b35546..423ccc1f1a6 100644 --- a/src/OpenTelemetry/ReadOnlyTagCollection.cs +++ b/src/OpenTelemetry/ReadOnlyTagCollection.cs @@ -1,8 +1,6 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -using System.Diagnostics; - namespace OpenTelemetry; /// @@ -13,31 +11,16 @@ namespace OpenTelemetry; public readonly struct ReadOnlyTagCollection { internal readonly KeyValuePair[] KeyAndValues; - private readonly HashSet? keyFilter; - private readonly int count; internal ReadOnlyTagCollection(KeyValuePair[]? keyAndValues) { this.KeyAndValues = keyAndValues ?? Array.Empty>(); - this.keyFilter = null; - this.count = this.KeyAndValues.Length; - } - - internal ReadOnlyTagCollection(HashSet keyFilter, KeyValuePair[] keyAndValues, int count) - { - Debug.Assert(keyFilter != null, "keyFilter was null"); - Debug.Assert(keyAndValues != null, "keyAndValues was null"); - Debug.Assert(count <= keyAndValues.Length, "count was invalid"); - - this.keyFilter = keyFilter; - this.KeyAndValues = keyAndValues; - this.count = count; } /// /// Gets the number of tags in the collection. /// - public int Count => this.count; + public int Count => this.KeyAndValues.Length; /// /// Returns an enumerator that iterates through the tags. @@ -64,7 +47,7 @@ internal Enumerator(ReadOnlyTagCollection source) /// /// Gets the tag at the current position of the enumerator. /// - public KeyValuePair Current { get; private set; } + public KeyValuePair Current { readonly get; private set; } /// /// Advances the enumerator to the next element of the public bool MoveNext() { - while (true) - { - int index = this.index; - if (index < this.source.Count) - { - var item = this.source.KeyAndValues[index++]; - this.index = index; + int index = this.index; - if (this.source.keyFilter?.Contains(item.Key) == true) - { - continue; - } - - this.Current = item; - return true; - } + if (index < this.source.Count) + { + this.Current = this.source.KeyAndValues[index]; - break; + this.index++; + return true; } return false; diff --git a/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs index 476976e4611..0e2e8c75cdf 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs @@ -164,12 +164,14 @@ public void TestExemplarsFilterTags() Assert.True(metricPoint.Value.StartTime >= testStartTime); Assert.True(metricPoint.Value.EndTime != default); var exemplars = GetExemplars(metricPoint.Value); - Assert.NotNull(exemplars); foreach (var exemplar in exemplars) { - Assert.NotNull(exemplar.FilteredTags); - Assert.Contains(new("key2", "value1"), exemplar.FilteredTags); - Assert.Contains(new("key3", "value1"), exemplar.FilteredTags); + Assert.NotEqual(0, exemplar.FilteredTags.MaximumCount); + + var filteredTags = exemplar.FilteredTags.ToReadOnlyList(); + + Assert.Contains(new("key2", "value1"), filteredTags); + Assert.Contains(new("key3", "value1"), filteredTags); } } @@ -185,14 +187,13 @@ private static double[] GenerateRandomValues(int count) return values; } - private static void ValidateExemplars(Exemplar[] exemplars, DateTimeOffset startTime, DateTimeOffset endTime, double[] measurementValues, bool traceContextExists) + private static void ValidateExemplars(ReadOnlyExemplarCollection exemplars, DateTimeOffset startTime, DateTimeOffset endTime, double[] measurementValues, bool traceContextExists) { - Assert.NotNull(exemplars); foreach (var exemplar in exemplars) { Assert.True(exemplar.Timestamp >= startTime && exemplar.Timestamp <= endTime, $"{startTime} < {exemplar.Timestamp} < {endTime}"); Assert.Contains(exemplar.DoubleValue, measurementValues); - Assert.Null(exemplar.FilteredTags); + Assert.Equal(0, exemplar.FilteredTags.MaximumCount); if (traceContextExists) { Assert.NotEqual(default, exemplar.TraceId); diff --git a/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs index 0e5c1e1e53f..ad2d96c5b3b 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs @@ -233,9 +233,14 @@ public IDisposable BuildMeterProvider( #endif } - internal static Exemplar[] GetExemplars(MetricPoint mp) + internal static ReadOnlyExemplarCollection GetExemplars(MetricPoint mp) { - return mp.GetExemplars().Where(exemplar => exemplar.Timestamp != default).ToArray(); + if (mp.TryGetExemplars(out var exemplars)) + { + return exemplars.Value; + } + + return new ReadOnlyExemplarCollection(Array.Empty()); } #if BUILDING_HOSTING_TESTS From c23e7bf641aed90ee2f4598d8614263b0faec94c Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 13 Feb 2024 16:31:41 -0800 Subject: [PATCH 03/29] Tweak. --- .../ConsoleMetricExporter.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs index 939990d6955..5a6b4359ee3 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs @@ -192,8 +192,17 @@ public override ExportResult Export(in Batch batch) { foreach (var exemplar in exemplars) { - exemplarString.Append("Value: "); - exemplarString.Append(exemplar.DoubleValue); + if (metricType.IsDouble()) + { + exemplarString.Append("Value: "); + exemplarString.Append(exemplar.DoubleValue); + } + else if (metricType.IsLong()) + { + exemplarString.Append("Value: "); + exemplarString.Append(exemplar.LongValue); + } + exemplarString.Append(" Timestamp: "); exemplarString.Append(exemplar.Timestamp.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture)); exemplarString.Append(" TraceId: "); From feb1c7ee5f5f1867180d9263d702b30c347351ea Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 13 Feb 2024 16:46:59 -0800 Subject: [PATCH 04/29] Improvements. --- .../Exemplar/FixedSizeExemplarReservoir.cs | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index 98e4e8bbcca..1c866665270 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -9,7 +9,7 @@ internal abstract class FixedSizeExemplarReservoir : ExemplarReservoir { private Exemplar[] bufferA = Array.Empty(); private Exemplar[] bufferB = Array.Empty(); - private Exemplar[] activeBuffer = Array.Empty(); + private Exemplar[]? activeBuffer; protected FixedSizeExemplarReservoir(int exemplarCount) { @@ -26,19 +26,25 @@ public override ReadOnlyExemplarCollection Collect() { var currentBuffer = this.activeBuffer; - this.activeBuffer = currentBuffer == this.bufferA + Debug.Assert(currentBuffer != null, "currentBuffer was null"); + + this.activeBuffer = null; + + var inactiveBuffer = currentBuffer == this.bufferA ? this.bufferB : this.bufferA; if (this.ResetOnCollect) { - for (int i = 0; i < this.activeBuffer.Length; i++) + for (int i = 0; i < inactiveBuffer.Length; i++) { - this.activeBuffer[i].Reset(); + inactiveBuffer[i].Reset(); } } - return new(currentBuffer); + this.activeBuffer = inactiveBuffer; + + return new(currentBuffer!); } internal sealed override void Initialize(AggregatorStore aggregatorStore) @@ -56,10 +62,17 @@ internal sealed override void Initialize(AggregatorStore aggregatorStore) base.Initialize(aggregatorStore); } - internal void UpdateExemplar(int exemplarIndex, in ExemplarMeasurement measurement) + protected void UpdateExemplar(int exemplarIndex, in ExemplarMeasurement measurement) where T : struct { - ref var exemplar = ref this.activeBuffer[exemplarIndex]; + var activeBuffer = this.activeBuffer; + if (activeBuffer == null) + { + // Note: This is expected to happen when we race with a collection. + return; + } + + ref var exemplar = ref activeBuffer[exemplarIndex]; exemplar.Timestamp = DateTimeOffset.UtcNow; From 4e97f1097c165e81502b74cce18ab9432a650d32 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 13 Feb 2024 16:52:36 -0800 Subject: [PATCH 05/29] API updates. --- .../Experimental/PublicAPI.Unshipped.txt | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt index 16832495101..4aa4aee2591 100644 --- a/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt @@ -12,16 +12,37 @@ OpenTelemetry.Metrics.AlwaysOnExemplarFilter.AlwaysOnExemplarFilter() -> void OpenTelemetry.Metrics.Exemplar OpenTelemetry.Metrics.Exemplar.DoubleValue.get -> double OpenTelemetry.Metrics.Exemplar.Exemplar() -> void +OpenTelemetry.Metrics.Exemplar.FilteredTags.get -> OpenTelemetry.ReadOnlyFilteredTagCollection +OpenTelemetry.Metrics.Exemplar.LongValue.get -> long OpenTelemetry.Metrics.Exemplar.SpanId.get -> System.Diagnostics.ActivitySpanId? OpenTelemetry.Metrics.Exemplar.Timestamp.get -> System.DateTimeOffset OpenTelemetry.Metrics.Exemplar.TraceId.get -> System.Diagnostics.ActivityTraceId? OpenTelemetry.Metrics.ExemplarFilter OpenTelemetry.Metrics.ExemplarFilter.ExemplarFilter() -> void -OpenTelemetry.Metrics.MetricPoint.GetExemplars() -> OpenTelemetry.Metrics.Exemplar[]! +OpenTelemetry.Metrics.ExemplarMeasurement +OpenTelemetry.Metrics.ExemplarMeasurement.ExemplarMeasurement() -> void +OpenTelemetry.Metrics.ExemplarMeasurement.Tags.get -> System.ReadOnlySpan> +OpenTelemetry.Metrics.ExemplarMeasurement.Value.get -> T +OpenTelemetry.Metrics.MetricPoint.TryGetExemplars(out OpenTelemetry.Metrics.ReadOnlyExemplarCollection? exemplars) -> bool OpenTelemetry.Metrics.MetricStreamConfiguration.CardinalityLimit.get -> int? OpenTelemetry.Metrics.MetricStreamConfiguration.CardinalityLimit.set -> void +OpenTelemetry.Metrics.ReadOnlyExemplarCollection +OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator +OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator.Current.get -> OpenTelemetry.Metrics.Exemplar +OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator.Enumerator() -> void +OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator.MoveNext() -> bool +OpenTelemetry.Metrics.ReadOnlyExemplarCollection.GetEnumerator() -> OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator +OpenTelemetry.Metrics.ReadOnlyExemplarCollection.ReadOnlyExemplarCollection() -> void OpenTelemetry.Metrics.TraceBasedExemplarFilter OpenTelemetry.Metrics.TraceBasedExemplarFilter.TraceBasedExemplarFilter() -> void +OpenTelemetry.ReadOnlyFilteredTagCollection +OpenTelemetry.ReadOnlyFilteredTagCollection.Enumerator +OpenTelemetry.ReadOnlyFilteredTagCollection.Enumerator.Current.get -> System.Collections.Generic.KeyValuePair +OpenTelemetry.ReadOnlyFilteredTagCollection.Enumerator.Enumerator() -> void +OpenTelemetry.ReadOnlyFilteredTagCollection.Enumerator.MoveNext() -> bool +OpenTelemetry.ReadOnlyFilteredTagCollection.GetEnumerator() -> OpenTelemetry.ReadOnlyFilteredTagCollection.Enumerator +OpenTelemetry.ReadOnlyFilteredTagCollection.MaximumCount.get -> int +OpenTelemetry.ReadOnlyFilteredTagCollection.ReadOnlyFilteredTagCollection() -> void static OpenTelemetry.Logs.LoggerProviderBuilderExtensions.AddProcessor(this OpenTelemetry.Logs.LoggerProviderBuilder! loggerProviderBuilder, OpenTelemetry.BaseProcessor! processor) -> OpenTelemetry.Logs.LoggerProviderBuilder! static OpenTelemetry.Logs.LoggerProviderBuilderExtensions.AddProcessor(this OpenTelemetry.Logs.LoggerProviderBuilder! loggerProviderBuilder, System.Func!>! implementationFactory) -> OpenTelemetry.Logs.LoggerProviderBuilder! static OpenTelemetry.Logs.LoggerProviderBuilderExtensions.AddProcessor(this OpenTelemetry.Logs.LoggerProviderBuilder! loggerProviderBuilder) -> OpenTelemetry.Logs.LoggerProviderBuilder! @@ -38,7 +59,6 @@ static OpenTelemetry.OpenTelemetryBuilderSdkExtensions.WithLogging(this OpenTele static OpenTelemetry.Sdk.CreateLoggerProviderBuilder() -> OpenTelemetry.Logs.LoggerProviderBuilder! abstract OpenTelemetry.Metrics.ExemplarFilter.ShouldSample(double value, System.ReadOnlySpan> tags) -> bool abstract OpenTelemetry.Metrics.ExemplarFilter.ShouldSample(long value, System.ReadOnlySpan> tags) -> bool -OpenTelemetry.Metrics.Exemplar.FilteredTags.get -> System.Collections.Generic.List>? override OpenTelemetry.Metrics.AlwaysOffExemplarFilter.ShouldSample(double value, System.ReadOnlySpan> tags) -> bool override OpenTelemetry.Metrics.AlwaysOffExemplarFilter.ShouldSample(long value, System.ReadOnlySpan> tags) -> bool override OpenTelemetry.Metrics.AlwaysOnExemplarFilter.ShouldSample(double value, System.ReadOnlySpan> tags) -> bool From 05e058ab2db06c17305e5c89534afb77980cd2d3 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 13 Feb 2024 17:02:06 -0800 Subject: [PATCH 06/29] Tweak. --- src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs index 5a6b4359ee3..7de49ea0fd7 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs @@ -190,7 +190,7 @@ public override ExportResult Export(in Batch batch) var exemplarString = new StringBuilder(); if (metricPoint.TryGetExemplars(out var exemplars)) { - foreach (var exemplar in exemplars) + foreach (ref readonly var exemplar in exemplars) { if (metricType.IsDouble()) { @@ -250,7 +250,7 @@ public override ExportResult Export(in Batch batch) { msg.AppendLine(); msg.AppendLine("Exemplars"); - msg.Append(exemplarString.ToString()); + msg.Append(exemplarString); } this.WriteLine(msg.ToString()); From 4542e83a249bed3de9c9c9a31914fe2135d69a10 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 13 Feb 2024 17:36:06 -0800 Subject: [PATCH 07/29] Tweaks. --- OpenTelemetry.sln | 1 + .../Exemplar/FixedSizeExemplarReservoir.cs | 8 +++- .../SimpleFixedSizeExemplarReservoir.cs | 26 +++++-------- src/OpenTelemetry/OpenTelemetry.csproj | 1 + src/Shared/ThreadSafeRandom.cs | 37 +++++++++++++++++++ 5 files changed, 55 insertions(+), 18 deletions(-) create mode 100644 src/Shared/ThreadSafeRandom.cs diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln index 0b9de051d0d..1e6cf7cf51a 100644 --- a/OpenTelemetry.sln +++ b/OpenTelemetry.sln @@ -278,6 +278,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shared", "Shared", "{A49299 src\Shared\TagAndValueTransformer.cs = src\Shared\TagAndValueTransformer.cs src\Shared\TagTransformer.cs = src\Shared\TagTransformer.cs src\Shared\TagTransformerJsonHelper.cs = src\Shared\TagTransformerJsonHelper.cs + src\Shared\ThreadSafeRandom.cs = src\Shared\ThreadSafeRandom.cs EndProjectSection EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "DiagnosticSourceInstrumentation", "DiagnosticSourceInstrumentation", "{28F3EC79-660C-4659-8B73-F90DC1173316}" diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index 1c866665270..446ede77b09 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -22,7 +22,7 @@ protected FixedSizeExemplarReservoir(int exemplarCount) /// Collects all the exemplars accumulated by the Reservoir. /// /// . - public override ReadOnlyExemplarCollection Collect() + public sealed override ReadOnlyExemplarCollection Collect() { var currentBuffer = this.activeBuffer; @@ -42,6 +42,8 @@ public override ReadOnlyExemplarCollection Collect() } } + this.OnCollectionCompleted(); + this.activeBuffer = inactiveBuffer; return new(currentBuffer!); @@ -62,6 +64,10 @@ internal sealed override void Initialize(AggregatorStore aggregatorStore) base.Initialize(aggregatorStore); } + protected virtual void OnCollectionCompleted() + { + } + protected void UpdateExemplar(int exemplarIndex, in ExemplarMeasurement measurement) where T : struct { diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs index 4852fe07995..e031b88bac2 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs @@ -1,6 +1,8 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +using OpenTelemetry.Internal; + namespace OpenTelemetry.Metrics; /// @@ -8,15 +10,12 @@ namespace OpenTelemetry.Metrics; /// internal sealed class SimpleFixedSizeExemplarReservoir : FixedSizeExemplarReservoir { - private readonly Random random; - private int measurementsSeen; public SimpleFixedSizeExemplarReservoir(int poolSize) : base(poolSize) { - this.measurementsSeen = 0; - this.random = new Random(); + this.measurementsSeen = -1; } public override void Offer(in ExemplarMeasurement measurement) @@ -29,37 +28,30 @@ public override void Offer(in ExemplarMeasurement measurement) this.Offer(in measurement); } - public override ReadOnlyExemplarCollection Collect() + protected override void OnCollectionCompleted() { // Reset internal state irrespective of temporality. // This ensures incoming measurements have fair chance // of making it to the reservoir. this.measurementsSeen = 0; - - return base.Collect(); } private void Offer(in ExemplarMeasurement measurement) where T : struct { - if (this.measurementsSeen < this.ExemplarCount) + var currentMeasurement = Interlocked.Increment(ref this.measurementsSeen); + + if (currentMeasurement < this.ExemplarCount) { - this.UpdateExemplar(this.measurementsSeen, in measurement); + this.UpdateExemplar(currentMeasurement, in measurement); } else { - int index; - lock (this.random) - { - index = this.random.Next(0, this.measurementsSeen); - } - + int index = ThreadSafeRandom.Next(0, currentMeasurement); if (index < this.ExemplarCount) { this.UpdateExemplar(index, in measurement); } } - - Interlocked.Increment(ref this.measurementsSeen); } } diff --git a/src/OpenTelemetry/OpenTelemetry.csproj b/src/OpenTelemetry/OpenTelemetry.csproj index fb1cee1bcd3..0f73d404b4b 100644 --- a/src/OpenTelemetry/OpenTelemetry.csproj +++ b/src/OpenTelemetry/OpenTelemetry.csproj @@ -20,6 +20,7 @@ + diff --git a/src/Shared/ThreadSafeRandom.cs b/src/Shared/ThreadSafeRandom.cs new file mode 100644 index 00000000000..1ba1b4d91c7 --- /dev/null +++ b/src/Shared/ThreadSafeRandom.cs @@ -0,0 +1,37 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +namespace OpenTelemetry.Internal; + +// Note: Inspired by https://devblogs.microsoft.com/pfxteam/getting-random-numbers-in-a-thread-safe-way/ +internal static class ThreadSafeRandom +{ +#if NET6_0_OR_GREATER + public static int Next(int min, int max) + { + return Random.Shared.Next(min, max); + } +#else + private static readonly Random GlobalRandom = new(); + + [ThreadStatic] + private static Random? localRandom; + + public static int Next(int min, int max) + { + var local = localRandom; + if (local == null) + { + int seed; + lock (GlobalRandom) + { + seed = GlobalRandom.Next(); + } + + localRandom = local = new Random(seed); + } + + return local.Next(min, max); + } +#endif +} From 82e6fa68c1a08599ed23c2a8a23f37eca32987f3 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 13 Feb 2024 22:19:57 -0800 Subject: [PATCH 08/29] Better concurrency. Bug fixes. --- .../Experimental/PublicAPI.Unshipped.txt | 1 + src/OpenTelemetry/Metrics/AggregatorStore.cs | 34 +- .../Metrics/Exemplar/Exemplar.cs | 111 +++- .../Exemplar/FixedSizeExemplarReservoir.cs | 53 +- .../Exemplar/ReadOnlyExemplarCollection.cs | 23 +- .../SimpleFixedSizeExemplarReservoir.cs | 10 +- src/OpenTelemetry/Metrics/MetricPoint.cs | 590 +----------------- 7 files changed, 176 insertions(+), 646 deletions(-) diff --git a/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt index 4aa4aee2591..c6122728cd3 100644 --- a/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt @@ -32,6 +32,7 @@ OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator.Current.get -> OpenT OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator.Enumerator() -> void OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator.MoveNext() -> bool OpenTelemetry.Metrics.ReadOnlyExemplarCollection.GetEnumerator() -> OpenTelemetry.Metrics.ReadOnlyExemplarCollection.Enumerator +OpenTelemetry.Metrics.ReadOnlyExemplarCollection.MaximumCount.get -> int OpenTelemetry.Metrics.ReadOnlyExemplarCollection.ReadOnlyExemplarCollection() -> void OpenTelemetry.Metrics.TraceBasedExemplarFilter OpenTelemetry.Metrics.TraceBasedExemplarFilter.TraceBasedExemplarFilter() -> void diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index fa4cefc7221..3aa3f896c68 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -188,7 +188,7 @@ internal void SnapshotDelta(int indexSnapshot) if (this.IsExemplarEnabled()) { - metricPoint.TakeSnapshotWithExemplar(outputDelta: true); + metricPoint.TakeSnapshotAndCollectExemplars(outputDelta: true); } else { @@ -213,7 +213,7 @@ internal void SnapshotDeltaWithMetricPointReclaim() { if (this.IsExemplarEnabled()) { - metricPointWithNoTags.TakeSnapshotWithExemplar(outputDelta: true); + metricPointWithNoTags.TakeSnapshotAndCollectExemplars(outputDelta: true); } else { @@ -236,7 +236,7 @@ internal void SnapshotDeltaWithMetricPointReclaim() { if (this.IsExemplarEnabled()) { - metricPointForOverflow.TakeSnapshotWithExemplar(outputDelta: true); + metricPointForOverflow.TakeSnapshotAndCollectExemplars(outputDelta: true); } else { @@ -301,7 +301,7 @@ internal void SnapshotDeltaWithMetricPointReclaim() if (this.IsExemplarEnabled()) { - metricPoint.TakeSnapshotWithExemplar(outputDelta: true); + metricPoint.TakeSnapshotAndCollectExemplars(outputDelta: true); } else { @@ -330,7 +330,7 @@ internal void SnapshotCumulative(int indexSnapshot) if (this.IsExemplarEnabled()) { - metricPoint.TakeSnapshotWithExemplar(outputDelta: false); + metricPoint.TakeSnapshotAndCollectExemplars(outputDelta: false); } else { @@ -946,10 +946,10 @@ private void UpdateLong(long value, ReadOnlySpan> } // TODO: can special case built-in filters to be bit faster. - if (this.IsExemplarEnabled()) + if (this.IsExemplarEnabled() + && this.exemplarFilter.ShouldSample(value, tags)) { - var shouldSample = this.exemplarFilter.ShouldSample(value, tags); - this.metricPoints[index].UpdateWithExemplar(value, tags: default, shouldSample); + this.metricPoints[index].UpdateAndOfferExemplar(value, tags: default); } else { @@ -990,10 +990,10 @@ private void UpdateLongCustomTags(long value, ReadOnlySpan[]? tagStorage; private MetricPointValueStorage valueStorage; + private volatile int isCriticalSectionOccupied; /// /// Gets the timestamp. /// - public DateTimeOffset Timestamp { get; internal set; } + public DateTimeOffset Timestamp { get; private set; } /// /// Gets the TraceId. /// - public ActivityTraceId? TraceId { get; internal set; } + public ActivityTraceId? TraceId { get; private set; } /// /// Gets the SpanId. /// - public ActivitySpanId? SpanId { get; internal set; } + public ActivitySpanId? SpanId { get; private set; } /// /// Gets the long value. @@ -54,7 +55,7 @@ struct Exemplar public long LongValue { readonly get => this.valueStorage.AsLong; - internal set => this.valueStorage.AsLong = value; + private set => this.valueStorage.AsLong = value; } /// @@ -63,7 +64,7 @@ public long LongValue public double DoubleValue { readonly get => this.valueStorage.AsDouble; - internal set => this.valueStorage.AsDouble = value; + private set => this.valueStorage.AsDouble = value; } /// @@ -72,24 +73,112 @@ public double DoubleValue public readonly ReadOnlyFilteredTagCollection FilteredTags => new(this.KeyFilter, this.tagStorage ?? Array.Empty>(), this.tagCount); - internal void StoreFilteredTags(ReadOnlySpan> tags) + internal void Update(in ExemplarMeasurement measurement) + where T : struct { - this.tagCount = tags.Length; - if (tags.Length == 0) + if (Interlocked.CompareExchange(ref this.isCriticalSectionOccupied, 1, 0) != 0) { + // Some other thread is already writing, abort. return; } - if (this.tagStorage == null || this.tagStorage.Length < this.tagCount) + this.Timestamp = DateTimeOffset.UtcNow; + + if (typeof(T) == typeof(long)) { - this.tagStorage = new KeyValuePair[this.tagCount]; + this.LongValue = (long)(object)measurement.Value; + } + else if (typeof(T) == typeof(double)) + { + this.DoubleValue = (double)(object)measurement.Value; + } + else + { + Debug.Fail("Invalid value type"); + this.DoubleValue = Convert.ToDouble(measurement.Value); } - tags.CopyTo(this.tagStorage); + var currentActivity = Activity.Current; + if (currentActivity != null) + { + this.TraceId = currentActivity.TraceId; + this.SpanId = currentActivity.SpanId; + } + else + { + this.TraceId = default; + this.SpanId = default; + } + + this.StoreRawTags(measurement.Tags); + + this.isCriticalSectionOccupied = 0; } internal void Reset() { this.Timestamp = default; } + + internal readonly bool IsUpdated() + { + if (this.Timestamp == default) + { + return false; + } + + if (this.isCriticalSectionOccupied != 0) + { + this.WaitIfUpdatingRare(); + } + + return true; + } + + internal readonly void WaitIfUpdatingRare() + { + var spinWait = default(SpinWait); + while (true) + { + spinWait.SpinOnce(); + + if (this.isCriticalSectionOccupied == 0) + { + return; + } + } + } + + internal readonly void Copy(ref Exemplar destination) + { + destination.Timestamp = this.Timestamp; + destination.TraceId = this.TraceId; + destination.SpanId = this.SpanId; + destination.valueStorage = this.valueStorage; + destination.KeyFilter = this.KeyFilter; + destination.tagCount = this.tagCount; + if (destination.tagCount > 0) + { + Debug.Assert(this.tagStorage != null, "tagStorage was null"); + + destination.tagStorage = new KeyValuePair[destination.tagCount]; + Array.Copy(this.tagStorage!, 0, destination.tagStorage, 0, destination.tagCount); + } + } + + private void StoreRawTags(ReadOnlySpan> tags) + { + this.tagCount = tags.Length; + if (tags.Length == 0) + { + return; + } + + if (this.tagStorage == null || this.tagStorage.Length < this.tagCount) + { + this.tagStorage = new KeyValuePair[this.tagCount]; + } + + tags.CopyTo(this.tagStorage); + } } diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index 446ede77b09..abbc8fba386 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -7,12 +7,15 @@ namespace OpenTelemetry.Metrics; internal abstract class FixedSizeExemplarReservoir : ExemplarReservoir { - private Exemplar[] bufferA = Array.Empty(); - private Exemplar[] bufferB = Array.Empty(); - private Exemplar[]? activeBuffer; + private readonly Exemplar[] bufferA; + private readonly Exemplar[] bufferB; + private volatile Exemplar[]? activeBuffer; protected FixedSizeExemplarReservoir(int exemplarCount) { + this.bufferA = new Exemplar[exemplarCount]; + this.bufferB = new Exemplar[exemplarCount]; + this.activeBuffer = this.bufferA; this.ExemplarCount = exemplarCount; } @@ -51,14 +54,14 @@ public sealed override ReadOnlyExemplarCollection Collect() internal sealed override void Initialize(AggregatorStore aggregatorStore) { - this.bufferA = new Exemplar[this.ExemplarCount]; - this.bufferB = new Exemplar[this.ExemplarCount]; - this.activeBuffer = this.bufferA; + var keyFilter = aggregatorStore.TagKeysInteresting; - for (int i = 0; i < this.ExemplarCount; i++) + for (int a = 0, b = 0; + a < this.bufferA.Length && b < this.bufferB.Length; + a++, b++) { - this.bufferA[i].KeyFilter = aggregatorStore.TagKeysInteresting; - this.bufferB[i].KeyFilter = aggregatorStore.TagKeysInteresting; + this.bufferA[a].KeyFilter = keyFilter; + this.bufferB[b].KeyFilter = keyFilter; } base.Initialize(aggregatorStore); @@ -78,36 +81,6 @@ protected void UpdateExemplar(int exemplarIndex, in ExemplarMeasurement me return; } - ref var exemplar = ref activeBuffer[exemplarIndex]; - - exemplar.Timestamp = DateTimeOffset.UtcNow; - - if (typeof(T) == typeof(long)) - { - exemplar.LongValue = (long)(object)measurement.Value; - } - else if (typeof(T) == typeof(double)) - { - exemplar.DoubleValue = (double)(object)measurement.Value; - } - else - { - Debug.Fail("Invalid value type"); - exemplar.DoubleValue = Convert.ToDouble(measurement.Value); - } - - var currentActivity = Activity.Current; - if (currentActivity != null) - { - exemplar.TraceId = currentActivity.TraceId; - exemplar.SpanId = currentActivity.SpanId; - } - else - { - exemplar.TraceId = default; - exemplar.SpanId = default; - } - - exemplar.StoreFilteredTags(measurement.Tags); + activeBuffer[exemplarIndex].Update(in measurement); } } diff --git a/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs b/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs index 081501ab8cd..ceff5d21696 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs @@ -32,6 +32,15 @@ internal ReadOnlyExemplarCollection(Exemplar[] exemplars) this.exemplars = exemplars!; } + /// + /// Gets the maximum number of s in the collection. + /// + /// + /// Note: Enumerating the collection may return fewer results depending on + /// which s in the collection received updates. + /// + public int MaximumCount => this.exemplars.Length; + /// /// Returns an enumerator that iterates through the s. /// @@ -41,9 +50,15 @@ public Enumerator GetEnumerator() internal ReadOnlyExemplarCollection Copy() { - var copy = new Exemplar[this.exemplars.Length]; - this.exemplars.CopyTo(copy, 0); - return new ReadOnlyExemplarCollection(copy); + var exemplarCopies = new Exemplar[this.exemplars.Length]; + + int i = 0; + foreach (ref readonly var exemplar in this) + { + exemplar.Copy(ref exemplarCopies[i++]); + } + + return new ReadOnlyExemplarCollection(exemplarCopies); } /// @@ -81,7 +96,7 @@ public bool MoveNext() var index = ++this.index; if (index < this.exemplars.Length) { - if (this.exemplars[index].Timestamp == default) + if (!this.exemplars[index].IsUpdated()) { continue; } diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs index e031b88bac2..177ed02e379 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs @@ -12,10 +12,10 @@ internal sealed class SimpleFixedSizeExemplarReservoir : FixedSizeExemplarReserv { private int measurementsSeen; - public SimpleFixedSizeExemplarReservoir(int poolSize) - : base(poolSize) + public SimpleFixedSizeExemplarReservoir() + : base(Environment.ProcessorCount) { - this.measurementsSeen = -1; + this.OnCollectionCompleted(); } public override void Offer(in ExemplarMeasurement measurement) @@ -33,13 +33,13 @@ protected override void OnCollectionCompleted() // Reset internal state irrespective of temporality. // This ensures incoming measurements have fair chance // of making it to the reservoir. - this.measurementsSeen = 0; + Interlocked.Exchange(ref this.measurementsSeen, 0); } private void Offer(in ExemplarMeasurement measurement) where T : struct { - var currentMeasurement = Interlocked.Increment(ref this.measurementsSeen); + var currentMeasurement = Interlocked.Increment(ref this.measurementsSeen) - 1; if (currentMeasurement < this.ExemplarCount) { diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index 7842c13b650..7e90b112f9c 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -24,8 +24,6 @@ public struct MetricPoint // that its using is already reclaimed, this helps avoid sorting of the tags for adding a new Dictionary entry. internal LookupData? LookupData; - private const int DefaultSimpleReservoirPoolSize = 1; - private readonly AggregatorStore aggregatorStore; private readonly AggregationType aggType; @@ -92,7 +90,7 @@ internal MetricPoint( if (aggregatorStore!.IsExemplarEnabled() && reservoir == null) { - reservoir = new SimpleFixedSizeExemplarReservoir(DefaultSimpleReservoirPoolSize); + reservoir = new SimpleFixedSizeExemplarReservoir(); } if (reservoir != null) @@ -447,132 +445,14 @@ internal void Update(long number) } } - internal void UpdateWithExemplar(long number, ReadOnlySpan> tags, bool isSampled) + internal void UpdateAndOfferExemplar(long number, ReadOnlySpan> tags) { - Debug.Assert(this.mpComponents != null, "this.mpComponents was null"); - - switch (this.aggType) - { - case AggregationType.LongSumIncomingDelta: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - unchecked - { - this.runningValue.AsLong += number; - } - - if (isSampled) - { - Debug.Assert(this.mpComponents.ExemplarReservoir != null, "ExemplarReservoir was null"); - - // TODO: Need to ensure that the lock is always released. - // A custom implementation of `ExemplarReservoir.Offer` might throw an exception. - this.mpComponents.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags)); - } - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.LongSumIncomingCumulative: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - this.runningValue.AsLong = number; - - if (isSampled) - { - Debug.Assert(this.mpComponents.ExemplarReservoir != null, "ExemplarReservoir was null"); - - // TODO: Need to ensure that the lock is always released. - // A custom implementation of `ExemplarReservoir.Offer` might throw an exception. - this.mpComponents.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags)); - } - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.LongGauge: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - this.runningValue.AsLong = number; - - if (isSampled) - { - Debug.Assert(this.mpComponents.ExemplarReservoir != null, "ExemplarReservoir was null"); - - // TODO: Need to ensure that the lock is always released. - // A custom implementation of `ExemplarReservoir.Offer` might throw an exception. - this.mpComponents.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags)); - } - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.Histogram: - { - this.UpdateHistogram((double)number, tags, reportExemplar: true, isSampled); - break; - } - - case AggregationType.HistogramWithMinMax: - { - this.UpdateHistogramWithMinMax((double)number, tags, reportExemplar: true, isSampled); - break; - } - - case AggregationType.HistogramWithBuckets: - { - this.UpdateHistogramWithBuckets((double)number, tags, reportExemplar: true, isSampled); - break; - } - - case AggregationType.HistogramWithMinMaxBuckets: - { - this.UpdateHistogramWithBucketsAndMinMax((double)number, tags, reportExemplar: true, isSampled); - break; - } + Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); - case AggregationType.Base2ExponentialHistogram: - { - this.UpdateBase2ExponentialHistogram((double)number, tags, true); - break; - } - - case AggregationType.Base2ExponentialHistogramWithMinMax: - { - this.UpdateBase2ExponentialHistogramWithMinMax((double)number, tags, true); - break; - } - } - - // There is a race with Snapshot: - // Update() updates the value - // Snapshot snapshots the value - // Snapshot sets status to NoCollectPending - // Update sets status to CollectPending -- this is not right as the Snapshot - // already included the updated value. - // In the absence of any new Update call until next Snapshot, - // this results in exporting an Update even though - // it had no update. - // TODO: For Delta, this can be mitigated - // by ignoring Zero points - this.MetricPointStatus = MetricPointStatus.CollectPending; + this.Update(number); - if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled) - { - Interlocked.Decrement(ref this.ReferenceCount); - } + this.mpComponents!.ExemplarReservoir!.Offer( + new ExemplarMeasurement(number, tags)); } internal void Update(double number) @@ -671,138 +551,14 @@ internal void Update(double number) } } - internal void UpdateWithExemplar(double number, ReadOnlySpan> tags, bool isSampled) + internal void UpdateAndOfferExemplar(double number, ReadOnlySpan> tags) { - Debug.Assert(this.mpComponents != null, "this.mpComponents was null"); + Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); - switch (this.aggType) - { - case AggregationType.DoubleSumIncomingDelta: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - unchecked - { - this.runningValue.AsDouble += number; - } + this.Update(number); - if (isSampled) - { - Debug.Assert(this.mpComponents.ExemplarReservoir != null, "ExemplarReservoir was null"); - - // TODO: Need to ensure that the lock is always released. - // A custom implementation of `ExemplarReservoir.Offer` might throw an exception. - this.mpComponents.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags)); - } - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.DoubleSumIncomingCumulative: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - unchecked - { - this.runningValue.AsDouble = number; - } - - if (isSampled) - { - Debug.Assert(this.mpComponents.ExemplarReservoir != null, "ExemplarReservoir was null"); - - // TODO: Need to ensure that the lock is always released. - // A custom implementation of `ExemplarReservoir.Offer` might throw an exception. - this.mpComponents.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags)); - } - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.DoubleGauge: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - unchecked - { - this.runningValue.AsDouble = number; - } - - if (isSampled) - { - Debug.Assert(this.mpComponents.ExemplarReservoir != null, "ExemplarReservoir was null"); - - // TODO: Need to ensure that the lock is always released. - // A custom implementation of `ExemplarReservoir.Offer` might throw an exception. - this.mpComponents.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags)); - } - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.Histogram: - { - this.UpdateHistogram(number, tags, reportExemplar: true, isSampled); - break; - } - - case AggregationType.HistogramWithMinMax: - { - this.UpdateHistogramWithMinMax(number, tags, reportExemplar: true, isSampled); - break; - } - - case AggregationType.HistogramWithBuckets: - { - this.UpdateHistogramWithBuckets(number, tags, reportExemplar: true, isSampled); - break; - } - - case AggregationType.HistogramWithMinMaxBuckets: - { - this.UpdateHistogramWithBucketsAndMinMax(number, tags, reportExemplar: true, isSampled); - break; - } - - case AggregationType.Base2ExponentialHistogram: - { - this.UpdateBase2ExponentialHistogram(number, tags, true); - break; - } - - case AggregationType.Base2ExponentialHistogramWithMinMax: - { - this.UpdateBase2ExponentialHistogramWithMinMax(number, tags, true); - break; - } - } - - // There is a race with Snapshot: - // Update() updates the value - // Snapshot snapshots the value - // Snapshot sets status to NoCollectPending - // Update sets status to CollectPending -- this is not right as the Snapshot - // already included the updated value. - // In the absence of any new Update call until next Snapshot, - // this results in exporting an Update even though - // it had no update. - // TODO: For Delta, this can be mitigated - // by ignoring Zero points - this.MetricPointStatus = MetricPointStatus.CollectPending; - - if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled) - { - Interlocked.Decrement(ref this.ReferenceCount); - } + this.mpComponents!.ExemplarReservoir!.Offer( + new ExemplarMeasurement(number, tags)); } internal void TakeSnapshot(bool outputDelta) @@ -1087,273 +843,13 @@ internal void TakeSnapshot(bool outputDelta) } } - internal void TakeSnapshotWithExemplar(bool outputDelta) + internal void TakeSnapshotAndCollectExemplars(bool outputDelta) { - Debug.Assert(this.mpComponents != null, "this.mpComponents was null"); - - switch (this.aggType) - { - case AggregationType.LongSumIncomingDelta: - case AggregationType.LongSumIncomingCumulative: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - if (outputDelta) - { - long initValue = this.runningValue.AsLong; - this.snapshotValue.AsLong = initValue - this.deltaLastValue.AsLong; - this.deltaLastValue.AsLong = initValue; - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - } - else - { - this.snapshotValue.AsLong = this.runningValue.AsLong; - } - - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.DoubleSumIncomingDelta: - case AggregationType.DoubleSumIncomingCumulative: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - if (outputDelta) - { - double initValue = this.runningValue.AsDouble; - this.snapshotValue.AsDouble = initValue - this.deltaLastValue.AsDouble; - this.deltaLastValue.AsDouble = initValue; - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - } - else - { - this.snapshotValue.AsDouble = this.runningValue.AsDouble; - } - - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); + Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); + this.TakeSnapshot(outputDelta); - break; - } - - case AggregationType.LongGauge: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - this.snapshotValue.AsLong = this.runningValue.AsLong; - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.DoubleGauge: - { - AcquireLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - this.snapshotValue.AsDouble = this.runningValue.AsDouble; - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); - - ReleaseLock(ref this.mpComponents!.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.HistogramWithBuckets: - { - var histogramBuckets = this.mpComponents!.HistogramBuckets; - - Debug.Assert(histogramBuckets != null, "histogramBuckets was null"); - - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; - - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogramBuckets.RunningSum = 0; - } - - Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); - - for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++) - { - histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i]; - if (outputDelta) - { - histogramBuckets.RunningBucketCounts[i] = 0; - } - } - - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); - - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.Histogram: - { - var histogramBuckets = this.mpComponents!.HistogramBuckets; - - Debug.Assert(histogramBuckets != null, "histogramBuckets was null"); - - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; - - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogramBuckets.RunningSum = 0; - } - - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.HistogramWithMinMaxBuckets: - { - var histogramBuckets = this.mpComponents!.HistogramBuckets; - - Debug.Assert(histogramBuckets != null, "histogramBuckets was null"); - - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; - histogramBuckets.SnapshotMin = histogramBuckets.RunningMin; - histogramBuckets.SnapshotMax = histogramBuckets.RunningMax; - - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogramBuckets.RunningSum = 0; - histogramBuckets.RunningMin = double.PositiveInfinity; - histogramBuckets.RunningMax = double.NegativeInfinity; - } - - Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); - - for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++) - { - histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i]; - if (outputDelta) - { - histogramBuckets.RunningBucketCounts[i] = 0; - } - } - - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.HistogramWithMinMax: - { - var histogramBuckets = this.mpComponents!.HistogramBuckets; - - Debug.Assert(histogramBuckets != null, "histogramBuckets was null"); - - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; - histogramBuckets.SnapshotMin = histogramBuckets.RunningMin; - histogramBuckets.SnapshotMax = histogramBuckets.RunningMax; - - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogramBuckets.RunningSum = 0; - histogramBuckets.RunningMin = double.PositiveInfinity; - histogramBuckets.RunningMax = double.NegativeInfinity; - } - - this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.Base2ExponentialHistogram: - { - var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; - - Debug.Assert(histogram != null, "histogram was null"); - - AcquireLock(ref histogram!.IsCriticalSectionOccupied); - - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogram.SnapshotSum = histogram.RunningSum; - histogram.Snapshot(); - - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogram.RunningSum = 0; - histogram.Reset(); - } - - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - ReleaseLock(ref histogram.IsCriticalSectionOccupied); - - break; - } - - case AggregationType.Base2ExponentialHistogramWithMinMax: - { - var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; - - Debug.Assert(histogram != null, "histogram was null"); - - AcquireLock(ref histogram!.IsCriticalSectionOccupied); - - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogram.SnapshotSum = histogram.RunningSum; - histogram.Snapshot(); - histogram.SnapshotMin = histogram.RunningMin; - histogram.SnapshotMax = histogram.RunningMax; - - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogram.RunningSum = 0; - histogram.Reset(); - histogram.RunningMin = double.PositiveInfinity; - histogram.RunningMax = double.NegativeInfinity; - } - - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - ReleaseLock(ref histogram.IsCriticalSectionOccupied); - - break; - } - } + this.mpComponents!.Exemplars = this.mpComponents.ExemplarReservoir!.Collect(); } private static void AcquireLock(ref int isCriticalSectionOccupied) @@ -1370,7 +866,7 @@ private static void ReleaseLock(ref int isCriticalSectionOccupied) Interlocked.Exchange(ref isCriticalSectionOccupied, 0); } - private void UpdateHistogram(double number, ReadOnlySpan> tags = default, bool reportExemplar = false, bool isSampled = false) + private void UpdateHistogram(double number) { Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); @@ -1384,20 +880,10 @@ private void UpdateHistogram(double number, ReadOnlySpan(number, tags)); - } - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); } - private void UpdateHistogramWithMinMax(double number, ReadOnlySpan> tags = default, bool reportExemplar = false, bool isSampled = false) + private void UpdateHistogramWithMinMax(double number) { Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); @@ -1413,20 +899,10 @@ private void UpdateHistogramWithMinMax(double number, ReadOnlySpan(number, tags)); - } - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); } - private void UpdateHistogramWithBuckets(double number, ReadOnlySpan> tags = default, bool reportExemplar = false, bool isSampled = false) + private void UpdateHistogramWithBuckets(double number) { Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); @@ -1443,22 +919,12 @@ private void UpdateHistogramWithBuckets(double number, ReadOnlySpan(number, tags, explicitBucketHistogramIndex: i)); - } } ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); } - private void UpdateHistogramWithBucketsAndMinMax(double number, ReadOnlySpan> tags = default, bool reportExemplar = false, bool isSampled = false) + private void UpdateHistogramWithBucketsAndMinMax(double number) { Debug.Assert(this.mpComponents?.HistogramBuckets != null, "histogramBuckets was null"); @@ -1476,16 +942,6 @@ private void UpdateHistogramWithBucketsAndMinMax(double number, ReadOnlySpan(number, tags, explicitBucketHistogramIndex: i)); - } - histogramBuckets.RunningMin = Math.Min(histogramBuckets.RunningMin, number); histogramBuckets.RunningMax = Math.Max(histogramBuckets.RunningMax, number); } @@ -1493,9 +949,7 @@ private void UpdateHistogramWithBucketsAndMinMax(double number, ReadOnlySpan> tags = default, bool reportExemplar = false) -#pragma warning restore IDE0060 // Remove unused parameter + private void UpdateBase2ExponentialHistogram(double number) { if (number < 0) { @@ -1518,9 +972,7 @@ private void UpdateBase2ExponentialHistogram(double number, ReadOnlySpan> tags = default, bool reportExemplar = false) -#pragma warning restore IDE0060 // Remove unused parameter + private void UpdateBase2ExponentialHistogramWithMinMax(double number) { if (number < 0) { From 6b20ac378d1a59e118b9ec1a947dbeab091e2899 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 14 Feb 2024 09:49:18 -0800 Subject: [PATCH 09/29] Tweaks. --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 58 +-- .../Metrics/Exemplar/Exemplar.cs | 34 +- src/OpenTelemetry/Metrics/MetricPoint.cs | 407 ++++++++++-------- .../Metrics/AggregatorTestsBase.cs | 102 ++--- 4 files changed, 304 insertions(+), 297 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index 3aa3f896c68..9dc624333ae 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -17,6 +17,7 @@ internal sealed class AggregatorStore internal readonly int CardinalityLimit; internal readonly bool EmitOverflowAttribute; internal readonly ConcurrentDictionary? TagsToMetricPointIndexDictionaryDelta; + internal readonly ExemplarFilter ExemplarFilter; internal long DroppedMeasurements = 0; private static readonly string MetricPointCapHitFixMessage = "Consider opting in for the experimental SDK feature to emit all the throttled metrics under the overflow attribute by setting env variable OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE = true. You could also modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit."; @@ -43,7 +44,6 @@ internal sealed class AggregatorStore private readonly int exponentialHistogramMaxScale; private readonly UpdateLongDelegate updateLongCallback; private readonly UpdateDoubleDelegate updateDoubleCallback; - private readonly ExemplarFilter exemplarFilter; private readonly Func[], int, int> lookupAggregatorStore; private int metricPointIndex = 0; @@ -73,7 +73,7 @@ internal AggregatorStore( this.exponentialHistogramMaxSize = metricStreamIdentity.ExponentialHistogramMaxSize; this.exponentialHistogramMaxScale = metricStreamIdentity.ExponentialHistogramMaxScale; this.StartTimeExclusive = DateTimeOffset.UtcNow; - this.exemplarFilter = exemplarFilter ?? DefaultExemplarFilter; + this.ExemplarFilter = exemplarFilter ?? DefaultExemplarFilter; if (metricStreamIdentity.TagKeys == null) { this.updateLongCallback = this.UpdateLong; @@ -141,7 +141,7 @@ internal bool IsExemplarEnabled() { // Using this filter to indicate On/Off // instead of another separate flag. - return this.exemplarFilter is not AlwaysOffExemplarFilter; + return this.ExemplarFilter is not AlwaysOffExemplarFilter; } internal void Update(long value, ReadOnlySpan> tags) @@ -931,7 +931,7 @@ private void UpdateLong(long value, ReadOnlySpan> if (this.EmitOverflowAttribute) { this.InitializeOverflowTagPointIfNotInitialized(); - this.metricPoints[1].Update(value); + this.metricPoints[1].Update(value, tags: default); return; } else @@ -945,16 +945,7 @@ private void UpdateLong(long value, ReadOnlySpan> } } - // TODO: can special case built-in filters to be bit faster. - if (this.IsExemplarEnabled() - && this.exemplarFilter.ShouldSample(value, tags)) - { - this.metricPoints[index].UpdateAndOfferExemplar(value, tags: default); - } - else - { - this.metricPoints[index].Update(value); - } + this.metricPoints[index].Update(value, tags: default); } catch (Exception) { @@ -975,7 +966,7 @@ private void UpdateLongCustomTags(long value, ReadOnlySpan> tags) tags.CopyTo(this.tagStorage); } + + private readonly void WaitForUpdateToCompleteRare() + { + var spinWait = default(SpinWait); + do + { + spinWait.SpinOnce(); + } + while (this.isCriticalSectionOccupied != 0); + } } diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index 7e90b112f9c..b41412e5f4e 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -367,198 +367,46 @@ internal readonly MetricPoint Copy() return copy; } - internal void Update(long number) + internal void Update(long number, ReadOnlySpan> tags) { - switch (this.aggType) + var measurement = new Measurement { - case AggregationType.LongSumIncomingDelta: - { - Interlocked.Add(ref this.runningValue.AsLong, number); - break; - } - - case AggregationType.LongSumIncomingCumulative: - { - Interlocked.Exchange(ref this.runningValue.AsLong, number); - break; - } + Value = number, + Tags = tags, + }; - case AggregationType.LongGauge: - { - Interlocked.Exchange(ref this.runningValue.AsLong, number); - break; - } + this.Update(ref measurement); - case AggregationType.Histogram: - { - this.UpdateHistogram((double)number); - break; - } - - case AggregationType.HistogramWithMinMax: - { - this.UpdateHistogramWithMinMax((double)number); - break; - } - - case AggregationType.HistogramWithBuckets: - { - this.UpdateHistogramWithBuckets((double)number); - break; - } - - case AggregationType.HistogramWithMinMaxBuckets: - { - this.UpdateHistogramWithBucketsAndMinMax((double)number); - break; - } - - case AggregationType.Base2ExponentialHistogram: - { - this.UpdateBase2ExponentialHistogram((double)number); - break; - } - - case AggregationType.Base2ExponentialHistogramWithMinMax: - { - this.UpdateBase2ExponentialHistogramWithMinMax((double)number); - break; - } - } - - // There is a race with Snapshot: - // Update() updates the value - // Snapshot snapshots the value - // Snapshot sets status to NoCollectPending - // Update sets status to CollectPending -- this is not right as the Snapshot - // already included the updated value. - // In the absence of any new Update call until next Snapshot, - // this results in exporting an Update even though - // it had no update. - // TODO: For Delta, this can be mitigated - // by ignoring Zero points - this.MetricPointStatus = MetricPointStatus.CollectPending; - - if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled) + if (this.ShouldOfferExemplar(ref measurement)) { - Interlocked.Decrement(ref this.ReferenceCount); - } - } + Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); - internal void UpdateAndOfferExemplar(long number, ReadOnlySpan> tags) - { - Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); - - this.Update(number); + this.mpComponents!.ExemplarReservoir!.Offer( + new ExemplarMeasurement(number, tags, measurement.ExplicitBucketHistogramBucketIndex)); + } - this.mpComponents!.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags)); + this.CompleteUpdate(); } - internal void Update(double number) + internal void Update(double number, ReadOnlySpan> tags) { - switch (this.aggType) + var measurement = new Measurement { - case AggregationType.DoubleSumIncomingDelta: - { - double initValue, newValue; - var sw = default(SpinWait); - while (true) - { - initValue = this.runningValue.AsDouble; - - unchecked - { - newValue = initValue + number; - } - - if (initValue == Interlocked.CompareExchange(ref this.runningValue.AsDouble, newValue, initValue)) - { - break; - } - - sw.SpinOnce(); - } - - break; - } + Value = number, + Tags = tags, + }; - case AggregationType.DoubleSumIncomingCumulative: - { - Interlocked.Exchange(ref this.runningValue.AsDouble, number); - break; - } + this.Update(ref measurement); - case AggregationType.DoubleGauge: - { - Interlocked.Exchange(ref this.runningValue.AsDouble, number); - break; - } - - case AggregationType.Histogram: - { - this.UpdateHistogram(number); - break; - } - - case AggregationType.HistogramWithMinMax: - { - this.UpdateHistogramWithMinMax(number); - break; - } - - case AggregationType.HistogramWithBuckets: - { - this.UpdateHistogramWithBuckets(number); - break; - } - - case AggregationType.HistogramWithMinMaxBuckets: - { - this.UpdateHistogramWithBucketsAndMinMax(number); - break; - } - - case AggregationType.Base2ExponentialHistogram: - { - this.UpdateBase2ExponentialHistogram(number); - break; - } - - case AggregationType.Base2ExponentialHistogramWithMinMax: - { - this.UpdateBase2ExponentialHistogramWithMinMax(number); - break; - } - } - - // There is a race with Snapshot: - // Update() updates the value - // Snapshot snapshots the value - // Snapshot sets status to NoCollectPending - // Update sets status to CollectPending -- this is not right as the Snapshot - // already included the updated value. - // In the absence of any new Update call until next Snapshot, - // this results in exporting an Update even though - // it had no update. - // TODO: For Delta, this can be mitigated - // by ignoring Zero points - this.MetricPointStatus = MetricPointStatus.CollectPending; - - if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled) + if (this.ShouldOfferExemplar(ref measurement)) { - Interlocked.Decrement(ref this.ReferenceCount); - } - } + Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); - internal void UpdateAndOfferExemplar(double number, ReadOnlySpan> tags) - { - Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); - - this.Update(number); + this.mpComponents!.ExemplarReservoir!.Offer( + new ExemplarMeasurement(number, tags, measurement.ExplicitBucketHistogramBucketIndex)); + } - this.mpComponents!.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags)); + this.CompleteUpdate(); } internal void TakeSnapshot(bool outputDelta) @@ -866,6 +714,176 @@ private static void ReleaseLock(ref int isCriticalSectionOccupied) Interlocked.Exchange(ref isCriticalSectionOccupied, 0); } + private bool ShouldOfferExemplar(ref Measurement measurement) + where T : struct + { + var exemplarFilter = this.aggregatorStore.ExemplarFilter; + + if (exemplarFilter is AlwaysOffExemplarFilter) + { + return false; + } + else if (exemplarFilter is AlwaysOnExemplarFilter) + { + return true; + } + else if (typeof(T) == typeof(long)) + { + return exemplarFilter.ShouldSample((long)(object)measurement.Value, measurement.Tags); + } + else if (typeof(T) == typeof(double)) + { + return exemplarFilter.ShouldSample((double)(object)measurement.Value, measurement.Tags); + } + else + { + Debug.Fail("Invalid value type"); + return exemplarFilter.ShouldSample(Convert.ToDouble(measurement.Value), measurement.Tags); + } + } + + private void Update(ref Measurement measurement) + { + var number = measurement.Value; + + switch (this.aggType) + { + case AggregationType.LongSumIncomingDelta: + { + Interlocked.Add(ref this.runningValue.AsLong, number); + break; + } + + case AggregationType.LongSumIncomingCumulative: + { + Interlocked.Exchange(ref this.runningValue.AsLong, number); + break; + } + + case AggregationType.LongGauge: + { + Interlocked.Exchange(ref this.runningValue.AsLong, number); + break; + } + + case AggregationType.Histogram: + { + this.UpdateHistogram((double)number); + break; + } + + case AggregationType.HistogramWithMinMax: + { + this.UpdateHistogramWithMinMax((double)number); + break; + } + + case AggregationType.HistogramWithBuckets: + { + measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets((double)number); + break; + } + + case AggregationType.HistogramWithMinMaxBuckets: + { + measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax((double)number); + break; + } + + case AggregationType.Base2ExponentialHistogram: + { + this.UpdateBase2ExponentialHistogram((double)number); + break; + } + + case AggregationType.Base2ExponentialHistogramWithMinMax: + { + this.UpdateBase2ExponentialHistogramWithMinMax((double)number); + break; + } + } + } + + private void Update(ref Measurement measurement) + { + var number = measurement.Value; + + switch (this.aggType) + { + case AggregationType.DoubleSumIncomingDelta: + { + double initValue, newValue; + var sw = default(SpinWait); + while (true) + { + initValue = this.runningValue.AsDouble; + + unchecked + { + newValue = initValue + number; + } + + if (initValue == Interlocked.CompareExchange(ref this.runningValue.AsDouble, newValue, initValue)) + { + break; + } + + sw.SpinOnce(); + } + + break; + } + + case AggregationType.DoubleSumIncomingCumulative: + { + Interlocked.Exchange(ref this.runningValue.AsDouble, number); + break; + } + + case AggregationType.DoubleGauge: + { + Interlocked.Exchange(ref this.runningValue.AsDouble, number); + break; + } + + case AggregationType.Histogram: + { + this.UpdateHistogram(number); + break; + } + + case AggregationType.HistogramWithMinMax: + { + this.UpdateHistogramWithMinMax(number); + break; + } + + case AggregationType.HistogramWithBuckets: + { + measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets(number); + break; + } + + case AggregationType.HistogramWithMinMaxBuckets: + { + measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax(number); + break; + } + + case AggregationType.Base2ExponentialHistogram: + { + this.UpdateBase2ExponentialHistogram(number); + break; + } + + case AggregationType.Base2ExponentialHistogramWithMinMax: + { + this.UpdateBase2ExponentialHistogramWithMinMax(number); + break; + } + } + } + private void UpdateHistogram(double number) { Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); @@ -902,7 +920,7 @@ private void UpdateHistogramWithMinMax(double number) ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); } - private void UpdateHistogramWithBuckets(double number) + private int UpdateHistogramWithBuckets(double number) { Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); @@ -922,9 +940,11 @@ private void UpdateHistogramWithBuckets(double number) } ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + + return i; } - private void UpdateHistogramWithBucketsAndMinMax(double number) + private int UpdateHistogramWithBucketsAndMinMax(double number) { Debug.Assert(this.mpComponents?.HistogramBuckets != null, "histogramBuckets was null"); @@ -947,6 +967,8 @@ private void UpdateHistogramWithBucketsAndMinMax(double number) } ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + + return i; } private void UpdateBase2ExponentialHistogram(double number) @@ -998,9 +1020,38 @@ private void UpdateBase2ExponentialHistogramWithMinMax(double number) ReleaseLock(ref histogram.IsCriticalSectionOccupied); } + private void CompleteUpdate() + { + // There is a race with Snapshot: + // Update() updates the value + // Snapshot snapshots the value + // Snapshot sets status to NoCollectPending + // Update sets status to CollectPending -- this is not right as the Snapshot + // already included the updated value. + // In the absence of any new Update call until next Snapshot, + // this results in exporting an Update even though + // it had no update. + // TODO: For Delta, this can be mitigated + // by ignoring Zero points + this.MetricPointStatus = MetricPointStatus.CollectPending; + + if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled) + { + Interlocked.Decrement(ref this.ReferenceCount); + } + } + [MethodImpl(MethodImplOptions.NoInlining)] private readonly void ThrowNotSupportedMetricTypeException(string methodName) { throw new NotSupportedException($"{methodName} is not supported for this metric type."); } + + private ref struct Measurement + where T : struct + { + public T Value; + public ReadOnlySpan> Tags; + public int ExplicitBucketHistogramBucketIndex; + } } diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs index 61d739d6a18..3c98d970ad3 100644 --- a/test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs +++ b/test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs @@ -31,38 +31,38 @@ protected AggregatorTestsBase(bool emitOverflowAttribute, bool shouldReclaimUnus public void HistogramDistributeToAllBucketsDefault() { var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, Metric.DefaultHistogramBounds, Metric.DefaultExponentialHistogramMaxBuckets, Metric.DefaultExponentialHistogramMaxScale); - histogramPoint.Update(-1); - histogramPoint.Update(0); - histogramPoint.Update(2); - histogramPoint.Update(5); - histogramPoint.Update(8); - histogramPoint.Update(10); - histogramPoint.Update(11); - histogramPoint.Update(25); - histogramPoint.Update(40); - histogramPoint.Update(50); - histogramPoint.Update(70); - histogramPoint.Update(75); - histogramPoint.Update(99); - histogramPoint.Update(100); - histogramPoint.Update(246); - histogramPoint.Update(250); - histogramPoint.Update(499); - histogramPoint.Update(500); - histogramPoint.Update(501); - histogramPoint.Update(750); - histogramPoint.Update(751); - histogramPoint.Update(1000); - histogramPoint.Update(1001); - histogramPoint.Update(2500); - histogramPoint.Update(2501); - histogramPoint.Update(5000); - histogramPoint.Update(5001); - histogramPoint.Update(7500); - histogramPoint.Update(7501); - histogramPoint.Update(10000); - histogramPoint.Update(10001); - histogramPoint.Update(10000000); + histogramPoint.Update(-1, tags: default); + histogramPoint.Update(0, tags: default); + histogramPoint.Update(2, tags: default); + histogramPoint.Update(5, tags: default); + histogramPoint.Update(8, tags: default); + histogramPoint.Update(10, tags: default); + histogramPoint.Update(11, tags: default); + histogramPoint.Update(25, tags: default); + histogramPoint.Update(40, tags: default); + histogramPoint.Update(50, tags: default); + histogramPoint.Update(70, tags: default); + histogramPoint.Update(75, tags: default); + histogramPoint.Update(99, tags: default); + histogramPoint.Update(100, tags: default); + histogramPoint.Update(246, tags: default); + histogramPoint.Update(250, tags: default); + histogramPoint.Update(499, tags: default); + histogramPoint.Update(500, tags: default); + histogramPoint.Update(501, tags: default); + histogramPoint.Update(750, tags: default); + histogramPoint.Update(751, tags: default); + histogramPoint.Update(1000, tags: default); + histogramPoint.Update(1001, tags: default); + histogramPoint.Update(2500, tags: default); + histogramPoint.Update(2501, tags: default); + histogramPoint.Update(5000, tags: default); + histogramPoint.Update(5001, tags: default); + histogramPoint.Update(7500, tags: default); + histogramPoint.Update(7501, tags: default); + histogramPoint.Update(10000, tags: default); + histogramPoint.Update(10001, tags: default); + histogramPoint.Update(10000000, tags: default); histogramPoint.TakeSnapshot(true); var count = histogramPoint.GetHistogramCount(); @@ -84,15 +84,15 @@ public void HistogramDistributeToAllBucketsCustom() var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, boundaries, Metric.DefaultExponentialHistogramMaxBuckets, Metric.DefaultExponentialHistogramMaxScale); // 5 recordings <=10 - histogramPoint.Update(-10); - histogramPoint.Update(0); - histogramPoint.Update(1); - histogramPoint.Update(9); - histogramPoint.Update(10); + histogramPoint.Update(-10, tags: default); + histogramPoint.Update(0, tags: default); + histogramPoint.Update(1, tags: default); + histogramPoint.Update(9, tags: default); + histogramPoint.Update(10, tags: default); // 2 recordings >10, <=20 - histogramPoint.Update(11); - histogramPoint.Update(19); + histogramPoint.Update(11, tags: default); + histogramPoint.Update(19, tags: default); histogramPoint.TakeSnapshot(true); @@ -132,12 +132,12 @@ public void HistogramBinaryBucketTest() var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, boundaries, Metric.DefaultExponentialHistogramMaxBuckets, Metric.DefaultExponentialHistogramMaxScale); // Act - histogramPoint.Update(-1); - histogramPoint.Update(boundaries[0]); - histogramPoint.Update(boundaries[boundaries.Length - 1]); + histogramPoint.Update(-1, tags: default); + histogramPoint.Update(boundaries[0], tags: default); + histogramPoint.Update(boundaries[boundaries.Length - 1], tags: default); for (var i = 0.5; i < boundaries.Length; i++) { - histogramPoint.Update(i); + histogramPoint.Update(i, tags: default); } histogramPoint.TakeSnapshot(true); @@ -164,13 +164,13 @@ public void HistogramWithOnlySumCount() var boundaries = Array.Empty(); var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.Histogram, null, boundaries, Metric.DefaultExponentialHistogramMaxBuckets, Metric.DefaultExponentialHistogramMaxScale); - histogramPoint.Update(-10); - histogramPoint.Update(0); - histogramPoint.Update(1); - histogramPoint.Update(9); - histogramPoint.Update(10); - histogramPoint.Update(11); - histogramPoint.Update(19); + histogramPoint.Update(-10, tags: default); + histogramPoint.Update(0, tags: default); + histogramPoint.Update(1, tags: default); + histogramPoint.Update(9, tags: default); + histogramPoint.Update(10, tags: default); + histogramPoint.Update(11, tags: default); + histogramPoint.Update(19, tags: default); histogramPoint.TakeSnapshot(true); @@ -501,7 +501,7 @@ private static void HistogramUpdateThread(object obj) for (int i = 0; i < 10; ++i) { - args.HistogramPoint.Update(10); + args.HistogramPoint.Update(10, tags: default); } Interlocked.Increment(ref args.ThreadsFinishedAllUpdatesCount); From 4c849be866eb9f16794109c91e32fdebfe720c02 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 14 Feb 2024 09:56:28 -0800 Subject: [PATCH 10/29] Tweaks. --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 45 +- src/OpenTelemetry/Metrics/MetricPoint.cs | 902 ++++++++++--------- 2 files changed, 458 insertions(+), 489 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index 9dc624333ae..b2f6c2219fa 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -186,14 +186,7 @@ internal void SnapshotDelta(int indexSnapshot) continue; } - if (this.IsExemplarEnabled()) - { - metricPoint.TakeSnapshotAndCollectExemplars(outputDelta: true); - } - else - { - metricPoint.TakeSnapshot(outputDelta: true); - } + metricPoint.TakeSnapshot(outputDelta: true); this.currentMetricPointBatch[this.batchSize] = i; this.batchSize++; @@ -211,14 +204,7 @@ internal void SnapshotDeltaWithMetricPointReclaim() ref var metricPointWithNoTags = ref this.metricPoints[0]; if (metricPointWithNoTags.MetricPointStatus != MetricPointStatus.NoCollectPending) { - if (this.IsExemplarEnabled()) - { - metricPointWithNoTags.TakeSnapshotAndCollectExemplars(outputDelta: true); - } - else - { - metricPointWithNoTags.TakeSnapshot(outputDelta: true); - } + metricPointWithNoTags.TakeSnapshot(outputDelta: true); this.currentMetricPointBatch[this.batchSize] = 0; this.batchSize++; @@ -234,14 +220,7 @@ internal void SnapshotDeltaWithMetricPointReclaim() ref var metricPointForOverflow = ref this.metricPoints[1]; if (metricPointForOverflow.MetricPointStatus != MetricPointStatus.NoCollectPending) { - if (this.IsExemplarEnabled()) - { - metricPointForOverflow.TakeSnapshotAndCollectExemplars(outputDelta: true); - } - else - { - metricPointForOverflow.TakeSnapshot(outputDelta: true); - } + metricPointForOverflow.TakeSnapshot(outputDelta: true); this.currentMetricPointBatch[this.batchSize] = 1; this.batchSize++; @@ -299,14 +278,7 @@ internal void SnapshotDeltaWithMetricPointReclaim() continue; } - if (this.IsExemplarEnabled()) - { - metricPoint.TakeSnapshotAndCollectExemplars(outputDelta: true); - } - else - { - metricPoint.TakeSnapshot(outputDelta: true); - } + metricPoint.TakeSnapshot(outputDelta: true); this.currentMetricPointBatch[this.batchSize] = i; this.batchSize++; @@ -328,14 +300,7 @@ internal void SnapshotCumulative(int indexSnapshot) continue; } - if (this.IsExemplarEnabled()) - { - metricPoint.TakeSnapshotAndCollectExemplars(outputDelta: false); - } - else - { - metricPoint.TakeSnapshot(outputDelta: false); - } + metricPoint.TakeSnapshot(outputDelta: false); this.currentMetricPointBatch[this.batchSize] = i; this.batchSize++; diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index b41412e5f4e..f84bd1501a9 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -60,13 +60,15 @@ internal MetricPoint( this.ReferenceCount = 1; this.LookupData = lookupData; + var isExemplarEnabled = aggregatorStore!.IsExemplarEnabled(); + ExemplarReservoir? reservoir = null; if (this.aggType == AggregationType.HistogramWithBuckets || this.aggType == AggregationType.HistogramWithMinMaxBuckets) { this.mpComponents = new MetricPointOptionalComponents(); this.mpComponents.HistogramBuckets = new HistogramBuckets(histogramExplicitBounds); - if (aggregatorStore!.IsExemplarEnabled()) + if (isExemplarEnabled) { reservoir = new AlignedHistogramBucketExemplarReservoir(histogramExplicitBounds!.Length); } @@ -88,7 +90,7 @@ internal MetricPoint( this.mpComponents = null; } - if (aggregatorStore!.IsExemplarEnabled() && reservoir == null) + if (isExemplarEnabled && reservoir == null) { reservoir = new SimpleFixedSizeExemplarReservoir(); } @@ -411,633 +413,635 @@ internal void Update(double number, ReadOnlySpan> internal void TakeSnapshot(bool outputDelta) { + this.Snapshot(outputDelta); + + var exemplarReservoir = this.mpComponents?.ExemplarReservoir; + if (exemplarReservoir != null) + { + this.mpComponents!.Exemplars = this.mpComponents.ExemplarReservoir!.Collect(); + } + } + + private static void AcquireLock(ref int isCriticalSectionOccupied) + { + var sw = default(SpinWait); + while (Interlocked.Exchange(ref isCriticalSectionOccupied, 1) != 0) + { + sw.SpinOnce(); + } + } + + private static void ReleaseLock(ref int isCriticalSectionOccupied) + { + Interlocked.Exchange(ref isCriticalSectionOccupied, 0); + } + + private readonly bool ShouldOfferExemplar(ref Measurement measurement) + where T : struct + { + var exemplarFilter = this.aggregatorStore.ExemplarFilter; + + if (exemplarFilter is AlwaysOffExemplarFilter) + { + return false; + } + else if (exemplarFilter is AlwaysOnExemplarFilter) + { + return true; + } + else if (typeof(T) == typeof(long)) + { + return exemplarFilter.ShouldSample((long)(object)measurement.Value, measurement.Tags); + } + else if (typeof(T) == typeof(double)) + { + return exemplarFilter.ShouldSample((double)(object)measurement.Value, measurement.Tags); + } + else + { + Debug.Fail("Invalid value type"); + return exemplarFilter.ShouldSample(Convert.ToDouble(measurement.Value), measurement.Tags); + } + } + + private void Update(ref Measurement measurement) + { + var number = measurement.Value; + switch (this.aggType) { case AggregationType.LongSumIncomingDelta: - case AggregationType.LongSumIncomingCumulative: { - if (outputDelta) - { - long initValue = Interlocked.Read(ref this.runningValue.AsLong); - this.snapshotValue.AsLong = initValue - this.deltaLastValue.AsLong; - this.deltaLastValue.AsLong = initValue; - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - // Check again if value got updated, if yes reset status. - // This ensures no Updates get Lost. - if (initValue != Interlocked.Read(ref this.runningValue.AsLong)) - { - this.MetricPointStatus = MetricPointStatus.CollectPending; - } - } - else - { - this.snapshotValue.AsLong = Interlocked.Read(ref this.runningValue.AsLong); - } - + Interlocked.Add(ref this.runningValue.AsLong, number); break; } - case AggregationType.DoubleSumIncomingDelta: - case AggregationType.DoubleSumIncomingCumulative: + case AggregationType.LongSumIncomingCumulative: { - if (outputDelta) - { - // TODO: - // Is this thread-safe way to read double? - // As long as the value is not -ve infinity, - // the exchange (to 0.0) will never occur, - // but we get the original value atomically. - double initValue = Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity); - this.snapshotValue.AsDouble = initValue - this.deltaLastValue.AsDouble; - this.deltaLastValue.AsDouble = initValue; - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - // Check again if value got updated, if yes reset status. - // This ensures no Updates get Lost. - if (initValue != Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity)) - { - this.MetricPointStatus = MetricPointStatus.CollectPending; - } - } - else - { - // TODO: - // Is this thread-safe way to read double? - // As long as the value is not -ve infinity, - // the exchange (to 0.0) will never occur, - // but we get the original value atomically. - this.snapshotValue.AsDouble = Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity); - } - + Interlocked.Exchange(ref this.runningValue.AsLong, number); break; } case AggregationType.LongGauge: { - this.snapshotValue.AsLong = Interlocked.Read(ref this.runningValue.AsLong); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - // Check again if value got updated, if yes reset status. - // This ensures no Updates get Lost. - if (this.snapshotValue.AsLong != Interlocked.Read(ref this.runningValue.AsLong)) - { - this.MetricPointStatus = MetricPointStatus.CollectPending; - } - + Interlocked.Exchange(ref this.runningValue.AsLong, number); break; } - case AggregationType.DoubleGauge: + case AggregationType.Histogram: { - // TODO: - // Is this thread-safe way to read double? - // As long as the value is not -ve infinity, - // the exchange (to 0.0) will never occur, - // but we get the original value atomically. - this.snapshotValue.AsDouble = Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; - - // Check again if value got updated, if yes reset status. - // This ensures no Updates get Lost. - if (this.snapshotValue.AsDouble != Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity)) - { - this.MetricPointStatus = MetricPointStatus.CollectPending; - } + this.UpdateHistogram((double)number); + break; + } + case AggregationType.HistogramWithMinMax: + { + this.UpdateHistogramWithMinMax((double)number); break; } case AggregationType.HistogramWithBuckets: { - Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); + measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets((double)number); + break; + } - var histogramBuckets = this.mpComponents!.HistogramBuckets; + case AggregationType.HistogramWithMinMaxBuckets: + { + measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax((double)number); + break; + } - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); + case AggregationType.Base2ExponentialHistogram: + { + this.UpdateBase2ExponentialHistogram((double)number); + break; + } - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; + case AggregationType.Base2ExponentialHistogramWithMinMax: + { + this.UpdateBase2ExponentialHistogramWithMinMax((double)number); + break; + } + } + } - if (outputDelta) + private void Update(ref Measurement measurement) + { + var number = measurement.Value; + + switch (this.aggType) + { + case AggregationType.DoubleSumIncomingDelta: + { + double initValue, newValue; + var sw = default(SpinWait); + while (true) { - this.runningValue.AsLong = 0; - histogramBuckets.RunningSum = 0; - } + initValue = this.runningValue.AsDouble; - Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); + unchecked + { + newValue = initValue + number; + } - for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++) - { - histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i]; - if (outputDelta) + if (initValue == Interlocked.CompareExchange(ref this.runningValue.AsDouble, newValue, initValue)) { - histogramBuckets.RunningBucketCounts[i] = 0; + break; } + + sw.SpinOnce(); } - this.MetricPointStatus = MetricPointStatus.NoCollectPending; + break; + } - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + case AggregationType.DoubleSumIncomingCumulative: + { + Interlocked.Exchange(ref this.runningValue.AsDouble, number); + break; + } + case AggregationType.DoubleGauge: + { + Interlocked.Exchange(ref this.runningValue.AsDouble, number); break; } case AggregationType.Histogram: { - Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - - var histogramBuckets = this.mpComponents!.HistogramBuckets; - - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; + this.UpdateHistogram(number); + break; + } - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogramBuckets.RunningSum = 0; - } + case AggregationType.HistogramWithMinMax: + { + this.UpdateHistogramWithMinMax(number); + break; + } - this.MetricPointStatus = MetricPointStatus.NoCollectPending; + case AggregationType.HistogramWithBuckets: + { + measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets(number); + break; + } - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + case AggregationType.HistogramWithMinMaxBuckets: + { + measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax(number); + break; + } + case AggregationType.Base2ExponentialHistogram: + { + this.UpdateBase2ExponentialHistogram(number); break; } - case AggregationType.HistogramWithMinMaxBuckets: + case AggregationType.Base2ExponentialHistogramWithMinMax: { - Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); + this.UpdateBase2ExponentialHistogramWithMinMax(number); + break; + } + } + } - var histogramBuckets = this.mpComponents!.HistogramBuckets; + private void UpdateHistogram(double number) + { + Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); + var histogramBuckets = this.mpComponents!.HistogramBuckets; - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; - histogramBuckets.SnapshotMin = histogramBuckets.RunningMin; - histogramBuckets.SnapshotMax = histogramBuckets.RunningMax; - - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogramBuckets.RunningSum = 0; - histogramBuckets.RunningMin = double.PositiveInfinity; - histogramBuckets.RunningMax = double.NegativeInfinity; - } + AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); + unchecked + { + this.runningValue.AsLong++; + histogramBuckets.RunningSum += number; + } - for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++) - { - histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i]; - if (outputDelta) - { - histogramBuckets.RunningBucketCounts[i] = 0; - } - } + ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + } - this.MetricPointStatus = MetricPointStatus.NoCollectPending; + private void UpdateHistogramWithMinMax(double number) + { + Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + var histogramBuckets = this.mpComponents!.HistogramBuckets; - break; - } + AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - case AggregationType.HistogramWithMinMax: - { - Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); + unchecked + { + this.runningValue.AsLong++; + histogramBuckets.RunningSum += number; + histogramBuckets.RunningMin = Math.Min(histogramBuckets.RunningMin, number); + histogramBuckets.RunningMax = Math.Max(histogramBuckets.RunningMax, number); + } - var histogramBuckets = this.mpComponents!.HistogramBuckets; + ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + } - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); + private int UpdateHistogramWithBuckets(double number) + { + Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; - histogramBuckets.SnapshotMin = histogramBuckets.RunningMin; - histogramBuckets.SnapshotMax = histogramBuckets.RunningMax; + var histogramBuckets = this.mpComponents!.HistogramBuckets; - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogramBuckets.RunningSum = 0; - histogramBuckets.RunningMin = double.PositiveInfinity; - histogramBuckets.RunningMax = double.NegativeInfinity; - } + int i = histogramBuckets!.FindBucketIndex(number); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; + Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + AcquireLock(ref histogramBuckets.IsCriticalSectionOccupied); - break; - } + unchecked + { + this.runningValue.AsLong++; + histogramBuckets.RunningSum += number; + histogramBuckets.RunningBucketCounts![i]++; + } - case AggregationType.Base2ExponentialHistogram: - { - Debug.Assert(this.mpComponents?.Base2ExponentialBucketHistogram != null, "Base2ExponentialBucketHistogram was null"); + ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; + return i; + } - AcquireLock(ref histogram!.IsCriticalSectionOccupied); + private int UpdateHistogramWithBucketsAndMinMax(double number) + { + Debug.Assert(this.mpComponents?.HistogramBuckets != null, "histogramBuckets was null"); - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogram.SnapshotSum = histogram.RunningSum; - histogram.Snapshot(); + var histogramBuckets = this.mpComponents!.HistogramBuckets; - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogram.RunningSum = 0; - histogram.Reset(); - } + int i = histogramBuckets!.FindBucketIndex(number); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; + AcquireLock(ref histogramBuckets.IsCriticalSectionOccupied); - ReleaseLock(ref histogram.IsCriticalSectionOccupied); + Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); - break; - } + unchecked + { + this.runningValue.AsLong++; + histogramBuckets.RunningSum += number; + histogramBuckets.RunningBucketCounts![i]++; - case AggregationType.Base2ExponentialHistogramWithMinMax: - { - Debug.Assert(this.mpComponents?.Base2ExponentialBucketHistogram != null, "Base2ExponentialBucketHistogram was null"); + histogramBuckets.RunningMin = Math.Min(histogramBuckets.RunningMin, number); + histogramBuckets.RunningMax = Math.Max(histogramBuckets.RunningMax, number); + } - var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; + ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - AcquireLock(ref histogram!.IsCriticalSectionOccupied); + return i; + } - this.snapshotValue.AsLong = this.runningValue.AsLong; - histogram.SnapshotSum = histogram.RunningSum; - histogram.Snapshot(); - histogram.SnapshotMin = histogram.RunningMin; - histogram.SnapshotMax = histogram.RunningMax; + private void UpdateBase2ExponentialHistogram(double number) + { + if (number < 0) + { + return; + } - if (outputDelta) - { - this.runningValue.AsLong = 0; - histogram.RunningSum = 0; - histogram.Reset(); - histogram.RunningMin = double.PositiveInfinity; - histogram.RunningMax = double.NegativeInfinity; - } + Debug.Assert(this.mpComponents?.Base2ExponentialBucketHistogram != null, "Base2ExponentialBucketHistogram was null"); - this.MetricPointStatus = MetricPointStatus.NoCollectPending; + var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; - ReleaseLock(ref histogram.IsCriticalSectionOccupied); + AcquireLock(ref histogram!.IsCriticalSectionOccupied); - break; - } + unchecked + { + this.runningValue.AsLong++; + histogram.RunningSum += number; + histogram.Record(number); } + + ReleaseLock(ref histogram.IsCriticalSectionOccupied); } - internal void TakeSnapshotAndCollectExemplars(bool outputDelta) + private void UpdateBase2ExponentialHistogramWithMinMax(double number) { - Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); + if (number < 0) + { + return; + } - this.TakeSnapshot(outputDelta); + Debug.Assert(this.mpComponents?.Base2ExponentialBucketHistogram != null, "Base2ExponentialBucketHistogram was null"); - this.mpComponents!.Exemplars = this.mpComponents.ExemplarReservoir!.Collect(); - } + var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; - private static void AcquireLock(ref int isCriticalSectionOccupied) - { - var sw = default(SpinWait); - while (Interlocked.Exchange(ref isCriticalSectionOccupied, 1) != 0) + AcquireLock(ref histogram!.IsCriticalSectionOccupied); + + unchecked { - sw.SpinOnce(); + this.runningValue.AsLong++; + histogram.RunningSum += number; + histogram.Record(number); + + histogram.RunningMin = Math.Min(histogram.RunningMin, number); + histogram.RunningMax = Math.Max(histogram.RunningMax, number); } - } - private static void ReleaseLock(ref int isCriticalSectionOccupied) - { - Interlocked.Exchange(ref isCriticalSectionOccupied, 0); + ReleaseLock(ref histogram.IsCriticalSectionOccupied); } - private bool ShouldOfferExemplar(ref Measurement measurement) - where T : struct + private void CompleteUpdate() { - var exemplarFilter = this.aggregatorStore.ExemplarFilter; + // There is a race with Snapshot: + // Update() updates the value + // Snapshot snapshots the value + // Snapshot sets status to NoCollectPending + // Update sets status to CollectPending -- this is not right as the Snapshot + // already included the updated value. + // In the absence of any new Update call until next Snapshot, + // this results in exporting an Update even though + // it had no update. + // TODO: For Delta, this can be mitigated + // by ignoring Zero points + this.MetricPointStatus = MetricPointStatus.CollectPending; - if (exemplarFilter is AlwaysOffExemplarFilter) - { - return false; - } - else if (exemplarFilter is AlwaysOnExemplarFilter) - { - return true; - } - else if (typeof(T) == typeof(long)) - { - return exemplarFilter.ShouldSample((long)(object)measurement.Value, measurement.Tags); - } - else if (typeof(T) == typeof(double)) - { - return exemplarFilter.ShouldSample((double)(object)measurement.Value, measurement.Tags); - } - else + if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled) { - Debug.Fail("Invalid value type"); - return exemplarFilter.ShouldSample(Convert.ToDouble(measurement.Value), measurement.Tags); + Interlocked.Decrement(ref this.ReferenceCount); } } - private void Update(ref Measurement measurement) + private void Snapshot(bool outputDelta) { - var number = measurement.Value; - switch (this.aggType) { case AggregationType.LongSumIncomingDelta: - { - Interlocked.Add(ref this.runningValue.AsLong, number); - break; - } - case AggregationType.LongSumIncomingCumulative: { - Interlocked.Exchange(ref this.runningValue.AsLong, number); - break; - } + if (outputDelta) + { + long initValue = Interlocked.Read(ref this.runningValue.AsLong); + this.snapshotValue.AsLong = initValue - this.deltaLastValue.AsLong; + this.deltaLastValue.AsLong = initValue; + this.MetricPointStatus = MetricPointStatus.NoCollectPending; - case AggregationType.LongGauge: - { - Interlocked.Exchange(ref this.runningValue.AsLong, number); - break; - } + // Check again if value got updated, if yes reset status. + // This ensures no Updates get Lost. + if (initValue != Interlocked.Read(ref this.runningValue.AsLong)) + { + this.MetricPointStatus = MetricPointStatus.CollectPending; + } + } + else + { + this.snapshotValue.AsLong = Interlocked.Read(ref this.runningValue.AsLong); + } - case AggregationType.Histogram: - { - this.UpdateHistogram((double)number); break; } - case AggregationType.HistogramWithMinMax: - { - this.UpdateHistogramWithMinMax((double)number); - break; - } - - case AggregationType.HistogramWithBuckets: - { - measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets((double)number); - break; - } - - case AggregationType.HistogramWithMinMaxBuckets: - { - measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax((double)number); - break; - } - - case AggregationType.Base2ExponentialHistogram: - { - this.UpdateBase2ExponentialHistogram((double)number); - break; - } - - case AggregationType.Base2ExponentialHistogramWithMinMax: - { - this.UpdateBase2ExponentialHistogramWithMinMax((double)number); - break; - } - } - } - - private void Update(ref Measurement measurement) - { - var number = measurement.Value; - - switch (this.aggType) - { case AggregationType.DoubleSumIncomingDelta: + case AggregationType.DoubleSumIncomingCumulative: { - double initValue, newValue; - var sw = default(SpinWait); - while (true) + if (outputDelta) { - initValue = this.runningValue.AsDouble; - - unchecked - { - newValue = initValue + number; - } + // TODO: + // Is this thread-safe way to read double? + // As long as the value is not -ve infinity, + // the exchange (to 0.0) will never occur, + // but we get the original value atomically. + double initValue = Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity); + this.snapshotValue.AsDouble = initValue - this.deltaLastValue.AsDouble; + this.deltaLastValue.AsDouble = initValue; + this.MetricPointStatus = MetricPointStatus.NoCollectPending; - if (initValue == Interlocked.CompareExchange(ref this.runningValue.AsDouble, newValue, initValue)) + // Check again if value got updated, if yes reset status. + // This ensures no Updates get Lost. + if (initValue != Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity)) { - break; + this.MetricPointStatus = MetricPointStatus.CollectPending; } - - sw.SpinOnce(); + } + else + { + // TODO: + // Is this thread-safe way to read double? + // As long as the value is not -ve infinity, + // the exchange (to 0.0) will never occur, + // but we get the original value atomically. + this.snapshotValue.AsDouble = Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity); } break; } - case AggregationType.DoubleSumIncomingCumulative: + case AggregationType.LongGauge: { - Interlocked.Exchange(ref this.runningValue.AsDouble, number); + this.snapshotValue.AsLong = Interlocked.Read(ref this.runningValue.AsLong); + this.MetricPointStatus = MetricPointStatus.NoCollectPending; + + // Check again if value got updated, if yes reset status. + // This ensures no Updates get Lost. + if (this.snapshotValue.AsLong != Interlocked.Read(ref this.runningValue.AsLong)) + { + this.MetricPointStatus = MetricPointStatus.CollectPending; + } + break; } case AggregationType.DoubleGauge: { - Interlocked.Exchange(ref this.runningValue.AsDouble, number); - break; - } + // TODO: + // Is this thread-safe way to read double? + // As long as the value is not -ve infinity, + // the exchange (to 0.0) will never occur, + // but we get the original value atomically. + this.snapshotValue.AsDouble = Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity); + this.MetricPointStatus = MetricPointStatus.NoCollectPending; - case AggregationType.Histogram: - { - this.UpdateHistogram(number); - break; - } + // Check again if value got updated, if yes reset status. + // This ensures no Updates get Lost. + if (this.snapshotValue.AsDouble != Interlocked.CompareExchange(ref this.runningValue.AsDouble, 0.0, double.NegativeInfinity)) + { + this.MetricPointStatus = MetricPointStatus.CollectPending; + } - case AggregationType.HistogramWithMinMax: - { - this.UpdateHistogramWithMinMax(number); break; } case AggregationType.HistogramWithBuckets: { - measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets(number); - break; - } + Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - case AggregationType.HistogramWithMinMaxBuckets: - { - measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax(number); - break; - } + var histogramBuckets = this.mpComponents!.HistogramBuckets; - case AggregationType.Base2ExponentialHistogram: - { - this.UpdateBase2ExponentialHistogram(number); - break; - } + AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); + + this.snapshotValue.AsLong = this.runningValue.AsLong; + histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; + + if (outputDelta) + { + this.runningValue.AsLong = 0; + histogramBuckets.RunningSum = 0; + } + + Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); + + for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++) + { + histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i]; + if (outputDelta) + { + histogramBuckets.RunningBucketCounts[i] = 0; + } + } + + this.MetricPointStatus = MetricPointStatus.NoCollectPending; + + ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - case AggregationType.Base2ExponentialHistogramWithMinMax: - { - this.UpdateBase2ExponentialHistogramWithMinMax(number); break; } - } - } - - private void UpdateHistogram(double number) - { - Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - var histogramBuckets = this.mpComponents!.HistogramBuckets; + case AggregationType.Histogram: + { + Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); + var histogramBuckets = this.mpComponents!.HistogramBuckets; - unchecked - { - this.runningValue.AsLong++; - histogramBuckets.RunningSum += number; - } + AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); + this.snapshotValue.AsLong = this.runningValue.AsLong; + histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - } + if (outputDelta) + { + this.runningValue.AsLong = 0; + histogramBuckets.RunningSum = 0; + } - private void UpdateHistogramWithMinMax(double number) - { - Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); + this.MetricPointStatus = MetricPointStatus.NoCollectPending; - var histogramBuckets = this.mpComponents!.HistogramBuckets; + ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); + break; + } - unchecked - { - this.runningValue.AsLong++; - histogramBuckets.RunningSum += number; - histogramBuckets.RunningMin = Math.Min(histogramBuckets.RunningMin, number); - histogramBuckets.RunningMax = Math.Max(histogramBuckets.RunningMax, number); - } + case AggregationType.HistogramWithMinMaxBuckets: + { + Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - } + var histogramBuckets = this.mpComponents!.HistogramBuckets; - private int UpdateHistogramWithBuckets(double number) - { - Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); + AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - var histogramBuckets = this.mpComponents!.HistogramBuckets; + this.snapshotValue.AsLong = this.runningValue.AsLong; + histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; + histogramBuckets.SnapshotMin = histogramBuckets.RunningMin; + histogramBuckets.SnapshotMax = histogramBuckets.RunningMax; - int i = histogramBuckets!.FindBucketIndex(number); + if (outputDelta) + { + this.runningValue.AsLong = 0; + histogramBuckets.RunningSum = 0; + histogramBuckets.RunningMin = double.PositiveInfinity; + histogramBuckets.RunningMax = double.NegativeInfinity; + } - Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); + Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); - AcquireLock(ref histogramBuckets.IsCriticalSectionOccupied); + for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++) + { + histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i]; + if (outputDelta) + { + histogramBuckets.RunningBucketCounts[i] = 0; + } + } - unchecked - { - this.runningValue.AsLong++; - histogramBuckets.RunningSum += number; - histogramBuckets.RunningBucketCounts![i]++; - } + this.MetricPointStatus = MetricPointStatus.NoCollectPending; - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - return i; - } + break; + } - private int UpdateHistogramWithBucketsAndMinMax(double number) - { - Debug.Assert(this.mpComponents?.HistogramBuckets != null, "histogramBuckets was null"); + case AggregationType.HistogramWithMinMax: + { + Debug.Assert(this.mpComponents?.HistogramBuckets != null, "HistogramBuckets was null"); - var histogramBuckets = this.mpComponents!.HistogramBuckets; + var histogramBuckets = this.mpComponents!.HistogramBuckets; - int i = histogramBuckets!.FindBucketIndex(number); + AcquireLock(ref histogramBuckets!.IsCriticalSectionOccupied); - AcquireLock(ref histogramBuckets.IsCriticalSectionOccupied); + this.snapshotValue.AsLong = this.runningValue.AsLong; + histogramBuckets.SnapshotSum = histogramBuckets.RunningSum; + histogramBuckets.SnapshotMin = histogramBuckets.RunningMin; + histogramBuckets.SnapshotMax = histogramBuckets.RunningMax; - Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null"); + if (outputDelta) + { + this.runningValue.AsLong = 0; + histogramBuckets.RunningSum = 0; + histogramBuckets.RunningMin = double.PositiveInfinity; + histogramBuckets.RunningMax = double.NegativeInfinity; + } - unchecked - { - this.runningValue.AsLong++; - histogramBuckets.RunningSum += number; - histogramBuckets.RunningBucketCounts![i]++; + this.MetricPointStatus = MetricPointStatus.NoCollectPending; - histogramBuckets.RunningMin = Math.Min(histogramBuckets.RunningMin, number); - histogramBuckets.RunningMax = Math.Max(histogramBuckets.RunningMax, number); - } + ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); - ReleaseLock(ref histogramBuckets.IsCriticalSectionOccupied); + break; + } - return i; - } + case AggregationType.Base2ExponentialHistogram: + { + Debug.Assert(this.mpComponents?.Base2ExponentialBucketHistogram != null, "Base2ExponentialBucketHistogram was null"); - private void UpdateBase2ExponentialHistogram(double number) - { - if (number < 0) - { - return; - } + var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; - Debug.Assert(this.mpComponents?.Base2ExponentialBucketHistogram != null, "Base2ExponentialBucketHistogram was null"); + AcquireLock(ref histogram!.IsCriticalSectionOccupied); - var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; + this.snapshotValue.AsLong = this.runningValue.AsLong; + histogram.SnapshotSum = histogram.RunningSum; + histogram.Snapshot(); - AcquireLock(ref histogram!.IsCriticalSectionOccupied); + if (outputDelta) + { + this.runningValue.AsLong = 0; + histogram.RunningSum = 0; + histogram.Reset(); + } - unchecked - { - this.runningValue.AsLong++; - histogram.RunningSum += number; - histogram.Record(number); - } + this.MetricPointStatus = MetricPointStatus.NoCollectPending; - ReleaseLock(ref histogram.IsCriticalSectionOccupied); - } + ReleaseLock(ref histogram.IsCriticalSectionOccupied); - private void UpdateBase2ExponentialHistogramWithMinMax(double number) - { - if (number < 0) - { - return; - } + break; + } - Debug.Assert(this.mpComponents?.Base2ExponentialBucketHistogram != null, "Base2ExponentialBucketHistogram was null"); + case AggregationType.Base2ExponentialHistogramWithMinMax: + { + Debug.Assert(this.mpComponents?.Base2ExponentialBucketHistogram != null, "Base2ExponentialBucketHistogram was null"); - var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; + var histogram = this.mpComponents!.Base2ExponentialBucketHistogram; - AcquireLock(ref histogram!.IsCriticalSectionOccupied); + AcquireLock(ref histogram!.IsCriticalSectionOccupied); - unchecked - { - this.runningValue.AsLong++; - histogram.RunningSum += number; - histogram.Record(number); + this.snapshotValue.AsLong = this.runningValue.AsLong; + histogram.SnapshotSum = histogram.RunningSum; + histogram.Snapshot(); + histogram.SnapshotMin = histogram.RunningMin; + histogram.SnapshotMax = histogram.RunningMax; - histogram.RunningMin = Math.Min(histogram.RunningMin, number); - histogram.RunningMax = Math.Max(histogram.RunningMax, number); - } + if (outputDelta) + { + this.runningValue.AsLong = 0; + histogram.RunningSum = 0; + histogram.Reset(); + histogram.RunningMin = double.PositiveInfinity; + histogram.RunningMax = double.NegativeInfinity; + } - ReleaseLock(ref histogram.IsCriticalSectionOccupied); - } + this.MetricPointStatus = MetricPointStatus.NoCollectPending; - private void CompleteUpdate() - { - // There is a race with Snapshot: - // Update() updates the value - // Snapshot snapshots the value - // Snapshot sets status to NoCollectPending - // Update sets status to CollectPending -- this is not right as the Snapshot - // already included the updated value. - // In the absence of any new Update call until next Snapshot, - // this results in exporting an Update even though - // it had no update. - // TODO: For Delta, this can be mitigated - // by ignoring Zero points - this.MetricPointStatus = MetricPointStatus.CollectPending; + ReleaseLock(ref histogram.IsCriticalSectionOccupied); - if (this.aggregatorStore.OutputDeltaWithUnusedMetricPointReclaimEnabled) - { - Interlocked.Decrement(ref this.ReferenceCount); + break; + } } } From de1856ab9bafcbe9e1c22cc655387182c955e38f Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 14 Feb 2024 14:51:07 -0800 Subject: [PATCH 11/29] Tweaks. --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 42 ++++++++- src/OpenTelemetry/Metrics/MetricPoint.cs | 93 ++++++-------------- 2 files changed, 64 insertions(+), 71 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index b2f6c2219fa..70071b5afdb 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -17,7 +17,7 @@ internal sealed class AggregatorStore internal readonly int CardinalityLimit; internal readonly bool EmitOverflowAttribute; internal readonly ConcurrentDictionary? TagsToMetricPointIndexDictionaryDelta; - internal readonly ExemplarFilter ExemplarFilter; + internal readonly ExemplarSamplingHelper ExemplarSampler; internal long DroppedMeasurements = 0; private static readonly string MetricPointCapHitFixMessage = "Consider opting in for the experimental SDK feature to emit all the throttled metrics under the overflow attribute by setting env variable OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE = true. You could also modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit."; @@ -44,6 +44,7 @@ internal sealed class AggregatorStore private readonly int exponentialHistogramMaxScale; private readonly UpdateLongDelegate updateLongCallback; private readonly UpdateDoubleDelegate updateDoubleCallback; + private readonly ExemplarFilter exemplarFilter; private readonly Func[], int, int> lookupAggregatorStore; private int metricPointIndex = 0; @@ -73,7 +74,7 @@ internal AggregatorStore( this.exponentialHistogramMaxSize = metricStreamIdentity.ExponentialHistogramMaxSize; this.exponentialHistogramMaxScale = metricStreamIdentity.ExponentialHistogramMaxScale; this.StartTimeExclusive = DateTimeOffset.UtcNow; - this.ExemplarFilter = exemplarFilter ?? DefaultExemplarFilter; + this.exemplarFilter = exemplarFilter ?? DefaultExemplarFilter; if (metricStreamIdentity.TagKeys == null) { this.updateLongCallback = this.UpdateLong; @@ -125,6 +126,8 @@ internal AggregatorStore( { this.lookupAggregatorStore = this.LookupAggregatorStore; } + + this.ExemplarSampler = new(this.exemplarFilter); } private delegate void UpdateLongDelegate(long value, ReadOnlySpan> tags); @@ -141,7 +144,7 @@ internal bool IsExemplarEnabled() { // Using this filter to indicate On/Off // instead of another separate flag. - return this.ExemplarFilter is not AlwaysOffExemplarFilter; + return this.exemplarFilter is not AlwaysOffExemplarFilter; } internal void Update(long value, ReadOnlySpan> tags) @@ -1068,4 +1071,37 @@ private int FindMetricAggregatorsCustomTag(ReadOnlySpan ShouldSampleLong; + public ShouldSampleFunc ShouldSampleDouble; + + public ExemplarSamplingHelper(ExemplarFilter exemplarFilter) + { + Debug.Assert(exemplarFilter != null, "exemplarFilter was null"); + + if (exemplarFilter is AlwaysOffExemplarFilter) + { + this.EarlySampleDecision = false; + this.ShouldSampleLong = static (_, _) => false; + this.ShouldSampleDouble = static (_, _) => false; + } + else if (exemplarFilter is AlwaysOnExemplarFilter) + { + this.EarlySampleDecision = true; + this.ShouldSampleLong = static (_, _) => true; + this.ShouldSampleDouble = static (_, _) => true; + } + else + { + this.EarlySampleDecision = null; + this.ShouldSampleLong = exemplarFilter!.ShouldSample; + this.ShouldSampleDouble = exemplarFilter.ShouldSample; + } + } + + internal delegate bool ShouldSampleFunc(T value, ReadOnlySpan> tags); + } } diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index f84bd1501a9..f29d3fafaad 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -371,20 +371,15 @@ internal readonly MetricPoint Copy() internal void Update(long number, ReadOnlySpan> tags) { - var measurement = new Measurement - { - Value = number, - Tags = tags, - }; - - this.Update(ref measurement); + this.Update(number, out var explicitBucketHistogramBucketIndex); - if (this.ShouldOfferExemplar(ref measurement)) + var exemplarSampler = this.aggregatorStore.ExemplarSampler; + if (exemplarSampler.EarlySampleDecision ?? exemplarSampler.ShouldSampleLong(number, tags)) { Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); this.mpComponents!.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags, measurement.ExplicitBucketHistogramBucketIndex)); + new ExemplarMeasurement(number, tags, explicitBucketHistogramBucketIndex)); } this.CompleteUpdate(); @@ -392,20 +387,15 @@ internal void Update(long number, ReadOnlySpan> ta internal void Update(double number, ReadOnlySpan> tags) { - var measurement = new Measurement - { - Value = number, - Tags = tags, - }; + this.Update(number, out var explicitBucketHistogramBucketIndex); - this.Update(ref measurement); - - if (this.ShouldOfferExemplar(ref measurement)) + var exemplarSampler = this.aggregatorStore.ExemplarSampler; + if (exemplarSampler.EarlySampleDecision ?? exemplarSampler.ShouldSampleDouble(number, tags)) { Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); this.mpComponents!.ExemplarReservoir!.Offer( - new ExemplarMeasurement(number, tags, measurement.ExplicitBucketHistogramBucketIndex)); + new ExemplarMeasurement(number, tags, explicitBucketHistogramBucketIndex)); } this.CompleteUpdate(); @@ -436,38 +426,9 @@ private static void ReleaseLock(ref int isCriticalSectionOccupied) Interlocked.Exchange(ref isCriticalSectionOccupied, 0); } - private readonly bool ShouldOfferExemplar(ref Measurement measurement) - where T : struct - { - var exemplarFilter = this.aggregatorStore.ExemplarFilter; - - if (exemplarFilter is AlwaysOffExemplarFilter) - { - return false; - } - else if (exemplarFilter is AlwaysOnExemplarFilter) - { - return true; - } - else if (typeof(T) == typeof(long)) - { - return exemplarFilter.ShouldSample((long)(object)measurement.Value, measurement.Tags); - } - else if (typeof(T) == typeof(double)) - { - return exemplarFilter.ShouldSample((double)(object)measurement.Value, measurement.Tags); - } - else - { - Debug.Fail("Invalid value type"); - return exemplarFilter.ShouldSample(Convert.ToDouble(measurement.Value), measurement.Tags); - } - } - - private void Update(ref Measurement measurement) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void Update(long number, out int explicitBucketHistogramBucketIndex) { - var number = measurement.Value; - switch (this.aggType) { case AggregationType.LongSumIncomingDelta: @@ -502,14 +463,14 @@ private void Update(ref Measurement measurement) case AggregationType.HistogramWithBuckets: { - measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets((double)number); - break; + explicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets((double)number); + return; } case AggregationType.HistogramWithMinMaxBuckets: { - measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax((double)number); - break; + explicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax((double)number); + return; } case AggregationType.Base2ExponentialHistogram: @@ -524,12 +485,13 @@ private void Update(ref Measurement measurement) break; } } + + explicitBucketHistogramBucketIndex = -1; } - private void Update(ref Measurement measurement) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void Update(double number, out int explicitBucketHistogramBucketIndex) { - var number = measurement.Value; - switch (this.aggType) { case AggregationType.DoubleSumIncomingDelta: @@ -582,14 +544,14 @@ private void Update(ref Measurement measurement) case AggregationType.HistogramWithBuckets: { - measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets(number); - break; + explicitBucketHistogramBucketIndex = this.UpdateHistogramWithBuckets(number); + return; } case AggregationType.HistogramWithMinMaxBuckets: { - measurement.ExplicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax(number); - break; + explicitBucketHistogramBucketIndex = this.UpdateHistogramWithBucketsAndMinMax(number); + return; } case AggregationType.Base2ExponentialHistogram: @@ -604,6 +566,8 @@ private void Update(ref Measurement measurement) break; } } + + explicitBucketHistogramBucketIndex = -1; } private void UpdateHistogram(double number) @@ -742,6 +706,7 @@ private void UpdateBase2ExponentialHistogramWithMinMax(double number) ReleaseLock(ref histogram.IsCriticalSectionOccupied); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void CompleteUpdate() { // There is a race with Snapshot: @@ -1050,12 +1015,4 @@ private readonly void ThrowNotSupportedMetricTypeException(string methodName) { throw new NotSupportedException($"{methodName} is not supported for this metric type."); } - - private ref struct Measurement - where T : struct - { - public T Value; - public ReadOnlySpan> Tags; - public int ExplicitBucketHistogramBucketIndex; - } } From ea58f75033873a41542e6e5d30e833918c56d7c6 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 14 Feb 2024 17:37:40 -0800 Subject: [PATCH 12/29] Tweak. --- .../Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs index 177ed02e379..54037cb6770 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs @@ -10,7 +10,7 @@ namespace OpenTelemetry.Metrics; /// internal sealed class SimpleFixedSizeExemplarReservoir : FixedSizeExemplarReservoir { - private int measurementsSeen; + private volatile int measurementsSeen; public SimpleFixedSizeExemplarReservoir() : base(Environment.ProcessorCount) @@ -33,7 +33,7 @@ protected override void OnCollectionCompleted() // Reset internal state irrespective of temporality. // This ensures incoming measurements have fair chance // of making it to the reservoir. - Interlocked.Exchange(ref this.measurementsSeen, 0); + this.measurementsSeen = 0; } private void Offer(in ExemplarMeasurement measurement) From 369738bd65772bc08fb43544b3e5582d620e1922 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 14 Feb 2024 17:38:18 -0800 Subject: [PATCH 13/29] Tweak. --- .../Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs index 54037cb6770..aa7641e4f08 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs @@ -15,7 +15,6 @@ internal sealed class SimpleFixedSizeExemplarReservoir : FixedSizeExemplarReserv public SimpleFixedSizeExemplarReservoir() : base(Environment.ProcessorCount) { - this.OnCollectionCompleted(); } public override void Offer(in ExemplarMeasurement measurement) From 17cbe69e9fba5f93e850b0f526952f33c39cd044 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 15 Feb 2024 10:18:13 -0800 Subject: [PATCH 14/29] Cleanup. --- .../AlignedHistogramBucketExemplarReservoir.cs | 4 ++++ .../Exemplar/FixedSizeExemplarReservoir.cs | 13 ++++++++----- .../SimpleFixedSizeExemplarReservoir.cs | 17 +++++++++++++---- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/AlignedHistogramBucketExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/AlignedHistogramBucketExemplarReservoir.cs index 62b1e78efa0..41e4e32bdcf 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/AlignedHistogramBucketExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/AlignedHistogramBucketExemplarReservoir.cs @@ -8,6 +8,10 @@ namespace OpenTelemetry.Metrics; /// /// The AlignedHistogramBucketExemplarReservoir implementation. /// +/// +/// Specification: . +/// internal sealed class AlignedHistogramBucketExemplarReservoir : FixedSizeExemplarReservoir { public AlignedHistogramBucketExemplarReservoir(int numberOfBuckets) diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index abbc8fba386..43630f890e0 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 using System.Diagnostics; +using OpenTelemetry.Internal; namespace OpenTelemetry.Metrics; @@ -11,15 +12,17 @@ internal abstract class FixedSizeExemplarReservoir : ExemplarReservoir private readonly Exemplar[] bufferB; private volatile Exemplar[]? activeBuffer; - protected FixedSizeExemplarReservoir(int exemplarCount) + protected FixedSizeExemplarReservoir(int capacity) { - this.bufferA = new Exemplar[exemplarCount]; - this.bufferB = new Exemplar[exemplarCount]; + Guard.ThrowIfOutOfRange(capacity, min: 1); + + this.bufferA = new Exemplar[capacity]; + this.bufferB = new Exemplar[capacity]; this.activeBuffer = this.bufferA; - this.ExemplarCount = exemplarCount; + this.Capacity = capacity; } - internal int ExemplarCount { get; } + internal int Capacity { get; } /// /// Collects all the exemplars accumulated by the Reservoir. diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs index aa7641e4f08..edc4b9d0f33 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs @@ -6,14 +6,23 @@ namespace OpenTelemetry.Metrics; /// -/// The SimpleExemplarReservoir implementation. +/// The SimpleFixedSizeExemplarReservoir implementation. /// +/// +/// Specification: . +/// internal sealed class SimpleFixedSizeExemplarReservoir : FixedSizeExemplarReservoir { private volatile int measurementsSeen; public SimpleFixedSizeExemplarReservoir() - : base(Environment.ProcessorCount) + : this(Environment.ProcessorCount) + { + } + + public SimpleFixedSizeExemplarReservoir(int capacity) + : base(capacity) { } @@ -40,14 +49,14 @@ private void Offer(in ExemplarMeasurement measurement) { var currentMeasurement = Interlocked.Increment(ref this.measurementsSeen) - 1; - if (currentMeasurement < this.ExemplarCount) + if (currentMeasurement < this.Capacity) { this.UpdateExemplar(currentMeasurement, in measurement); } else { int index = ThreadSafeRandom.Next(0, currentMeasurement); - if (index < this.ExemplarCount) + if (index < this.Capacity) { this.UpdateExemplar(index, in measurement); } From 39230c97bbef2c8703547d445771a4861e600e11 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 15 Feb 2024 12:20:19 -0800 Subject: [PATCH 15/29] Cleanup. --- .../Implementation/MetricItemExtensions.cs | 84 +++++++------------ 1 file changed, 30 insertions(+), 54 deletions(-) diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs index 63f78bdfb81..cd9c8a39062 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 using System.Collections.Concurrent; +using System.Diagnostics; using System.Runtime.CompilerServices; using Google.Protobuf; using Google.Protobuf.Collections; @@ -271,29 +272,8 @@ internal static OtlpMetrics.Metric ToOtlpMetric(this Metric metric) { foreach (ref readonly var exemplar in exemplars) { - byte[] traceIdBytes = new byte[16]; - exemplar.TraceId?.CopyTo(traceIdBytes); - - byte[] spanIdBytes = new byte[8]; - exemplar.SpanId?.CopyTo(spanIdBytes); - - var otlpExemplar = new OtlpMetrics.Exemplar - { - TimeUnixNano = (ulong)exemplar.Timestamp.ToUnixTimeNanoseconds(), - TraceId = UnsafeByteOperations.UnsafeWrap(traceIdBytes), - SpanId = UnsafeByteOperations.UnsafeWrap(spanIdBytes), - AsDouble = exemplar.DoubleValue, - }; - - foreach (var tag in exemplar.FilteredTags) - { - if (OtlpKeyValueTransformer.Instance.TryTransformTag(tag, out var result)) - { - otlpExemplar.FilteredAttributes.Add(result); - } - } - - dataPoint.Exemplars.Add(otlpExemplar); + dataPoint.Exemplars.Add( + ToOtlpExemplar(exemplar.DoubleValue, in exemplar)); } } @@ -375,51 +355,47 @@ private static void AddScopeAttributes(IEnumerable> } } - /* - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static OtlpMetrics.Exemplar ToOtlpExemplar(this IExemplar exemplar) + private static OtlpMetrics.Exemplar ToOtlpExemplar(T value, in Metrics.Exemplar exemplar) { - var otlpExemplar = new OtlpMetrics.Exemplar(); - - if (exemplar.Value is double doubleValue) + var otlpExemplar = new OtlpMetrics.Exemplar { - otlpExemplar.AsDouble = doubleValue; - } - else if (exemplar.Value is long longValue) + TimeUnixNano = (ulong)exemplar.Timestamp.ToUnixTimeNanoseconds(), + }; + + if (exemplar.TraceId.HasValue) { - otlpExemplar.AsInt = longValue; + byte[] traceIdBytes = new byte[16]; + exemplar.TraceId?.CopyTo(traceIdBytes); + + byte[] spanIdBytes = new byte[8]; + exemplar.SpanId?.CopyTo(spanIdBytes); + + otlpExemplar.TraceId = UnsafeByteOperations.UnsafeWrap(traceIdBytes); + otlpExemplar.SpanId = UnsafeByteOperations.UnsafeWrap(spanIdBytes); } - else + + if (typeof(T) == typeof(long)) { - // TODO: Determine how we want to handle exceptions here. - // Do we want to just skip this exemplar and move on? - // Should we skip recording the whole metric? - throw new ArgumentException(); + otlpExemplar.AsInt = (long)(object)value; } - - otlpExemplar.TimeUnixNano = (ulong)exemplar.Timestamp.ToUnixTimeNanoseconds(); - - // TODO: Do the TagEnumerationState thing. - foreach (var tag in exemplar.FilteredTags) + else if (typeof(T) == typeof(double)) { - otlpExemplar.FilteredAttributes.Add(tag.ToOtlpAttribute()); + otlpExemplar.AsDouble = (double)(object)value; } - - if (exemplar.TraceId != default) + else { - byte[] traceIdBytes = new byte[16]; - exemplar.TraceId.CopyTo(traceIdBytes); - otlpExemplar.TraceId = UnsafeByteOperations.UnsafeWrap(traceIdBytes); + Debug.Fail("Unexpected type"); + otlpExemplar.AsDouble = Convert.ToDouble(value); } - if (exemplar.SpanId != default) + foreach (var tag in exemplar.FilteredTags) { - byte[] spanIdBytes = new byte[8]; - exemplar.SpanId.CopyTo(spanIdBytes); - otlpExemplar.SpanId = UnsafeByteOperations.UnsafeWrap(spanIdBytes); + if (OtlpKeyValueTransformer.Instance.TryTransformTag(tag, out var result)) + { + otlpExemplar.FilteredAttributes.Add(result); + } } return otlpExemplar; } - */ } From 7a8669c3dc6b41fedb8086b7b68e31d590e0f638 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 15 Feb 2024 12:47:31 -0800 Subject: [PATCH 16/29] Tweaks. --- .../ConsoleMetricExporter.cs | 19 +++++++++++-------- .../Implementation/MetricItemExtensions.cs | 5 +++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs index 7de49ea0fd7..4d7b777d12e 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs @@ -192,23 +192,26 @@ public override ExportResult Export(in Batch batch) { foreach (ref readonly var exemplar in exemplars) { + exemplarString.Append("Timestamp: "); + exemplarString.Append(exemplar.Timestamp.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture)); if (metricType.IsDouble()) { - exemplarString.Append("Value: "); + exemplarString.Append(" Value: "); exemplarString.Append(exemplar.DoubleValue); } else if (metricType.IsLong()) { - exemplarString.Append("Value: "); + exemplarString.Append(" Value: "); exemplarString.Append(exemplar.LongValue); } - exemplarString.Append(" Timestamp: "); - exemplarString.Append(exemplar.Timestamp.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture)); - exemplarString.Append(" TraceId: "); - exemplarString.Append(exemplar.TraceId); - exemplarString.Append(" SpanId: "); - exemplarString.Append(exemplar.SpanId); + if (exemplar.TraceId.HasValue) + { + exemplarString.Append(" TraceId: "); + exemplarString.Append(exemplar.TraceId.Value.ToHexString()); + exemplarString.Append(" SpanId: "); + exemplarString.Append(exemplar.SpanId.Value.ToHexString()); + } bool appendedTagString = false; foreach (var tag in exemplar.FilteredTags) diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs index cd9c8a39062..b3db8c18027 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs @@ -356,6 +356,7 @@ private static void AddScopeAttributes(IEnumerable> } private static OtlpMetrics.Exemplar ToOtlpExemplar(T value, in Metrics.Exemplar exemplar) + where T : struct { var otlpExemplar = new OtlpMetrics.Exemplar { @@ -365,10 +366,10 @@ private static OtlpMetrics.Exemplar ToOtlpExemplar(T value, in Metrics.Exempl if (exemplar.TraceId.HasValue) { byte[] traceIdBytes = new byte[16]; - exemplar.TraceId?.CopyTo(traceIdBytes); + exemplar.TraceId.Value.CopyTo(traceIdBytes); byte[] spanIdBytes = new byte[8]; - exemplar.SpanId?.CopyTo(spanIdBytes); + exemplar.SpanId.Value.CopyTo(spanIdBytes); otlpExemplar.TraceId = UnsafeByteOperations.UnsafeWrap(traceIdBytes); otlpExemplar.SpanId = UnsafeByteOperations.UnsafeWrap(spanIdBytes); From 65e87ebd4a6c8e154cb4540217b35f9c4f53838c Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 15 Feb 2024 15:05:16 -0800 Subject: [PATCH 17/29] CHANGELOG patch. --- src/OpenTelemetry/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 6260596c78c..a3da443e0ca 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -42,6 +42,11 @@ [IMetricsListener](https://learn.microsoft.com/dotNet/api/microsoft.extensions.diagnostics.metrics.imetricslistener). ([#5265](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5265)) +* **Experimental (pre-release builds only):** `Exemplar` and `ExemplarReservoir` + APIs have been updated to match the OpenTelemetry Specification and to achieve + better throughput under load. + ([#5364](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5364)) + ## 1.7.0 Released 2023-Dec-08 From aa0b3b53a1c647702f9b66119015779237a8a396 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 16 Feb 2024 10:26:21 -0800 Subject: [PATCH 18/29] Faster init for FixedSizeExemplarReservoir buffers. --- .../Exemplar/FixedSizeExemplarReservoir.cs | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index 43630f890e0..82b4731156a 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -2,6 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 using System.Diagnostics; +#if NET6_0_OR_GREATER +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +#endif using OpenTelemetry.Internal; namespace OpenTelemetry.Metrics; @@ -59,13 +63,27 @@ internal sealed override void Initialize(AggregatorStore aggregatorStore) { var keyFilter = aggregatorStore.TagKeysInteresting; - for (int a = 0, b = 0; - a < this.bufferA.Length && b < this.bufferB.Length; - a++, b++) +#if NET6_0_OR_GREATER + var length = this.bufferA.Length; + ref var a = ref MemoryMarshal.GetArrayDataReference(this.bufferA); + ref var b = ref MemoryMarshal.GetArrayDataReference(this.bufferB); + do { - this.bufferA[a].KeyFilter = keyFilter; - this.bufferB[b].KeyFilter = keyFilter; + a.KeyFilter = keyFilter; + b.KeyFilter = keyFilter; + a = ref Unsafe.Add(ref a, 1); + b = ref Unsafe.Add(ref b, 1); } + while (--length > 0); +#else + for (int i = 0; + i < this.bufferA.Length && i < this.bufferB.Length; + i++) + { + this.bufferA[i].KeyFilter = keyFilter; + this.bufferB[i].KeyFilter = keyFilter; + } +#endif base.Initialize(aggregatorStore); } From ea2f149d84f6dfaa5d669eaba85f7089b8dfd3e6 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 16 Feb 2024 13:58:09 -0800 Subject: [PATCH 19/29] Tweaks. --- src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs | 16 +++++++++------- .../Exemplar/FixedSizeExemplarReservoir.cs | 10 +++++----- .../Exemplar/SimpleFixedSizeExemplarReservoir.cs | 4 ++-- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs index 0982c0407fa..b2e9a8a7c25 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs @@ -1,13 +1,15 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +using System.Diagnostics; #if EXPOSE_EXPERIMENTAL_FEATURES && NET8_0_OR_GREATER using System.Diagnostics.CodeAnalysis; +#endif +using System.Runtime.CompilerServices; +#if EXPOSE_EXPERIMENTAL_FEATURES && NET8_0_OR_GREATER using OpenTelemetry.Internal; #endif -using System.Diagnostics; - namespace OpenTelemetry.Metrics; #if EXPOSE_EXPERIMENTAL_FEATURES @@ -32,7 +34,7 @@ struct Exemplar private int tagCount; private KeyValuePair[]? tagStorage; private MetricPointValueStorage valueStorage; - private volatile int isCriticalSectionOccupied; + private int isCriticalSectionOccupied; /// /// Gets the timestamp. @@ -76,7 +78,7 @@ public readonly ReadOnlyFilteredTagCollection FilteredTags internal void Update(in ExemplarMeasurement measurement) where T : struct { - if (Interlocked.CompareExchange(ref this.isCriticalSectionOccupied, 1, 0) != 0) + if (Interlocked.Exchange(ref this.isCriticalSectionOccupied, 1) != 0) { // Some other thread is already writing, abort. return; @@ -112,7 +114,7 @@ internal void Update(in ExemplarMeasurement measurement) this.StoreRawTags(measurement.Tags); - this.isCriticalSectionOccupied = 0; + Volatile.Write(ref this.isCriticalSectionOccupied, 0); } internal void Reset() @@ -122,7 +124,7 @@ internal void Reset() internal readonly bool IsUpdated() { - if (this.isCriticalSectionOccupied != 0) + if (Volatile.Read(ref Unsafe.AsRef(in this.isCriticalSectionOccupied)) != 0) { this.WaitForUpdateToCompleteRare(); return true; @@ -171,6 +173,6 @@ private readonly void WaitForUpdateToCompleteRare() { spinWait.SpinOnce(); } - while (this.isCriticalSectionOccupied != 0); + while (Volatile.Read(ref Unsafe.AsRef(in this.isCriticalSectionOccupied)) != 0); } } diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index 82b4731156a..e7f3efd6467 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -14,7 +14,7 @@ internal abstract class FixedSizeExemplarReservoir : ExemplarReservoir { private readonly Exemplar[] bufferA; private readonly Exemplar[] bufferB; - private volatile Exemplar[]? activeBuffer; + private Exemplar[]? activeBuffer; protected FixedSizeExemplarReservoir(int capacity) { @@ -34,11 +34,11 @@ protected FixedSizeExemplarReservoir(int capacity) /// . public sealed override ReadOnlyExemplarCollection Collect() { - var currentBuffer = this.activeBuffer; + var currentBuffer = Volatile.Read(ref this.activeBuffer); Debug.Assert(currentBuffer != null, "currentBuffer was null"); - this.activeBuffer = null; + Volatile.Write(ref this.activeBuffer, null); var inactiveBuffer = currentBuffer == this.bufferA ? this.bufferB @@ -54,7 +54,7 @@ public sealed override ReadOnlyExemplarCollection Collect() this.OnCollectionCompleted(); - this.activeBuffer = inactiveBuffer; + Volatile.Write(ref this.activeBuffer, inactiveBuffer); return new(currentBuffer!); } @@ -95,7 +95,7 @@ protected virtual void OnCollectionCompleted() protected void UpdateExemplar(int exemplarIndex, in ExemplarMeasurement measurement) where T : struct { - var activeBuffer = this.activeBuffer; + var activeBuffer = Volatile.Read(ref this.activeBuffer); if (activeBuffer == null) { // Note: This is expected to happen when we race with a collection. diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs index edc4b9d0f33..bfab577ddae 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs @@ -14,7 +14,7 @@ namespace OpenTelemetry.Metrics; /// internal sealed class SimpleFixedSizeExemplarReservoir : FixedSizeExemplarReservoir { - private volatile int measurementsSeen; + private int measurementsSeen; public SimpleFixedSizeExemplarReservoir() : this(Environment.ProcessorCount) @@ -41,7 +41,7 @@ protected override void OnCollectionCompleted() // Reset internal state irrespective of temporality. // This ensures incoming measurements have fair chance // of making it to the reservoir. - this.measurementsSeen = 0; + Volatile.Write(ref this.measurementsSeen, 0); } private void Offer(in ExemplarMeasurement measurement) From ee7a978204b6b1cab3cda6175d45ccea6067d222 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Sat, 17 Feb 2024 21:25:44 -0800 Subject: [PATCH 20/29] Tweak. --- .../Exemplar/FixedSizeExemplarReservoir.cs | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index e7f3efd6467..3a778465eda 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -95,13 +95,24 @@ protected virtual void OnCollectionCompleted() protected void UpdateExemplar(int exemplarIndex, in ExemplarMeasurement measurement) where T : struct { - var activeBuffer = Volatile.Read(ref this.activeBuffer); - if (activeBuffer == null) + var activeBuffer = Volatile.Read(ref this.activeBuffer) + ?? this.AcquireActiveBufferRare(); + + activeBuffer[exemplarIndex].Update(in measurement); + } + + private Exemplar[] AcquireActiveBufferRare() + { + // Note: We reach here if performing a write while racing with collect. + + Exemplar[]? activeBuffer; + var spinWait = default(SpinWait); + do { - // Note: This is expected to happen when we race with a collection. - return; + spinWait.SpinOnce(); } + while ((activeBuffer = Volatile.Read(ref this.activeBuffer)) == null); - activeBuffer[exemplarIndex].Update(in measurement); + return activeBuffer; } } From 6ccfd4d4ecf0577ab7364e80ef75e71d59b527d1 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Sat, 17 Feb 2024 22:36:00 -0800 Subject: [PATCH 21/29] Tweaks. --- .../Exemplar/FixedSizeExemplarReservoir.cs | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index 3a778465eda..bd57d121651 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -34,13 +34,13 @@ protected FixedSizeExemplarReservoir(int capacity) /// . public sealed override ReadOnlyExemplarCollection Collect() { - var currentBuffer = Volatile.Read(ref this.activeBuffer); + var activeBuffer = Volatile.Read(ref this.activeBuffer); - Debug.Assert(currentBuffer != null, "currentBuffer was null"); + Debug.Assert(activeBuffer != null, "activeBuffer was null"); Volatile.Write(ref this.activeBuffer, null); - var inactiveBuffer = currentBuffer == this.bufferA + var inactiveBuffer = activeBuffer == this.bufferA ? this.bufferB : this.bufferA; @@ -51,12 +51,40 @@ public sealed override ReadOnlyExemplarCollection Collect() inactiveBuffer[i].Reset(); } } + else + { +#if NET6_0_OR_GREATER + var length = this.Capacity; + ref var inactive = ref MemoryMarshal.GetArrayDataReference(inactiveBuffer); + ref var active = ref MemoryMarshal.GetArrayDataReference(activeBuffer); + do + { + if (active.IsUpdated()) + { + active.Copy(ref inactive); + } + + inactive = ref Unsafe.Add(ref inactive, 1); + active = ref Unsafe.Add(ref active, 1); + } + while (--length > 0); +#else + for (int i = 0; i <= activeBuffer!.Length; i++) + { + ref var active = ref activeBuffer[i]; + if (active.IsUpdated()) + { + active.Copy(ref inactiveBuffer[i]); + } + } +#endif + } this.OnCollectionCompleted(); Volatile.Write(ref this.activeBuffer, inactiveBuffer); - return new(currentBuffer!); + return new(activeBuffer!); } internal sealed override void Initialize(AggregatorStore aggregatorStore) From b2b327aa47a0cc49dcaeccdf21f76a13c2ccd424 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Sat, 17 Feb 2024 22:39:15 -0800 Subject: [PATCH 22/29] Tweak. --- .../Metrics/Exemplar/FixedSizeExemplarReservoir.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index bd57d121651..2bd6d275388 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -69,7 +69,7 @@ public sealed override ReadOnlyExemplarCollection Collect() } while (--length > 0); #else - for (int i = 0; i <= activeBuffer!.Length; i++) + for (int i = 0; i < activeBuffer!.Length; i++) { ref var active = ref activeBuffer[i]; if (active.IsUpdated()) From a6011de62db0328f91ace037dfcceb332e67bb41 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 20 Feb 2024 15:14:37 -0800 Subject: [PATCH 23/29] WIP --- .../Exemplar/ReadOnlyExemplarCollection.cs | 12 ++++++++++ .../Metrics/MetricExemplarTests.cs | 24 ++++++++++++------- .../Metrics/MetricTestsBase.cs | 6 ++--- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs b/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs index ceff5d21696..6d58aed7d38 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/ReadOnlyExemplarCollection.cs @@ -61,6 +61,18 @@ internal ReadOnlyExemplarCollection Copy() return new ReadOnlyExemplarCollection(exemplarCopies); } + internal IReadOnlyList ToReadOnlyList() + { + var list = new List(this.MaximumCount); + + foreach (var item in this) + { + list.Add(item); + } + + return list; + } + /// /// Enumerates the elements of a . /// diff --git a/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs index 72c16b1f70f..2f08749b187 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs @@ -38,7 +38,7 @@ public void TestExemplarsCounter(MetricReaderTemporalityPreference temporality) metricReaderOptions.TemporalityPreference = temporality; })); - var measurementValues = GenerateRandomValues(10); + var measurementValues = GenerateRandomValues(2); foreach (var value in measurementValues) { counter.Add(value); @@ -49,13 +49,8 @@ public void TestExemplarsCounter(MetricReaderTemporalityPreference temporality) Assert.NotNull(metricPoint); Assert.True(metricPoint.Value.StartTime >= testStartTime); Assert.True(metricPoint.Value.EndTime != default); - var exemplars = GetExemplars(metricPoint.Value); - // TODO: Modify the test to better test cumulative. - // In cumulative, where SimpleFixedSizeExemplarReservoir's size is - // more than the count of new measurements, it is possible - // that the exemplar value is for a measurement that was recorded in the prior - // cycle. The current ValidateExemplars() does not handle this case. + var exemplars = GetExemplars(metricPoint.Value); ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, measurementValues, false); exportedItems.Clear(); @@ -64,7 +59,7 @@ public void TestExemplarsCounter(MetricReaderTemporalityPreference temporality) Thread.Sleep(10); // Compensates for low resolution timing in netfx. #endif - measurementValues = GenerateRandomValues(10); + measurementValues = GenerateRandomValues(1); foreach (var value in measurementValues) { var act = new Activity("test").Start(); @@ -77,7 +72,18 @@ public void TestExemplarsCounter(MetricReaderTemporalityPreference temporality) Assert.NotNull(metricPoint); Assert.True(metricPoint.Value.StartTime >= testStartTime); Assert.True(metricPoint.Value.EndTime != default); + exemplars = GetExemplars(metricPoint.Value); + + if (temporality == MetricReaderTemporalityPreference.Cumulative) + { + Assert.Equal(2, exemplars.Count); + } + else + { + Assert.Single(exemplars); + } + ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, measurementValues, true); } @@ -187,7 +193,7 @@ private static double[] GenerateRandomValues(int count) return values; } - private static void ValidateExemplars(ReadOnlyExemplarCollection exemplars, DateTimeOffset startTime, DateTimeOffset endTime, double[] measurementValues, bool traceContextExists) + private static void ValidateExemplars(IReadOnlyList exemplars, DateTimeOffset startTime, DateTimeOffset endTime, double[] measurementValues, bool traceContextExists) { foreach (var exemplar in exemplars) { diff --git a/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs index ad2d96c5b3b..6d18bed47de 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricTestsBase.cs @@ -233,14 +233,14 @@ public IDisposable BuildMeterProvider( #endif } - internal static ReadOnlyExemplarCollection GetExemplars(MetricPoint mp) + internal static IReadOnlyList GetExemplars(MetricPoint mp) { if (mp.TryGetExemplars(out var exemplars)) { - return exemplars.Value; + return exemplars.Value.ToReadOnlyList(); } - return new ReadOnlyExemplarCollection(Array.Empty()); + return Array.Empty(); } #if BUILDING_HOSTING_TESTS From 015670aba405e002178d64d34847c8b6093222fe Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 20 Feb 2024 20:21:43 -0800 Subject: [PATCH 24/29] Tweaks. --- src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs | 6 +++--- .../Exemplar/FixedSizeExemplarReservoir.cs | 18 +++++++++--------- .../SimpleFixedSizeExemplarReservoir.cs | 7 ++----- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs index b2e9a8a7c25..f1d9862ee37 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs @@ -114,7 +114,7 @@ internal void Update(in ExemplarMeasurement measurement) this.StoreRawTags(measurement.Tags); - Volatile.Write(ref this.isCriticalSectionOccupied, 0); + Interlocked.Exchange(ref this.isCriticalSectionOccupied, 0); } internal void Reset() @@ -124,7 +124,7 @@ internal void Reset() internal readonly bool IsUpdated() { - if (Volatile.Read(ref Unsafe.AsRef(in this.isCriticalSectionOccupied)) != 0) + if (Interlocked.CompareExchange(ref Unsafe.AsRef(in this.isCriticalSectionOccupied), 0, 0) != 0) { this.WaitForUpdateToCompleteRare(); return true; @@ -173,6 +173,6 @@ private readonly void WaitForUpdateToCompleteRare() { spinWait.SpinOnce(); } - while (Volatile.Read(ref Unsafe.AsRef(in this.isCriticalSectionOccupied)) != 0); + while (Interlocked.CompareExchange(ref Unsafe.AsRef(in this.isCriticalSectionOccupied), 0, 0) != 0); } } diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index 2bd6d275388..ad4a7127e62 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -12,6 +12,7 @@ namespace OpenTelemetry.Metrics; internal abstract class FixedSizeExemplarReservoir : ExemplarReservoir { + private static readonly Exemplar[] NeverMatchExemplarBuffer = new Exemplar[0]; private readonly Exemplar[] bufferA; private readonly Exemplar[] bufferB; private Exemplar[]? activeBuffer; @@ -34,12 +35,10 @@ protected FixedSizeExemplarReservoir(int capacity) /// . public sealed override ReadOnlyExemplarCollection Collect() { - var activeBuffer = Volatile.Read(ref this.activeBuffer); + var activeBuffer = Interlocked.Exchange(ref this.activeBuffer, null); Debug.Assert(activeBuffer != null, "activeBuffer was null"); - Volatile.Write(ref this.activeBuffer, null); - var inactiveBuffer = activeBuffer == this.bufferA ? this.bufferB : this.bufferA; @@ -50,6 +49,8 @@ public sealed override ReadOnlyExemplarCollection Collect() { inactiveBuffer[i].Reset(); } + + this.OnReset(); } else { @@ -80,9 +81,7 @@ public sealed override ReadOnlyExemplarCollection Collect() #endif } - this.OnCollectionCompleted(); - - Volatile.Write(ref this.activeBuffer, inactiveBuffer); + Interlocked.Exchange(ref this.activeBuffer, inactiveBuffer); return new(activeBuffer!); } @@ -116,14 +115,14 @@ internal sealed override void Initialize(AggregatorStore aggregatorStore) base.Initialize(aggregatorStore); } - protected virtual void OnCollectionCompleted() + protected virtual void OnReset() { } protected void UpdateExemplar(int exemplarIndex, in ExemplarMeasurement measurement) where T : struct { - var activeBuffer = Volatile.Read(ref this.activeBuffer) + var activeBuffer = Interlocked.CompareExchange(ref this.activeBuffer, null, NeverMatchExemplarBuffer) ?? this.AcquireActiveBufferRare(); activeBuffer[exemplarIndex].Update(in measurement); @@ -134,12 +133,13 @@ private Exemplar[] AcquireActiveBufferRare() // Note: We reach here if performing a write while racing with collect. Exemplar[]? activeBuffer; + var spinWait = default(SpinWait); do { spinWait.SpinOnce(); } - while ((activeBuffer = Volatile.Read(ref this.activeBuffer)) == null); + while ((activeBuffer = Interlocked.CompareExchange(ref this.activeBuffer, null, NeverMatchExemplarBuffer)) == null); return activeBuffer; } diff --git a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs index bfab577ddae..e9f65412448 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/SimpleFixedSizeExemplarReservoir.cs @@ -36,12 +36,9 @@ public override void Offer(in ExemplarMeasurement measurement) this.Offer(in measurement); } - protected override void OnCollectionCompleted() + protected override void OnReset() { - // Reset internal state irrespective of temporality. - // This ensures incoming measurements have fair chance - // of making it to the reservoir. - Volatile.Write(ref this.measurementsSeen, 0); + Interlocked.Exchange(ref this.measurementsSeen, 0); } private void Offer(in ExemplarMeasurement measurement) From fb521aad291e5213574c67f645945308bc90014e Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 20 Feb 2024 21:12:59 -0800 Subject: [PATCH 25/29] Test improvements and tweaks. --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 91 ++++++++++++-- src/OpenTelemetry/Metrics/Metric.cs | 13 +- src/OpenTelemetry/Metrics/MetricPoint.cs | 14 ++- src/OpenTelemetry/Metrics/MetricReaderExt.cs | 16 +-- .../Metrics/MetricStreamConfiguration.cs | 3 + .../Metrics/MetricExemplarTests.cs | 118 ++++++++++++------ 6 files changed, 197 insertions(+), 58 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index 70071b5afdb..f8bc9c85a27 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -18,6 +18,7 @@ internal sealed class AggregatorStore internal readonly bool EmitOverflowAttribute; internal readonly ConcurrentDictionary? TagsToMetricPointIndexDictionaryDelta; internal readonly ExemplarSamplingHelper ExemplarSampler; + internal readonly Func? ExemplarReservoirFactory; internal long DroppedMeasurements = 0; private static readonly string MetricPointCapHitFixMessage = "Consider opting in for the experimental SDK feature to emit all the throttled metrics under the overflow attribute by setting env variable OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE = true. You could also modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit."; @@ -60,7 +61,8 @@ internal AggregatorStore( int cardinalityLimit, bool emitOverflowAttribute, bool shouldReclaimUnusedMetricPoints, - ExemplarFilter? exemplarFilter = null) + ExemplarFilter? exemplarFilter = null, + Func? exemplarReservoirFactory = null) { this.name = metricStreamIdentity.InstrumentName; this.CardinalityLimit = cardinalityLimit; @@ -75,6 +77,7 @@ internal AggregatorStore( this.exponentialHistogramMaxScale = metricStreamIdentity.ExponentialHistogramMaxScale; this.StartTimeExclusive = DateTimeOffset.UtcNow; this.exemplarFilter = exemplarFilter ?? DefaultExemplarFilter; + this.ExemplarReservoirFactory = exemplarReservoirFactory; if (metricStreamIdentity.TagKeys == null) { this.updateLongCallback = this.UpdateLong; @@ -345,11 +348,24 @@ private void InitializeZeroTagPointIfNotInitialized() if (this.OutputDelta) { var lookupData = new LookupData(0, Tags.EmptyTags, Tags.EmptyTags); - this.metricPoints[0] = new MetricPoint(this, this.aggType, null, this.histogramBounds, this.exponentialHistogramMaxSize, this.exponentialHistogramMaxScale, lookupData); + this.metricPoints[0] = new MetricPoint( + this, + this.aggType, + tagKeysAndValues: null, + this.histogramBounds, + this.exponentialHistogramMaxSize, + this.exponentialHistogramMaxScale, + lookupData); } else { - this.metricPoints[0] = new MetricPoint(this, this.aggType, null, this.histogramBounds, this.exponentialHistogramMaxSize, this.exponentialHistogramMaxScale); + this.metricPoints[0] = new MetricPoint( + this, + this.aggType, + tagKeysAndValues: null, + this.histogramBounds, + this.exponentialHistogramMaxSize, + this.exponentialHistogramMaxScale); } this.zeroTagMetricPointInitialized = true; @@ -373,11 +389,24 @@ private void InitializeOverflowTagPointIfNotInitialized() if (this.OutputDelta) { var lookupData = new LookupData(1, tags, tags); - this.metricPoints[1] = new MetricPoint(this, this.aggType, keyValuePairs, this.histogramBounds, this.exponentialHistogramMaxSize, this.exponentialHistogramMaxScale, lookupData); + this.metricPoints[1] = new MetricPoint( + this, + this.aggType, + keyValuePairs, + this.histogramBounds, + this.exponentialHistogramMaxSize, + this.exponentialHistogramMaxScale, + lookupData); } else { - this.metricPoints[1] = new MetricPoint(this, this.aggType, keyValuePairs, this.histogramBounds, this.exponentialHistogramMaxSize, this.exponentialHistogramMaxScale); + this.metricPoints[1] = new MetricPoint( + this, + this.aggType, + keyValuePairs, + this.histogramBounds, + this.exponentialHistogramMaxSize, + this.exponentialHistogramMaxScale); } this.overflowTagMetricPointInitialized = true; @@ -446,7 +475,13 @@ private int LookupAggregatorStore(KeyValuePair[] tagKeysAndValu } ref var metricPoint = ref this.metricPoints[aggregatorIndex]; - metricPoint = new MetricPoint(this, this.aggType, sortedTags.KeyValuePairs, this.histogramBounds, this.exponentialHistogramMaxSize, this.exponentialHistogramMaxScale); + metricPoint = new MetricPoint( + this, + this.aggType, + sortedTags.KeyValuePairs, + this.histogramBounds, + this.exponentialHistogramMaxSize, + this.exponentialHistogramMaxScale); // Add to dictionary *after* initializing MetricPoint // as other threads can start writing to the @@ -495,7 +530,13 @@ private int LookupAggregatorStore(KeyValuePair[] tagKeysAndValu } ref var metricPoint = ref this.metricPoints[aggregatorIndex]; - metricPoint = new MetricPoint(this, this.aggType, givenTags.KeyValuePairs, this.histogramBounds, this.exponentialHistogramMaxSize, this.exponentialHistogramMaxScale); + metricPoint = new MetricPoint( + this, + this.aggType, + givenTags.KeyValuePairs, + this.histogramBounds, + this.exponentialHistogramMaxSize, + this.exponentialHistogramMaxScale); // Add to dictionary *after* initializing MetricPoint // as other threads can start writing to the @@ -571,7 +612,14 @@ private int LookupAggregatorStoreForDeltaWithReclaim(KeyValuePair? exemplarReservoirFactory = null) { this.InstrumentIdentity = instrumentIdentity; @@ -155,7 +156,15 @@ internal Metric( throw new NotSupportedException($"Unsupported Instrument Type: {instrumentIdentity.InstrumentType.FullName}"); } - this.AggregatorStore = new AggregatorStore(instrumentIdentity, aggType, temporality, cardinalityLimit, emitOverflowAttribute, shouldReclaimUnusedMetricPoints, exemplarFilter); + this.AggregatorStore = new AggregatorStore( + instrumentIdentity, + aggType, + temporality, + cardinalityLimit, + emitOverflowAttribute, + shouldReclaimUnusedMetricPoints, + exemplarFilter, + exemplarReservoirFactory); this.Temporality = temporality; } diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index eb8e43bacb8..573b2ce6ae4 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -62,7 +62,17 @@ internal MetricPoint( var isExemplarEnabled = aggregatorStore!.IsExemplarEnabled(); - ExemplarReservoir? reservoir = null; + ExemplarReservoir? reservoir; + try + { + reservoir = aggregatorStore.ExemplarReservoirFactory?.Invoke(); + } + catch + { + // todo: Log + reservoir = null; + } + if (this.aggType == AggregationType.HistogramWithBuckets || this.aggType == AggregationType.HistogramWithMinMaxBuckets) { @@ -70,7 +80,7 @@ internal MetricPoint( this.mpComponents.HistogramBuckets = new HistogramBuckets(histogramExplicitBounds); if (isExemplarEnabled) { - reservoir = new AlignedHistogramBucketExemplarReservoir(histogramExplicitBounds!.Length); + reservoir ??= new AlignedHistogramBucketExemplarReservoir(histogramExplicitBounds!.Length + 1); } } else if (this.aggType == AggregationType.Histogram || diff --git a/src/OpenTelemetry/Metrics/MetricReaderExt.cs b/src/OpenTelemetry/Metrics/MetricReaderExt.cs index 9f3b6fa10d2..6b78958e6a5 100644 --- a/src/OpenTelemetry/Metrics/MetricReaderExt.cs +++ b/src/OpenTelemetry/Metrics/MetricReaderExt.cs @@ -147,14 +147,14 @@ internal virtual List AddMetricWithViews(Instrument instrument, List? ExemplarReservoirFactory { get; set; } + internal string[]? CopiedTagKeys { get; private set; } internal int? ViewId { get; set; } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs index 2f08749b187..ebd8afdb0bf 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs @@ -1,23 +1,18 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#nullable enable + using System.Diagnostics; using System.Diagnostics.Metrics; using OpenTelemetry.Tests; using Xunit; -using Xunit.Abstractions; namespace OpenTelemetry.Metrics.Tests; public class MetricExemplarTests : MetricTestsBase { private const int MaxTimeToAllowForFlush = 10000; - private readonly ITestOutputHelper output; - - public MetricExemplarTests(ITestOutputHelper output) - { - this.output = output; - } [Theory] [InlineData(MetricReaderTemporalityPreference.Cumulative)] @@ -33,15 +28,21 @@ public void TestExemplarsCounter(MetricReaderTemporalityPreference temporality) using var container = this.BuildMeterProvider(out var meterProvider, builder => builder .AddMeter(meter.Name) .SetExemplarFilter(new AlwaysOnExemplarFilter()) + .AddView( + "testCounter", + new MetricStreamConfiguration + { + ExemplarReservoirFactory = () => new SimpleFixedSizeExemplarReservoir(3), + }) .AddInMemoryExporter(exportedItems, metricReaderOptions => { metricReaderOptions.TemporalityPreference = temporality; })); - var measurementValues = GenerateRandomValues(2); + var measurementValues = GenerateRandomValues(2, false, null); foreach (var value in measurementValues) { - counter.Add(value); + counter.Add(value.Value); } meterProvider.ForceFlush(MaxTimeToAllowForFlush); @@ -51,7 +52,7 @@ public void TestExemplarsCounter(MetricReaderTemporalityPreference temporality) Assert.True(metricPoint.Value.EndTime != default); var exemplars = GetExemplars(metricPoint.Value); - ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, measurementValues, false); + ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, measurementValues); exportedItems.Clear(); @@ -59,12 +60,11 @@ public void TestExemplarsCounter(MetricReaderTemporalityPreference temporality) Thread.Sleep(10); // Compensates for low resolution timing in netfx. #endif - measurementValues = GenerateRandomValues(1); - foreach (var value in measurementValues) + var secondMeasurementValues = GenerateRandomValues(1, true, measurementValues); + foreach (var value in secondMeasurementValues) { - var act = new Activity("test").Start(); - counter.Add(value); - act.Stop(); + using var act = new Activity("test").Start(); + counter.Add(value.Value); } meterProvider.ForceFlush(MaxTimeToAllowForFlush); @@ -77,18 +77,21 @@ public void TestExemplarsCounter(MetricReaderTemporalityPreference temporality) if (temporality == MetricReaderTemporalityPreference.Cumulative) { - Assert.Equal(2, exemplars.Count); + Assert.Equal(3, exemplars.Count); + secondMeasurementValues = secondMeasurementValues.Concat(measurementValues).ToArray(); } else { Assert.Single(exemplars); } - ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, measurementValues, true); + ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, secondMeasurementValues); } - [Fact] - public void TestExemplarsHistogram() + [Theory] + [InlineData(MetricReaderTemporalityPreference.Cumulative)] + [InlineData(MetricReaderTemporalityPreference.Delta)] + public void TestExemplarsHistogram(MetricReaderTemporalityPreference temporality) { DateTime testStartTime = DateTime.UtcNow; var exportedItems = new List(); @@ -96,18 +99,30 @@ public void TestExemplarsHistogram() using var meter = new Meter($"{Utils.GetCurrentMethodName()}"); var histogram = meter.CreateHistogram("testHistogram"); + var buckets = new double[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }; + using var container = this.BuildMeterProvider(out var meterProvider, builder => builder .AddMeter(meter.Name) .SetExemplarFilter(new AlwaysOnExemplarFilter()) + .AddView( + "testHistogram", + new ExplicitBucketHistogramConfiguration + { + Boundaries = buckets, + }) .AddInMemoryExporter(exportedItems, metricReaderOptions => { - metricReaderOptions.TemporalityPreference = MetricReaderTemporalityPreference.Delta; + metricReaderOptions.TemporalityPreference = temporality; })); - var measurementValues = GenerateRandomValues(10); + var measurementValues = buckets + /* 2000 is here to test overflow measurement */ + .Concat(new double[] { 2000 }) + .Select(b => (Value: b, ExpectTraceId: false)) + .ToArray(); foreach (var value in measurementValues) { - histogram.Record(value); + histogram.Record(value.Value); } meterProvider.ForceFlush(MaxTimeToAllowForFlush); @@ -116,7 +131,7 @@ public void TestExemplarsHistogram() Assert.True(metricPoint.Value.StartTime >= testStartTime); Assert.True(metricPoint.Value.EndTime != default); var exemplars = GetExemplars(metricPoint.Value); - ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, measurementValues, false); + ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, measurementValues); exportedItems.Clear(); @@ -124,11 +139,11 @@ public void TestExemplarsHistogram() Thread.Sleep(10); // Compensates for low resolution timing in netfx. #endif - measurementValues = GenerateRandomValues(10); - foreach (var value in measurementValues) + var secondMeasurementValues = buckets.Take(1).Select(b => (Value: b, ExpectTraceId: true)).ToArray(); + foreach (var value in secondMeasurementValues) { using var act = new Activity("test").Start(); - histogram.Record(value); + histogram.Record(value.Value); } meterProvider.ForceFlush(MaxTimeToAllowForFlush); @@ -136,8 +151,20 @@ public void TestExemplarsHistogram() Assert.NotNull(metricPoint); Assert.True(metricPoint.Value.StartTime >= testStartTime); Assert.True(metricPoint.Value.EndTime != default); + exemplars = GetExemplars(metricPoint.Value); - ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, measurementValues, true); + + if (temporality == MetricReaderTemporalityPreference.Cumulative) + { + Assert.Equal(11, exemplars.Count); + secondMeasurementValues = secondMeasurementValues.Concat(measurementValues).ToArray(); + } + else + { + Assert.Single(exemplars); + } + + ValidateExemplars(exemplars, metricPoint.Value.StartTime, metricPoint.Value.EndTime, secondMeasurementValues); } [Fact] @@ -158,10 +185,14 @@ public void TestExemplarsFilterTags() metricReaderOptions.TemporalityPreference = MetricReaderTemporalityPreference.Delta; })); - var measurementValues = GenerateRandomValues(10); + var measurementValues = GenerateRandomValues(10, false, null); foreach (var value in measurementValues) { - histogram.Record(value, new("key1", "value1"), new("key2", "value1"), new("key3", "value1")); + histogram.Record( + value.Value, + new("key1", "value1"), + new("key2", "value1"), + new("key3", "value1")); } meterProvider.ForceFlush(MaxTimeToAllowForFlush); @@ -181,26 +212,43 @@ public void TestExemplarsFilterTags() } } - private static double[] GenerateRandomValues(int count) + private static (double Value, bool ExpectTraceId)[] GenerateRandomValues( + int count, + bool expectTraceId, + (double Value, bool ExpectTraceId)[]? previousValues) { var random = new Random(); - var values = new double[count]; + var values = new (double, bool)[count]; for (int i = 0; i < count; i++) { - values[i] = random.NextDouble(); + var nextValue = random.NextDouble(); + if (values.Any(m => m.Item1 == nextValue) + || previousValues?.Any(m => m.Value == nextValue) == true) + { + i--; + continue; + } + + values[i] = (nextValue, expectTraceId); } return values; } - private static void ValidateExemplars(IReadOnlyList exemplars, DateTimeOffset startTime, DateTimeOffset endTime, double[] measurementValues, bool traceContextExists) + private static void ValidateExemplars( + IReadOnlyList exemplars, + DateTimeOffset startTime, + DateTimeOffset endTime, + (double Value, bool ExpectTraceId)[] measurementValues) { foreach (var exemplar in exemplars) { Assert.True(exemplar.Timestamp >= startTime && exemplar.Timestamp <= endTime, $"{startTime} < {exemplar.Timestamp} < {endTime}"); - Assert.Contains(exemplar.DoubleValue, measurementValues); Assert.Equal(0, exemplar.FilteredTags.MaximumCount); - if (traceContextExists) + + var measurement = measurementValues.FirstOrDefault(v => v.Value == exemplar.DoubleValue); + Assert.NotEqual(default, measurement); + if (measurement.ExpectTraceId) { Assert.NotEqual(default, exemplar.TraceId); Assert.NotEqual(default, exemplar.SpanId); From 8bde052e02ced2f91298fb9b03cd54d7f4ad4e2d Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 21 Feb 2024 09:39:29 -0800 Subject: [PATCH 26/29] Code review. --- src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs | 8 +++++++- src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs | 5 +++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs index f1d9862ee37..faae1caeaf8 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs @@ -70,8 +70,14 @@ public double DoubleValue } /// - /// Gets the FilteredTags (i.e any tags that were dropped during aggregation). + /// Gets the filtered tags. /// + /// + /// Note: represents the set of tags which were + /// supplied at measurement but dropped due to filtering configured by a + /// view (). If view tag + /// filtering is not configured will be empty. + /// public readonly ReadOnlyFilteredTagCollection FilteredTags => new(this.KeyFilter, this.tagStorage ?? Array.Empty>(), this.tagCount); diff --git a/src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs b/src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs index 4b0bfd777b8..3dcd4a8d9df 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs @@ -51,6 +51,11 @@ internal ExemplarMeasurement( /// /// Gets the measurement tags. /// + /// + /// Note: represents the full set of tags supplied at + /// measurement regardless of any filtering configured by a view (). + /// public ReadOnlySpan> Tags { get; } internal int ExplicitBucketHistogramBucketIndex { get; } From c2c7f0a42ee61d809a7a44f81b10397df205cf24 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 21 Feb 2024 09:56:47 -0800 Subject: [PATCH 27/29] Code review. --- src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs index faae1caeaf8..ff6626d1a4a 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs @@ -31,6 +31,8 @@ namespace OpenTelemetry.Metrics; struct Exemplar { internal HashSet? KeyFilter; + + private static readonly ReadOnlyFilteredTagCollection Empty = new(keyFilter: null, Array.Empty>(), count: 0); private int tagCount; private KeyValuePair[]? tagStorage; private MetricPointValueStorage valueStorage; @@ -79,7 +81,21 @@ public double DoubleValue /// filtering is not configured will be empty. /// public readonly ReadOnlyFilteredTagCollection FilteredTags - => new(this.KeyFilter, this.tagStorage ?? Array.Empty>(), this.tagCount); + { + get + { + if (this.tagCount == 0) + { + return Empty; + } + else + { + Debug.Assert(this.tagStorage != null, "tagStorage was null"); + + return new(this.KeyFilter, this.tagStorage!, this.tagCount); + } + } + } internal void Update(in ExemplarMeasurement measurement) where T : struct From 0ffba7d6d17cfba223d77057ed764be461807735 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 21 Feb 2024 10:46:30 -0800 Subject: [PATCH 28/29] Code review. --- src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs | 12 ++++-------- .../Metrics/Exemplar/FixedSizeExemplarReservoir.cs | 10 +++++----- src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs | 8 ++++---- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs index ff6626d1a4a..0f8ce59b942 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs @@ -22,17 +22,13 @@ namespace OpenTelemetry.Metrics; #endif public #else -/// -/// Represents an Exemplar data. -/// -#pragma warning disable SA1623 // The property's documentation summary text should begin with: `Gets or sets` internal #endif struct Exemplar { - internal HashSet? KeyFilter; + internal HashSet? ViewDefinedTagKeys; - private static readonly ReadOnlyFilteredTagCollection Empty = new(keyFilter: null, Array.Empty>(), count: 0); + private static readonly ReadOnlyFilteredTagCollection Empty = new(excludedKeys: null, Array.Empty>(), count: 0); private int tagCount; private KeyValuePair[]? tagStorage; private MetricPointValueStorage valueStorage; @@ -92,7 +88,7 @@ public readonly ReadOnlyFilteredTagCollection FilteredTags { Debug.Assert(this.tagStorage != null, "tagStorage was null"); - return new(this.KeyFilter, this.tagStorage!, this.tagCount); + return new(this.ViewDefinedTagKeys, this.tagStorage!, this.tagCount); } } } @@ -161,7 +157,7 @@ internal readonly void Copy(ref Exemplar destination) destination.TraceId = this.TraceId; destination.SpanId = this.SpanId; destination.valueStorage = this.valueStorage; - destination.KeyFilter = this.KeyFilter; + destination.ViewDefinedTagKeys = this.ViewDefinedTagKeys; destination.tagCount = this.tagCount; if (destination.tagCount > 0) { diff --git a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs index ad4a7127e62..bf2c12a5963 100644 --- a/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs +++ b/src/OpenTelemetry/Metrics/Exemplar/FixedSizeExemplarReservoir.cs @@ -88,7 +88,7 @@ public sealed override ReadOnlyExemplarCollection Collect() internal sealed override void Initialize(AggregatorStore aggregatorStore) { - var keyFilter = aggregatorStore.TagKeysInteresting; + var viewDefinedTagKeys = aggregatorStore.TagKeysInteresting; #if NET6_0_OR_GREATER var length = this.bufferA.Length; @@ -96,8 +96,8 @@ internal sealed override void Initialize(AggregatorStore aggregatorStore) ref var b = ref MemoryMarshal.GetArrayDataReference(this.bufferB); do { - a.KeyFilter = keyFilter; - b.KeyFilter = keyFilter; + a.ViewDefinedTagKeys = viewDefinedTagKeys; + b.ViewDefinedTagKeys = viewDefinedTagKeys; a = ref Unsafe.Add(ref a, 1); b = ref Unsafe.Add(ref b, 1); } @@ -107,8 +107,8 @@ internal sealed override void Initialize(AggregatorStore aggregatorStore) i < this.bufferA.Length && i < this.bufferB.Length; i++) { - this.bufferA[i].KeyFilter = keyFilter; - this.bufferB[i].KeyFilter = keyFilter; + this.bufferA[i].ViewDefinedTagKeys = viewDefinedTagKeys; + this.bufferB[i].ViewDefinedTagKeys = viewDefinedTagKeys; } #endif diff --git a/src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs b/src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs index 85f74ae626f..d48e24e40e6 100644 --- a/src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs +++ b/src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs @@ -25,19 +25,19 @@ namespace OpenTelemetry; #endif readonly struct ReadOnlyFilteredTagCollection { - private readonly HashSet? keyFilter; + private readonly HashSet? excludedKeys; private readonly KeyValuePair[] tags; private readonly int count; internal ReadOnlyFilteredTagCollection( - HashSet? keyFilter, + HashSet? excludedKeys, KeyValuePair[] tags, int count) { Debug.Assert(tags != null, "tags was null"); Debug.Assert(count <= tags!.Length, "count was invalid"); - this.keyFilter = keyFilter; + this.excludedKeys = excludedKeys; this.tags = tags; this.count = count; } @@ -107,7 +107,7 @@ public bool MoveNext() { var item = this.source.tags[index]; - if (this.source.keyFilter?.Contains(item.Key) == true) + if (this.source.excludedKeys?.Contains(item.Key) == true) { continue; } From fb9b07ad25c552605eb2a1a065429d32bbd679e6 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 21 Feb 2024 10:53:17 -0800 Subject: [PATCH 29/29] Code review. --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 26 +++++++++----------- src/OpenTelemetry/Metrics/MetricPoint.cs | 8 +++--- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index f8bc9c85a27..c97995b92d9 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -17,7 +17,7 @@ internal sealed class AggregatorStore internal readonly int CardinalityLimit; internal readonly bool EmitOverflowAttribute; internal readonly ConcurrentDictionary? TagsToMetricPointIndexDictionaryDelta; - internal readonly ExemplarSamplingHelper ExemplarSampler; + internal readonly ExemplarFilteringHelper ExemplarFilter; internal readonly Func? ExemplarReservoirFactory; internal long DroppedMeasurements = 0; @@ -45,7 +45,6 @@ internal sealed class AggregatorStore private readonly int exponentialHistogramMaxScale; private readonly UpdateLongDelegate updateLongCallback; private readonly UpdateDoubleDelegate updateDoubleCallback; - private readonly ExemplarFilter exemplarFilter; private readonly Func[], int, int> lookupAggregatorStore; private int metricPointIndex = 0; @@ -76,7 +75,6 @@ internal AggregatorStore( this.exponentialHistogramMaxSize = metricStreamIdentity.ExponentialHistogramMaxSize; this.exponentialHistogramMaxScale = metricStreamIdentity.ExponentialHistogramMaxScale; this.StartTimeExclusive = DateTimeOffset.UtcNow; - this.exemplarFilter = exemplarFilter ?? DefaultExemplarFilter; this.ExemplarReservoirFactory = exemplarReservoirFactory; if (metricStreamIdentity.TagKeys == null) { @@ -130,7 +128,7 @@ internal AggregatorStore( this.lookupAggregatorStore = this.LookupAggregatorStore; } - this.ExemplarSampler = new(this.exemplarFilter); + this.ExemplarFilter = new(exemplarFilter ?? DefaultExemplarFilter); } private delegate void UpdateLongDelegate(long value, ReadOnlySpan> tags); @@ -144,11 +142,7 @@ internal AggregatorStore( internal double[] HistogramBounds => this.histogramBounds; internal bool IsExemplarEnabled() - { - // Using this filter to indicate On/Off - // instead of another separate flag. - return this.exemplarFilter is not AlwaysOffExemplarFilter; - } + => this.ExemplarFilter.Enabled; internal void Update(long value, ReadOnlySpan> tags) { @@ -1141,30 +1135,34 @@ private int FindMetricAggregatorsCustomTag(ReadOnlySpan ShouldSampleLong; - public ShouldSampleFunc ShouldSampleDouble; + public readonly bool Enabled; + public readonly bool? EarlySampleDecision; + public readonly ShouldSampleFunc ShouldSampleLong; + public readonly ShouldSampleFunc ShouldSampleDouble; - public ExemplarSamplingHelper(ExemplarFilter exemplarFilter) + public ExemplarFilteringHelper(ExemplarFilter exemplarFilter) { Debug.Assert(exemplarFilter != null, "exemplarFilter was null"); if (exemplarFilter is AlwaysOffExemplarFilter) { + this.Enabled = false; this.EarlySampleDecision = false; this.ShouldSampleLong = static (_, _) => false; this.ShouldSampleDouble = static (_, _) => false; } else if (exemplarFilter is AlwaysOnExemplarFilter) { + this.Enabled = true; this.EarlySampleDecision = true; this.ShouldSampleLong = static (_, _) => true; this.ShouldSampleDouble = static (_, _) => true; } else { + this.Enabled = true; this.EarlySampleDecision = null; this.ShouldSampleLong = exemplarFilter!.ShouldSample; this.ShouldSampleDouble = exemplarFilter.ShouldSample; diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index 573b2ce6ae4..01693e61622 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -383,8 +383,8 @@ internal void Update(long number, ReadOnlySpan> ta { this.Update(number, out var explicitBucketHistogramBucketIndex); - var exemplarSampler = this.aggregatorStore.ExemplarSampler; - if (exemplarSampler.EarlySampleDecision ?? exemplarSampler.ShouldSampleLong(number, tags)) + var exemplarFilter = this.aggregatorStore.ExemplarFilter; + if (exemplarFilter.EarlySampleDecision ?? exemplarFilter.ShouldSampleLong(number, tags)) { Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null"); @@ -399,8 +399,8 @@ internal void Update(double number, ReadOnlySpan> { this.Update(number, out var explicitBucketHistogramBucketIndex); - var exemplarSampler = this.aggregatorStore.ExemplarSampler; - if (exemplarSampler.EarlySampleDecision ?? exemplarSampler.ShouldSampleDouble(number, tags)) + var exemplarFilter = this.aggregatorStore.ExemplarFilter; + if (exemplarFilter.EarlySampleDecision ?? exemplarFilter.ShouldSampleDouble(number, tags)) { Debug.Assert(this.mpComponents?.ExemplarReservoir != null, "ExemplarReservoir was null");