From 7f82ce02899e3b37f7d62c4deade42eb1fb272c7 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Tue, 23 Nov 2021 00:32:29 -0800 Subject: [PATCH 1/4] Update HistogramMeasurement --- .../ConsoleMetricExporter.cs | 52 ++++++------ .../Implementation/MetricItemExtensions.cs | 13 +-- .../Implementation/PrometheusSerializerExt.cs | 61 ++++++-------- .../.publicApi/net461/PublicAPI.Unshipped.txt | 13 ++- .../netstandard2.0/PublicAPI.Unshipped.txt | 13 ++- .../Metrics/HistogramMeasurement.cs | 31 +++++++ .../Metrics/HistogramMeasurements.cs | 41 +++++++++- src/OpenTelemetry/Metrics/MetricPoint.cs | 81 +++++++------------ .../Metrics/AggregatorTest.cs | 30 ++++--- .../Metrics/MetricViewTests.cs | 39 +++++---- 10 files changed, 221 insertions(+), 153 deletions(-) create mode 100644 src/OpenTelemetry/Metrics/HistogramMeasurement.cs diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs index 3465f413143..1d4c7477b5a 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs @@ -99,42 +99,40 @@ public override ExportResult Export(in Batch batch) var count = metricPoint.GetHistogramCount(); bucketsBuilder.Append($"Sum: {sum} Count: {count} \n"); - var explicitBounds = metricPoint.GetExplicitBounds(); - if (explicitBounds != null) + bool isFirstIteration = true; + double previousExplicitBound = default; + foreach (var histogramMeasurement in metricPoint.HistogramMeasurements) { - var bucketCounts = metricPoint.GetBucketCounts(); - for (int i = 0; i < explicitBounds.Length + 1; i++) + if (isFirstIteration) { - if (i == 0) - { - bucketsBuilder.Append("(-Infinity,"); - bucketsBuilder.Append(explicitBounds[i]); - bucketsBuilder.Append(']'); - bucketsBuilder.Append(':'); - bucketsBuilder.Append(bucketCounts[i]); - } - else if (i == explicitBounds.Length) + bucketsBuilder.Append("(-Infinity,"); + bucketsBuilder.Append(histogramMeasurement.ExplicitBound); + bucketsBuilder.Append(']'); + bucketsBuilder.Append(':'); + bucketsBuilder.Append(histogramMeasurement.BucketCount); + previousExplicitBound = histogramMeasurement.ExplicitBound; + isFirstIteration = false; + } + else + { + bucketsBuilder.Append('('); + bucketsBuilder.Append(previousExplicitBound); + bucketsBuilder.Append(','); + if (histogramMeasurement.ExplicitBound != double.PositiveInfinity) { - bucketsBuilder.Append('('); - bucketsBuilder.Append(explicitBounds[i - 1]); - bucketsBuilder.Append(','); - bucketsBuilder.Append("+Infinity]"); - bucketsBuilder.Append(':'); - bucketsBuilder.Append(bucketCounts[i]); + bucketsBuilder.Append(histogramMeasurement.ExplicitBound); } else { - bucketsBuilder.Append('('); - bucketsBuilder.Append(explicitBounds[i - 1]); - bucketsBuilder.Append(','); - bucketsBuilder.Append(explicitBounds[i]); - bucketsBuilder.Append(']'); - bucketsBuilder.Append(':'); - bucketsBuilder.Append(bucketCounts[i]); + bucketsBuilder.Append("+Infinity"); } - bucketsBuilder.AppendLine(); + bucketsBuilder.Append(']'); + bucketsBuilder.Append(':'); + bucketsBuilder.Append(histogramMeasurement.BucketCount); } + + bucketsBuilder.AppendLine(); } valueDisplay = bucketsBuilder.ToString(); diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs index 1857c899ddb..9227d1b0d81 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs @@ -249,17 +249,12 @@ internal static OtlpMetrics.Metric ToOtlpMetric(this Metric metric) dataPoint.Count = (ulong)metricPoint.LongValue; dataPoint.Sum = metricPoint.DoubleValue; - var bucketCounts = metricPoint.GetBucketCounts(); - if (bucketCounts != null) + foreach (var histogramMeasurement in metricPoint.HistogramMeasurements) { - var explicitBounds = metricPoint.GetExplicitBounds(); - for (int i = 0; i < bucketCounts.Length; i++) + dataPoint.BucketCounts.Add((ulong)histogramMeasurement.BucketCount); + if (histogramMeasurement.ExplicitBound != double.PositiveInfinity) { - dataPoint.BucketCounts.Add((ulong)bucketCounts[i]); - if (i < bucketCounts.Length - 1) - { - dataPoint.ExplicitBounds.Add(explicitBounds[i]); - } + dataPoint.ExplicitBounds.Add(histogramMeasurement.ExplicitBound); } } diff --git a/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs b/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs index 59b727a86eb..5ce8e64f517 100644 --- a/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs +++ b/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs @@ -91,51 +91,42 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) var tags = metricPoint.Tags; var timestamp = metricPoint.EndTime.ToUnixTimeMilliseconds(); - var bucketCounts = metricPoint.GetBucketCounts(); - if (bucketCounts != null) + long totalCount = 0; + foreach (var histogramMeasurement in metricPoint.HistogramMeasurements) { - // Histogram buckets - var explicitBounds = metricPoint.GetExplicitBounds(); - long totalCount = 0; - for (int idxBound = 0; idxBound < explicitBounds.Length + 1; idxBound++) - { - totalCount += bucketCounts[idxBound]; + totalCount += histogramMeasurement.BucketCount; - cursor = WriteMetricName(buffer, cursor, metric.Name, metric.Unit); - cursor = WriteAsciiStringNoEscape(buffer, cursor, "_bucket{"); + cursor = WriteMetricName(buffer, cursor, metric.Name, metric.Unit); + cursor = WriteAsciiStringNoEscape(buffer, cursor, "_bucket{"); - foreach (var tag in tags) - { - cursor = WriteLabel(buffer, cursor, tag.Key, tag.Value); - buffer[cursor++] = unchecked((byte)','); - } + foreach (var tag in tags) + { + cursor = WriteLabel(buffer, cursor, tag.Key, tag.Value); + buffer[cursor++] = unchecked((byte)','); + } - cursor = WriteAsciiStringNoEscape(buffer, cursor, "le=\""); + cursor = WriteAsciiStringNoEscape(buffer, cursor, "le=\""); - if (idxBound < explicitBounds.Length) - { - cursor = WriteDouble(buffer, cursor, explicitBounds[idxBound]); - } - else - { - cursor = WriteAsciiStringNoEscape(buffer, cursor, "+Inf"); - } + if (histogramMeasurement.ExplicitBound != double.PositiveInfinity) + { + cursor = WriteDouble(buffer, cursor, histogramMeasurement.ExplicitBound); + } + else + { + cursor = WriteAsciiStringNoEscape(buffer, cursor, "+Inf"); + } - cursor = WriteAsciiStringNoEscape(buffer, cursor, "\"} "); + cursor = WriteAsciiStringNoEscape(buffer, cursor, "\"} "); - cursor = WriteLong(buffer, cursor, totalCount); - buffer[cursor++] = unchecked((byte)' '); + cursor = WriteLong(buffer, cursor, totalCount); + buffer[cursor++] = unchecked((byte)' '); - cursor = WriteLong(buffer, cursor, timestamp); + cursor = WriteLong(buffer, cursor, timestamp); - buffer[cursor++] = ASCII_LINEFEED; - } + buffer[cursor++] = ASCII_LINEFEED; } // Histogram sum - var count = metricPoint.GetHistogramCount(); - var sum = metricPoint.GetHistogramSum(); - cursor = WriteMetricName(buffer, cursor, metric.Name, metric.Unit); cursor = WriteAsciiStringNoEscape(buffer, cursor, "_sum"); @@ -159,7 +150,7 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) buffer[cursor++] = unchecked((byte)' '); - cursor = WriteDouble(buffer, cursor, sum); + cursor = WriteDouble(buffer, cursor, metricPoint.GetHistogramSum()); buffer[cursor++] = unchecked((byte)' '); cursor = WriteLong(buffer, cursor, timestamp); @@ -190,7 +181,7 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) buffer[cursor++] = unchecked((byte)' '); - cursor = WriteLong(buffer, cursor, count); + cursor = WriteLong(buffer, cursor, metricPoint.GetHistogramCount()); buffer[cursor++] = unchecked((byte)' '); cursor = WriteLong(buffer, cursor, timestamp); diff --git a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt index 9bfa7df92f2..408f111f89d 100644 --- a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt @@ -13,6 +13,17 @@ OpenTelemetry.Metrics.AggregationTemporalityAttribute.Supported.get -> OpenTelem OpenTelemetry.Metrics.BaseExportingMetricReader OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter exporter) -> void OpenTelemetry.Metrics.BaseExportingMetricReader.SupportedExportModes.get -> OpenTelemetry.Metrics.ExportModes +OpenTelemetry.Metrics.HistogramMeasurement +OpenTelemetry.Metrics.HistogramMeasurement.BucketCount.get -> long +OpenTelemetry.Metrics.HistogramMeasurement.ExplicitBound.get -> double +OpenTelemetry.Metrics.HistogramMeasurement.HistogramMeasurement() -> void +OpenTelemetry.Metrics.HistogramMeasurements +OpenTelemetry.Metrics.HistogramMeasurements.Enumerator +OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.Current.get -> OpenTelemetry.Metrics.HistogramMeasurement +OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.Enumerator() -> void +OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.MoveNext() -> bool +OpenTelemetry.Metrics.HistogramMeasurements.GetEnumerator() -> OpenTelemetry.Metrics.HistogramMeasurements.Enumerator +OpenTelemetry.Metrics.MetricPoint.HistogramMeasurements.get -> OpenTelemetry.Metrics.HistogramMeasurements OpenTelemetry.Metrics.MetricPointsAccessor OpenTelemetry.Metrics.MetricPointsAccessor.MetricPointsAccessor() -> void OpenTelemetry.Metrics.MetricPointsAccessor.Dispose() -> void @@ -52,8 +63,6 @@ OpenTelemetry.Metrics.Metric.Unit.get -> string OpenTelemetry.Metrics.MetricPoint OpenTelemetry.Metrics.MetricPoint.DoubleValue.get -> double OpenTelemetry.Metrics.MetricPoint.EndTime.get -> System.DateTimeOffset -OpenTelemetry.Metrics.MetricPoint.GetBucketCounts() -> long[] -OpenTelemetry.Metrics.MetricPoint.GetExplicitBounds() -> double[] OpenTelemetry.Metrics.MetricPoint.GetHistogramCount() -> long OpenTelemetry.Metrics.MetricPoint.GetHistogramSum() -> double OpenTelemetry.Metrics.MetricPoint.LongValue.get -> long diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt index 9bfa7df92f2..408f111f89d 100644 --- a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt @@ -13,6 +13,17 @@ OpenTelemetry.Metrics.AggregationTemporalityAttribute.Supported.get -> OpenTelem OpenTelemetry.Metrics.BaseExportingMetricReader OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter exporter) -> void OpenTelemetry.Metrics.BaseExportingMetricReader.SupportedExportModes.get -> OpenTelemetry.Metrics.ExportModes +OpenTelemetry.Metrics.HistogramMeasurement +OpenTelemetry.Metrics.HistogramMeasurement.BucketCount.get -> long +OpenTelemetry.Metrics.HistogramMeasurement.ExplicitBound.get -> double +OpenTelemetry.Metrics.HistogramMeasurement.HistogramMeasurement() -> void +OpenTelemetry.Metrics.HistogramMeasurements +OpenTelemetry.Metrics.HistogramMeasurements.Enumerator +OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.Current.get -> OpenTelemetry.Metrics.HistogramMeasurement +OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.Enumerator() -> void +OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.MoveNext() -> bool +OpenTelemetry.Metrics.HistogramMeasurements.GetEnumerator() -> OpenTelemetry.Metrics.HistogramMeasurements.Enumerator +OpenTelemetry.Metrics.MetricPoint.HistogramMeasurements.get -> OpenTelemetry.Metrics.HistogramMeasurements OpenTelemetry.Metrics.MetricPointsAccessor OpenTelemetry.Metrics.MetricPointsAccessor.MetricPointsAccessor() -> void OpenTelemetry.Metrics.MetricPointsAccessor.Dispose() -> void @@ -52,8 +63,6 @@ OpenTelemetry.Metrics.Metric.Unit.get -> string OpenTelemetry.Metrics.MetricPoint OpenTelemetry.Metrics.MetricPoint.DoubleValue.get -> double OpenTelemetry.Metrics.MetricPoint.EndTime.get -> System.DateTimeOffset -OpenTelemetry.Metrics.MetricPoint.GetBucketCounts() -> long[] -OpenTelemetry.Metrics.MetricPoint.GetExplicitBounds() -> double[] OpenTelemetry.Metrics.MetricPoint.GetHistogramCount() -> long OpenTelemetry.Metrics.MetricPoint.GetHistogramSum() -> double OpenTelemetry.Metrics.MetricPoint.LongValue.get -> long diff --git a/src/OpenTelemetry/Metrics/HistogramMeasurement.cs b/src/OpenTelemetry/Metrics/HistogramMeasurement.cs new file mode 100644 index 00000000000..66997b0dc31 --- /dev/null +++ b/src/OpenTelemetry/Metrics/HistogramMeasurement.cs @@ -0,0 +1,31 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +namespace OpenTelemetry.Metrics +{ + public readonly struct HistogramMeasurement + { + internal HistogramMeasurement(double explicitBound, long bucketCount) + { + this.ExplicitBound = explicitBound; + this.BucketCount = bucketCount; + } + + public double ExplicitBound { get; } + + public long BucketCount { get; } + } +} diff --git a/src/OpenTelemetry/Metrics/HistogramMeasurements.cs b/src/OpenTelemetry/Metrics/HistogramMeasurements.cs index a59b325a5ca..b17675dcdc3 100644 --- a/src/OpenTelemetry/Metrics/HistogramMeasurements.cs +++ b/src/OpenTelemetry/Metrics/HistogramMeasurements.cs @@ -16,7 +16,7 @@ namespace OpenTelemetry.Metrics { - internal class HistogramMeasurements + public class HistogramMeasurements { internal readonly long[] BucketCounts; @@ -41,5 +41,44 @@ internal HistogramMeasurements(double[] histogramBounds) this.AggregatedBucketCounts = histogramBounds != null ? new long[histogramBounds.Length + 1] : null; this.LockObject = new object(); } + + public Enumerator GetEnumerator() => new(this); + + public struct Enumerator + { + private readonly bool isHistogramSumCount; + private readonly int numberOfBuckets; + private readonly int numberofExplicitBounds; + private readonly HistogramMeasurements histogramMeasurements; + private int index; + + internal Enumerator(HistogramMeasurements histogramMeasurements) + { + this.histogramMeasurements = histogramMeasurements; + this.index = 0; + this.Current = default; + this.isHistogramSumCount = histogramMeasurements.ExplicitBounds == null; + this.numberOfBuckets = this.isHistogramSumCount ? default : histogramMeasurements.BucketCounts.Length; + this.numberofExplicitBounds = this.isHistogramSumCount ? default : histogramMeasurements.ExplicitBounds.Length; + } + + public HistogramMeasurement Current { get; private set; } + + public bool MoveNext() + { + if (!this.isHistogramSumCount && this.index < this.numberOfBuckets) + { + double explicitBound = this.index < this.numberofExplicitBounds + ? this.histogramMeasurements.ExplicitBounds[this.index] + : double.PositiveInfinity; + long bucketCount = this.histogramMeasurements.AggregatedBucketCounts[this.index]; + this.Current = new HistogramMeasurement(explicitBound, bucketCount); + this.index++; + return true; + } + + return false; + } + } } } diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index e90c8b77972..3848bc4ceda 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -23,7 +23,6 @@ namespace OpenTelemetry.Metrics public struct MetricPoint { private readonly AggregationType aggType; - private readonly HistogramMeasurements histogramMeasurements; private long longVal; private long lastLongSum; private double doubleVal; @@ -52,15 +51,15 @@ internal MetricPoint( if (this.aggType == AggregationType.Histogram) { - this.histogramMeasurements = new HistogramMeasurements(histogramBounds); + this.HistogramMeasurements = new HistogramMeasurements(histogramBounds); } else if (this.aggType == AggregationType.HistogramSumCount) { - this.histogramMeasurements = new HistogramMeasurements(null); + this.HistogramMeasurements = new HistogramMeasurements(null); } else { - this.histogramMeasurements = null; + this.HistogramMeasurements = null; } } @@ -77,37 +76,15 @@ internal MetricPoint( public double DoubleValue { get; internal set; } - internal MetricPointStatus MetricPointStatus { get; private set; } - - public long[] GetBucketCounts() - { - if (this.aggType == AggregationType.Histogram) - { - return this.histogramMeasurements.AggregatedBucketCounts; - } - else - { - throw new NotSupportedException($"{nameof(this.GetBucketCounts)} is not supported for this metric type."); - } - } + public readonly HistogramMeasurements HistogramMeasurements { get; } - public double[] GetExplicitBounds() - { - if (this.aggType == AggregationType.Histogram) - { - return this.histogramMeasurements.ExplicitBounds; - } - else - { - throw new NotSupportedException($"{nameof(this.GetExplicitBounds)} is not supported for this metric type."); - } - } + internal MetricPointStatus MetricPointStatus { get; private set; } public long GetHistogramCount() { if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) { - return this.histogramMeasurements.Count; + return this.HistogramMeasurements.Count; } else { @@ -119,7 +96,7 @@ public double GetHistogramSum() { if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) { - return this.histogramMeasurements.Sum; + return this.HistogramMeasurements.Sum; } else { @@ -202,20 +179,20 @@ internal void Update(double number) case AggregationType.Histogram: { int i; - for (i = 0; i < this.histogramMeasurements.ExplicitBounds.Length; i++) + for (i = 0; i < this.HistogramMeasurements.ExplicitBounds.Length; i++) { // Upper bound is inclusive - if (number <= this.histogramMeasurements.ExplicitBounds[i]) + if (number <= this.HistogramMeasurements.ExplicitBounds[i]) { break; } } - lock (this.histogramMeasurements.LockObject) + lock (this.HistogramMeasurements.LockObject) { - this.histogramMeasurements.CountVal++; - this.histogramMeasurements.SumVal += number; - this.histogramMeasurements.BucketCounts[i]++; + this.HistogramMeasurements.CountVal++; + this.HistogramMeasurements.SumVal += number; + this.HistogramMeasurements.BucketCounts[i]++; } break; @@ -223,10 +200,10 @@ internal void Update(double number) case AggregationType.HistogramSumCount: { - lock (this.histogramMeasurements.LockObject) + lock (this.HistogramMeasurements.LockObject) { - this.histogramMeasurements.CountVal++; - this.histogramMeasurements.SumVal += number; + this.HistogramMeasurements.CountVal++; + this.HistogramMeasurements.SumVal += number; } break; @@ -348,22 +325,22 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.Histogram: { - lock (this.histogramMeasurements.LockObject) + lock (this.HistogramMeasurements.LockObject) { - this.histogramMeasurements.Count = this.histogramMeasurements.CountVal; - this.histogramMeasurements.Sum = this.histogramMeasurements.SumVal; + this.HistogramMeasurements.Count = this.HistogramMeasurements.CountVal; + this.HistogramMeasurements.Sum = this.HistogramMeasurements.SumVal; if (outputDelta) { - this.histogramMeasurements.CountVal = 0; - this.histogramMeasurements.SumVal = 0; + this.HistogramMeasurements.CountVal = 0; + this.HistogramMeasurements.SumVal = 0; } - for (int i = 0; i < this.histogramMeasurements.BucketCounts.Length; i++) + for (int i = 0; i < this.HistogramMeasurements.BucketCounts.Length; i++) { - this.histogramMeasurements.AggregatedBucketCounts[i] = this.histogramMeasurements.BucketCounts[i]; + this.HistogramMeasurements.AggregatedBucketCounts[i] = this.HistogramMeasurements.BucketCounts[i]; if (outputDelta) { - this.histogramMeasurements.BucketCounts[i] = 0; + this.HistogramMeasurements.BucketCounts[i] = 0; } } @@ -375,14 +352,14 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.HistogramSumCount: { - lock (this.histogramMeasurements.LockObject) + lock (this.HistogramMeasurements.LockObject) { - this.histogramMeasurements.Count = this.histogramMeasurements.CountVal; - this.histogramMeasurements.Sum = this.histogramMeasurements.SumVal; + this.HistogramMeasurements.Count = this.HistogramMeasurements.CountVal; + this.HistogramMeasurements.Sum = this.HistogramMeasurements.SumVal; if (outputDelta) { - this.histogramMeasurements.CountVal = 0; - this.histogramMeasurements.SumVal = 0; + this.HistogramMeasurements.CountVal = 0; + this.HistogramMeasurements.SumVal = 0; } this.MetricPointStatus = MetricPointStatus.NoCollectPending; diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs index fc6f3aed88a..47544776f0e 100644 --- a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs +++ b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs @@ -50,12 +50,14 @@ public void HistogramDistributeToAllBucketsDefault() histogramPoint.TakeSnapshot(true); var count = histogramPoint.GetHistogramCount(); - var bucketCounts = histogramPoint.GetBucketCounts(); Assert.Equal(22, count); - for (int i = 0; i < bucketCounts.Length; i++) + + int actualCount = 0; + foreach (var histogramMeasurement in histogramPoint.HistogramMeasurements) { - Assert.Equal(2, bucketCounts[i]); + Assert.Equal(2, histogramMeasurement.BucketCount); + actualCount++; } } @@ -80,17 +82,24 @@ public void HistogramDistributeToAllBucketsCustom() var count = histogramPoint.GetHistogramCount(); var sum = histogramPoint.GetHistogramSum(); - var bucketCounts = histogramPoint.GetBucketCounts(); // Sum of all recordings Assert.Equal(40, sum); // Count = # of recordings Assert.Equal(7, count); - Assert.Equal(boundaries.Length + 1, bucketCounts.Length); - Assert.Equal(5, bucketCounts[0]); - Assert.Equal(2, bucketCounts[1]); - Assert.Equal(0, bucketCounts[2]); + + int index = 0; + int actualCount = 0; + var expectedBucketCounts = new long[] { 5, 2, 0 }; + foreach (var histogramMeasurement in histogramPoint.HistogramMeasurements) + { + Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); + index++; + actualCount++; + } + + Assert.Equal(boundaries.Length + 1, actualCount); } [Fact] @@ -118,8 +127,9 @@ public void HistogramWithOnlySumCount() // Count = # of recordings Assert.Equal(7, count); - Assert.Throws(() => histogramPoint.GetBucketCounts()); - Assert.Throws(() => histogramPoint.GetExplicitBounds()); + // There should be no enumeration of BucketCounts and ExplicitBounds for HistogramSumCount + var enumerator = histogramPoint.HistogramMeasurements.GetEnumerator(); + Assert.False(enumerator.MoveNext()); } } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs index 1daf90a7b52..e54c5b77339 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs @@ -390,18 +390,21 @@ public void ViewToProduceCustomHistogramBound() var count = histogramPoint.GetHistogramCount(); var sum = histogramPoint.GetHistogramSum(); - var bucketCounts = histogramPoint.GetBucketCounts(); - var explicitBounds = histogramPoint.GetExplicitBounds(); Assert.Equal(40, sum); Assert.Equal(7, count); - Assert.Equal(Metric.DefaultHistogramBounds.Length + 1, bucketCounts.Length); - Assert.Equal(2, bucketCounts[0]); - Assert.Equal(1, bucketCounts[1]); - Assert.Equal(2, bucketCounts[2]); - Assert.Equal(2, bucketCounts[3]); - Assert.Equal(0, bucketCounts[4]); - Assert.Equal(0, bucketCounts[5]); + + int index = 0; + int actualCount = 0; + var expectedBucketCounts = new long[] { 2, 1, 2, 2, 0, 0, 0, 0, 0, 0, 0 }; + foreach (var histogramMeasurement in histogramPoint.HistogramMeasurements) + { + Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); + index++; + actualCount++; + } + + Assert.Equal(Metric.DefaultHistogramBounds.Length + 1, actualCount); List metricPointsCustom = new List(); foreach (ref var mp in metricCustom.GetMetricPoints()) @@ -414,15 +417,21 @@ public void ViewToProduceCustomHistogramBound() count = histogramPoint.GetHistogramCount(); sum = histogramPoint.GetHistogramSum(); - bucketCounts = histogramPoint.GetBucketCounts(); - explicitBounds = histogramPoint.GetExplicitBounds(); Assert.Equal(40, sum); Assert.Equal(7, count); - Assert.Equal(boundaries.Length + 1, bucketCounts.Length); - Assert.Equal(5, bucketCounts[0]); - Assert.Equal(2, bucketCounts[1]); - Assert.Equal(0, bucketCounts[2]); + + index = 0; + actualCount = 0; + expectedBucketCounts = new long[] { 5, 2, 0 }; + foreach (var histogramMeasurement in histogramPoint.HistogramMeasurements) + { + Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); + index++; + actualCount++; + } + + Assert.Equal(boundaries.Length + 1, actualCount); } [Fact] From 4b8d421e6ea6ba8bfe3c82b755866a437869b326 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Tue, 23 Nov 2021 00:50:30 -0800 Subject: [PATCH 2/4] Update CHANGELOG.md --- src/OpenTelemetry/CHANGELOG.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 77e87c2a07e..419c5b1bc5d 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -11,8 +11,13 @@ ([#2657](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2657)) * Remove MetricStreamConfiguration.Aggregation, as the feature to customize -aggregation is not implemented yet. -([#2660](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2660)) + aggregation is not implemented yet. + ([#2660](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2660)) + +* Refactored `HistogramMeasurements` to provide an enumerator for enumerating + the BucketCounts and ExplicitBounds. Removed `GetBucketCounts` and + `GetExplicitBounds` methods from `MetricPoint`. + ([#2664](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2664)) ## 1.2.0-beta2 From 9ba6bb72bd659c52212368d7acb0a71717f431fe Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Tue, 23 Nov 2021 11:32:25 -0800 Subject: [PATCH 3/4] Address PR comments --- .../ConsoleMetricExporter.cs | 2 +- .../Implementation/MetricItemExtensions.cs | 2 +- .../Implementation/PrometheusSerializerExt.cs | 2 +- .../.publicApi/net461/PublicAPI.Unshipped.txt | 22 ++++---- .../netstandard2.0/PublicAPI.Unshipped.txt | 22 ++++---- src/OpenTelemetry/CHANGELOG.md | 6 +- ...ogramMeasurement.cs => HistogramBucket.cs} | 6 +- ...ramMeasurements.cs => HistogramBuckets.cs} | 22 ++++---- src/OpenTelemetry/Metrics/MetricPoint.cs | 56 +++++++++---------- .../Metrics/AggregatorTest.cs | 6 +- .../Metrics/MetricViewTests.cs | 4 +- 11 files changed, 74 insertions(+), 76 deletions(-) rename src/OpenTelemetry/Metrics/{HistogramMeasurement.cs => HistogramBucket.cs} (80%) rename src/OpenTelemetry/Metrics/{HistogramMeasurements.cs => HistogramBuckets.cs} (70%) diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs index 1d4c7477b5a..63a76846e55 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs @@ -101,7 +101,7 @@ public override ExportResult Export(in Batch batch) bool isFirstIteration = true; double previousExplicitBound = default; - foreach (var histogramMeasurement in metricPoint.HistogramMeasurements) + foreach (var histogramMeasurement in metricPoint.HistogramBuckets) { if (isFirstIteration) { diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs index 9227d1b0d81..28c66974db8 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs @@ -249,7 +249,7 @@ internal static OtlpMetrics.Metric ToOtlpMetric(this Metric metric) dataPoint.Count = (ulong)metricPoint.LongValue; dataPoint.Sum = metricPoint.DoubleValue; - foreach (var histogramMeasurement in metricPoint.HistogramMeasurements) + foreach (var histogramMeasurement in metricPoint.HistogramBuckets) { dataPoint.BucketCounts.Add((ulong)histogramMeasurement.BucketCount); if (histogramMeasurement.ExplicitBound != double.PositiveInfinity) diff --git a/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs b/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs index 5ce8e64f517..c7e07742791 100644 --- a/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs +++ b/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs @@ -92,7 +92,7 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) var timestamp = metricPoint.EndTime.ToUnixTimeMilliseconds(); long totalCount = 0; - foreach (var histogramMeasurement in metricPoint.HistogramMeasurements) + foreach (var histogramMeasurement in metricPoint.HistogramBuckets) { totalCount += histogramMeasurement.BucketCount; diff --git a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt index 75fc1e29c33..6971a01db58 100644 --- a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt @@ -13,17 +13,17 @@ OpenTelemetry.Metrics.AggregationTemporalityAttribute.Supported.get -> OpenTelem OpenTelemetry.Metrics.BaseExportingMetricReader OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter exporter) -> void OpenTelemetry.Metrics.BaseExportingMetricReader.SupportedExportModes.get -> OpenTelemetry.Metrics.ExportModes -OpenTelemetry.Metrics.HistogramMeasurement -OpenTelemetry.Metrics.HistogramMeasurement.BucketCount.get -> long -OpenTelemetry.Metrics.HistogramMeasurement.ExplicitBound.get -> double -OpenTelemetry.Metrics.HistogramMeasurement.HistogramMeasurement() -> void -OpenTelemetry.Metrics.HistogramMeasurements -OpenTelemetry.Metrics.HistogramMeasurements.Enumerator -OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.Current.get -> OpenTelemetry.Metrics.HistogramMeasurement -OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.Enumerator() -> void -OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.MoveNext() -> bool -OpenTelemetry.Metrics.HistogramMeasurements.GetEnumerator() -> OpenTelemetry.Metrics.HistogramMeasurements.Enumerator -OpenTelemetry.Metrics.MetricPoint.HistogramMeasurements.get -> OpenTelemetry.Metrics.HistogramMeasurements +OpenTelemetry.Metrics.HistogramBucket +OpenTelemetry.Metrics.HistogramBucket.BucketCount.get -> long +OpenTelemetry.Metrics.HistogramBucket.ExplicitBound.get -> double +OpenTelemetry.Metrics.HistogramBucket.HistogramBucket() -> void +OpenTelemetry.Metrics.HistogramBuckets +OpenTelemetry.Metrics.HistogramBuckets.Enumerator +OpenTelemetry.Metrics.HistogramBuckets.Enumerator.Current.get -> OpenTelemetry.Metrics.HistogramBucket +OpenTelemetry.Metrics.HistogramBuckets.Enumerator.Enumerator() -> void +OpenTelemetry.Metrics.HistogramBuckets.Enumerator.MoveNext() -> bool +OpenTelemetry.Metrics.HistogramBuckets.GetEnumerator() -> OpenTelemetry.Metrics.HistogramBuckets.Enumerator +OpenTelemetry.Metrics.MetricPoint.HistogramBuckets.get -> OpenTelemetry.Metrics.HistogramBuckets OpenTelemetry.Metrics.MetricPointsAccessor OpenTelemetry.Metrics.MetricPointsAccessor.MetricPointsAccessor() -> void OpenTelemetry.Metrics.MetricPointsAccessor.Dispose() -> void diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt index 75fc1e29c33..6971a01db58 100644 --- a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt @@ -13,17 +13,17 @@ OpenTelemetry.Metrics.AggregationTemporalityAttribute.Supported.get -> OpenTelem OpenTelemetry.Metrics.BaseExportingMetricReader OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter exporter) -> void OpenTelemetry.Metrics.BaseExportingMetricReader.SupportedExportModes.get -> OpenTelemetry.Metrics.ExportModes -OpenTelemetry.Metrics.HistogramMeasurement -OpenTelemetry.Metrics.HistogramMeasurement.BucketCount.get -> long -OpenTelemetry.Metrics.HistogramMeasurement.ExplicitBound.get -> double -OpenTelemetry.Metrics.HistogramMeasurement.HistogramMeasurement() -> void -OpenTelemetry.Metrics.HistogramMeasurements -OpenTelemetry.Metrics.HistogramMeasurements.Enumerator -OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.Current.get -> OpenTelemetry.Metrics.HistogramMeasurement -OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.Enumerator() -> void -OpenTelemetry.Metrics.HistogramMeasurements.Enumerator.MoveNext() -> bool -OpenTelemetry.Metrics.HistogramMeasurements.GetEnumerator() -> OpenTelemetry.Metrics.HistogramMeasurements.Enumerator -OpenTelemetry.Metrics.MetricPoint.HistogramMeasurements.get -> OpenTelemetry.Metrics.HistogramMeasurements +OpenTelemetry.Metrics.HistogramBucket +OpenTelemetry.Metrics.HistogramBucket.BucketCount.get -> long +OpenTelemetry.Metrics.HistogramBucket.ExplicitBound.get -> double +OpenTelemetry.Metrics.HistogramBucket.HistogramBucket() -> void +OpenTelemetry.Metrics.HistogramBuckets +OpenTelemetry.Metrics.HistogramBuckets.Enumerator +OpenTelemetry.Metrics.HistogramBuckets.Enumerator.Current.get -> OpenTelemetry.Metrics.HistogramBucket +OpenTelemetry.Metrics.HistogramBuckets.Enumerator.Enumerator() -> void +OpenTelemetry.Metrics.HistogramBuckets.Enumerator.MoveNext() -> bool +OpenTelemetry.Metrics.HistogramBuckets.GetEnumerator() -> OpenTelemetry.Metrics.HistogramBuckets.Enumerator +OpenTelemetry.Metrics.MetricPoint.HistogramBuckets.get -> OpenTelemetry.Metrics.HistogramBuckets OpenTelemetry.Metrics.MetricPointsAccessor OpenTelemetry.Metrics.MetricPointsAccessor.MetricPointsAccessor() -> void OpenTelemetry.Metrics.MetricPointsAccessor.Dispose() -> void diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 419c5b1bc5d..6771c21cf9a 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -14,9 +14,9 @@ aggregation is not implemented yet. ([#2660](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2660)) -* Refactored `HistogramMeasurements` to provide an enumerator for enumerating - the BucketCounts and ExplicitBounds. Removed `GetBucketCounts` and - `GetExplicitBounds` methods from `MetricPoint`. +* Renamed `HistogramMeasurements` to `HistogramBuckets` and added an enumerator + of type `HistogramBucket` for enumerating `BucketCounts` and `ExplicitBounds`. + Removed `GetBucketCounts` and `GetExplicitBounds` methods from `MetricPoint`. ([#2664](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2664)) ## 1.2.0-beta2 diff --git a/src/OpenTelemetry/Metrics/HistogramMeasurement.cs b/src/OpenTelemetry/Metrics/HistogramBucket.cs similarity index 80% rename from src/OpenTelemetry/Metrics/HistogramMeasurement.cs rename to src/OpenTelemetry/Metrics/HistogramBucket.cs index 66997b0dc31..646d3a2964b 100644 --- a/src/OpenTelemetry/Metrics/HistogramMeasurement.cs +++ b/src/OpenTelemetry/Metrics/HistogramBucket.cs @@ -1,4 +1,4 @@ -// +// // Copyright The OpenTelemetry Authors // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -16,9 +16,9 @@ namespace OpenTelemetry.Metrics { - public readonly struct HistogramMeasurement + public readonly struct HistogramBucket { - internal HistogramMeasurement(double explicitBound, long bucketCount) + internal HistogramBucket(double explicitBound, long bucketCount) { this.ExplicitBound = explicitBound; this.BucketCount = bucketCount; diff --git a/src/OpenTelemetry/Metrics/HistogramMeasurements.cs b/src/OpenTelemetry/Metrics/HistogramBuckets.cs similarity index 70% rename from src/OpenTelemetry/Metrics/HistogramMeasurements.cs rename to src/OpenTelemetry/Metrics/HistogramBuckets.cs index b17675dcdc3..2ef2e43f07c 100644 --- a/src/OpenTelemetry/Metrics/HistogramMeasurements.cs +++ b/src/OpenTelemetry/Metrics/HistogramBuckets.cs @@ -1,4 +1,4 @@ -// +// // Copyright The OpenTelemetry Authors // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -16,7 +16,7 @@ namespace OpenTelemetry.Metrics { - public class HistogramMeasurements + public class HistogramBuckets { internal readonly long[] BucketCounts; @@ -34,7 +34,7 @@ public class HistogramMeasurements internal double Sum; - internal HistogramMeasurements(double[] histogramBounds) + internal HistogramBuckets(double[] histogramBounds) { this.ExplicitBounds = histogramBounds; this.BucketCounts = histogramBounds != null ? new long[histogramBounds.Length + 1] : null; @@ -46,33 +46,31 @@ internal HistogramMeasurements(double[] histogramBounds) public struct Enumerator { - private readonly bool isHistogramSumCount; private readonly int numberOfBuckets; private readonly int numberofExplicitBounds; - private readonly HistogramMeasurements histogramMeasurements; + private readonly HistogramBuckets histogramMeasurements; private int index; - internal Enumerator(HistogramMeasurements histogramMeasurements) + internal Enumerator(HistogramBuckets histogramMeasurements) { this.histogramMeasurements = histogramMeasurements; this.index = 0; this.Current = default; - this.isHistogramSumCount = histogramMeasurements.ExplicitBounds == null; - this.numberOfBuckets = this.isHistogramSumCount ? default : histogramMeasurements.BucketCounts.Length; - this.numberofExplicitBounds = this.isHistogramSumCount ? default : histogramMeasurements.ExplicitBounds.Length; + this.numberOfBuckets = histogramMeasurements.ExplicitBounds == null ? 0 : histogramMeasurements.BucketCounts.Length; + this.numberofExplicitBounds = histogramMeasurements.ExplicitBounds == null ? 0 : histogramMeasurements.ExplicitBounds.Length; } - public HistogramMeasurement Current { get; private set; } + public HistogramBucket Current { get; private set; } public bool MoveNext() { - if (!this.isHistogramSumCount && this.index < this.numberOfBuckets) + if (this.index < this.numberOfBuckets) { double explicitBound = this.index < this.numberofExplicitBounds ? this.histogramMeasurements.ExplicitBounds[this.index] : double.PositiveInfinity; long bucketCount = this.histogramMeasurements.AggregatedBucketCounts[this.index]; - this.Current = new HistogramMeasurement(explicitBound, bucketCount); + this.Current = new HistogramBucket(explicitBound, bucketCount); this.index++; return true; } diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index 3848bc4ceda..4dc0a5ab9dc 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -51,15 +51,15 @@ internal MetricPoint( if (this.aggType == AggregationType.Histogram) { - this.HistogramMeasurements = new HistogramMeasurements(histogramBounds); + this.HistogramBuckets = new HistogramBuckets(histogramBounds); } else if (this.aggType == AggregationType.HistogramSumCount) { - this.HistogramMeasurements = new HistogramMeasurements(null); + this.HistogramBuckets = new HistogramBuckets(null); } else { - this.HistogramMeasurements = null; + this.HistogramBuckets = null; } } @@ -76,7 +76,7 @@ internal MetricPoint( public double DoubleValue { get; internal set; } - public readonly HistogramMeasurements HistogramMeasurements { get; } + public readonly HistogramBuckets HistogramBuckets { get; } internal MetricPointStatus MetricPointStatus { get; private set; } @@ -84,7 +84,7 @@ public long GetHistogramCount() { if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) { - return this.HistogramMeasurements.Count; + return this.HistogramBuckets.Count; } else { @@ -96,7 +96,7 @@ public double GetHistogramSum() { if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) { - return this.HistogramMeasurements.Sum; + return this.HistogramBuckets.Sum; } else { @@ -179,20 +179,20 @@ internal void Update(double number) case AggregationType.Histogram: { int i; - for (i = 0; i < this.HistogramMeasurements.ExplicitBounds.Length; i++) + for (i = 0; i < this.HistogramBuckets.ExplicitBounds.Length; i++) { // Upper bound is inclusive - if (number <= this.HistogramMeasurements.ExplicitBounds[i]) + if (number <= this.HistogramBuckets.ExplicitBounds[i]) { break; } } - lock (this.HistogramMeasurements.LockObject) + lock (this.HistogramBuckets.LockObject) { - this.HistogramMeasurements.CountVal++; - this.HistogramMeasurements.SumVal += number; - this.HistogramMeasurements.BucketCounts[i]++; + this.HistogramBuckets.CountVal++; + this.HistogramBuckets.SumVal += number; + this.HistogramBuckets.BucketCounts[i]++; } break; @@ -200,10 +200,10 @@ internal void Update(double number) case AggregationType.HistogramSumCount: { - lock (this.HistogramMeasurements.LockObject) + lock (this.HistogramBuckets.LockObject) { - this.HistogramMeasurements.CountVal++; - this.HistogramMeasurements.SumVal += number; + this.HistogramBuckets.CountVal++; + this.HistogramBuckets.SumVal += number; } break; @@ -325,22 +325,22 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.Histogram: { - lock (this.HistogramMeasurements.LockObject) + lock (this.HistogramBuckets.LockObject) { - this.HistogramMeasurements.Count = this.HistogramMeasurements.CountVal; - this.HistogramMeasurements.Sum = this.HistogramMeasurements.SumVal; + this.HistogramBuckets.Count = this.HistogramBuckets.CountVal; + this.HistogramBuckets.Sum = this.HistogramBuckets.SumVal; if (outputDelta) { - this.HistogramMeasurements.CountVal = 0; - this.HistogramMeasurements.SumVal = 0; + this.HistogramBuckets.CountVal = 0; + this.HistogramBuckets.SumVal = 0; } - for (int i = 0; i < this.HistogramMeasurements.BucketCounts.Length; i++) + for (int i = 0; i < this.HistogramBuckets.BucketCounts.Length; i++) { - this.HistogramMeasurements.AggregatedBucketCounts[i] = this.HistogramMeasurements.BucketCounts[i]; + this.HistogramBuckets.AggregatedBucketCounts[i] = this.HistogramBuckets.BucketCounts[i]; if (outputDelta) { - this.HistogramMeasurements.BucketCounts[i] = 0; + this.HistogramBuckets.BucketCounts[i] = 0; } } @@ -352,14 +352,14 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.HistogramSumCount: { - lock (this.HistogramMeasurements.LockObject) + lock (this.HistogramBuckets.LockObject) { - this.HistogramMeasurements.Count = this.HistogramMeasurements.CountVal; - this.HistogramMeasurements.Sum = this.HistogramMeasurements.SumVal; + this.HistogramBuckets.Count = this.HistogramBuckets.CountVal; + this.HistogramBuckets.Sum = this.HistogramBuckets.SumVal; if (outputDelta) { - this.HistogramMeasurements.CountVal = 0; - this.HistogramMeasurements.SumVal = 0; + this.HistogramBuckets.CountVal = 0; + this.HistogramBuckets.SumVal = 0; } this.MetricPointStatus = MetricPointStatus.NoCollectPending; diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs index 47544776f0e..a08904be899 100644 --- a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs +++ b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs @@ -54,7 +54,7 @@ public void HistogramDistributeToAllBucketsDefault() Assert.Equal(22, count); int actualCount = 0; - foreach (var histogramMeasurement in histogramPoint.HistogramMeasurements) + foreach (var histogramMeasurement in histogramPoint.HistogramBuckets) { Assert.Equal(2, histogramMeasurement.BucketCount); actualCount++; @@ -92,7 +92,7 @@ public void HistogramDistributeToAllBucketsCustom() int index = 0; int actualCount = 0; var expectedBucketCounts = new long[] { 5, 2, 0 }; - foreach (var histogramMeasurement in histogramPoint.HistogramMeasurements) + foreach (var histogramMeasurement in histogramPoint.HistogramBuckets) { Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); index++; @@ -128,7 +128,7 @@ public void HistogramWithOnlySumCount() Assert.Equal(7, count); // There should be no enumeration of BucketCounts and ExplicitBounds for HistogramSumCount - var enumerator = histogramPoint.HistogramMeasurements.GetEnumerator(); + var enumerator = histogramPoint.HistogramBuckets.GetEnumerator(); Assert.False(enumerator.MoveNext()); } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs index e54c5b77339..152b242c4e5 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs @@ -397,7 +397,7 @@ public void ViewToProduceCustomHistogramBound() int index = 0; int actualCount = 0; var expectedBucketCounts = new long[] { 2, 1, 2, 2, 0, 0, 0, 0, 0, 0, 0 }; - foreach (var histogramMeasurement in histogramPoint.HistogramMeasurements) + foreach (var histogramMeasurement in histogramPoint.HistogramBuckets) { Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); index++; @@ -424,7 +424,7 @@ public void ViewToProduceCustomHistogramBound() index = 0; actualCount = 0; expectedBucketCounts = new long[] { 5, 2, 0 }; - foreach (var histogramMeasurement in histogramPoint.HistogramMeasurements) + foreach (var histogramMeasurement in histogramPoint.HistogramBuckets) { Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); index++; From e41e84e9b5cfa42236fb1f67f5b128679076159c Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Tue, 23 Nov 2021 11:59:00 -0800 Subject: [PATCH 4/4] Add public method GetHistogramBuckets --- .../ConsoleMetricExporter.cs | 2 +- .../Implementation/MetricItemExtensions.cs | 2 +- .../Implementation/PrometheusSerializerExt.cs | 2 +- .../.publicApi/net461/PublicAPI.Unshipped.txt | 2 +- .../netstandard2.0/PublicAPI.Unshipped.txt | 2 +- src/OpenTelemetry/CHANGELOG.md | 8 ++- src/OpenTelemetry/Metrics/MetricPoint.cs | 69 +++++++++++-------- .../Metrics/AggregatorTest.cs | 6 +- .../Metrics/MetricViewTests.cs | 4 +- 9 files changed, 55 insertions(+), 42 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs index 63a76846e55..064c1dad14a 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs @@ -101,7 +101,7 @@ public override ExportResult Export(in Batch batch) bool isFirstIteration = true; double previousExplicitBound = default; - foreach (var histogramMeasurement in metricPoint.HistogramBuckets) + foreach (var histogramMeasurement in metricPoint.GetHistogramBuckets()) { if (isFirstIteration) { diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs index 28c66974db8..e85cdcda231 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs @@ -249,7 +249,7 @@ internal static OtlpMetrics.Metric ToOtlpMetric(this Metric metric) dataPoint.Count = (ulong)metricPoint.LongValue; dataPoint.Sum = metricPoint.DoubleValue; - foreach (var histogramMeasurement in metricPoint.HistogramBuckets) + foreach (var histogramMeasurement in metricPoint.GetHistogramBuckets()) { dataPoint.BucketCounts.Add((ulong)histogramMeasurement.BucketCount); if (histogramMeasurement.ExplicitBound != double.PositiveInfinity) diff --git a/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs b/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs index c7e07742791..ea554caa82b 100644 --- a/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs +++ b/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs @@ -92,7 +92,7 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) var timestamp = metricPoint.EndTime.ToUnixTimeMilliseconds(); long totalCount = 0; - foreach (var histogramMeasurement in metricPoint.HistogramBuckets) + foreach (var histogramMeasurement in metricPoint.GetHistogramBuckets()) { totalCount += histogramMeasurement.BucketCount; diff --git a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt index 6971a01db58..abde8e7515a 100644 --- a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt @@ -23,7 +23,7 @@ OpenTelemetry.Metrics.HistogramBuckets.Enumerator.Current.get -> OpenTelemetry.M OpenTelemetry.Metrics.HistogramBuckets.Enumerator.Enumerator() -> void OpenTelemetry.Metrics.HistogramBuckets.Enumerator.MoveNext() -> bool OpenTelemetry.Metrics.HistogramBuckets.GetEnumerator() -> OpenTelemetry.Metrics.HistogramBuckets.Enumerator -OpenTelemetry.Metrics.MetricPoint.HistogramBuckets.get -> OpenTelemetry.Metrics.HistogramBuckets +OpenTelemetry.Metrics.MetricPoint.GetHistogramBuckets() -> OpenTelemetry.Metrics.HistogramBuckets OpenTelemetry.Metrics.MetricPointsAccessor OpenTelemetry.Metrics.MetricPointsAccessor.MetricPointsAccessor() -> void OpenTelemetry.Metrics.MetricPointsAccessor.Dispose() -> void diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt index 6971a01db58..abde8e7515a 100644 --- a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt @@ -23,7 +23,7 @@ OpenTelemetry.Metrics.HistogramBuckets.Enumerator.Current.get -> OpenTelemetry.M OpenTelemetry.Metrics.HistogramBuckets.Enumerator.Enumerator() -> void OpenTelemetry.Metrics.HistogramBuckets.Enumerator.MoveNext() -> bool OpenTelemetry.Metrics.HistogramBuckets.GetEnumerator() -> OpenTelemetry.Metrics.HistogramBuckets.Enumerator -OpenTelemetry.Metrics.MetricPoint.HistogramBuckets.get -> OpenTelemetry.Metrics.HistogramBuckets +OpenTelemetry.Metrics.MetricPoint.GetHistogramBuckets() -> OpenTelemetry.Metrics.HistogramBuckets OpenTelemetry.Metrics.MetricPointsAccessor OpenTelemetry.Metrics.MetricPointsAccessor.MetricPointsAccessor() -> void OpenTelemetry.Metrics.MetricPointsAccessor.Dispose() -> void diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 6771c21cf9a..e9cace8ad15 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -14,9 +14,11 @@ aggregation is not implemented yet. ([#2660](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2660)) -* Renamed `HistogramMeasurements` to `HistogramBuckets` and added an enumerator - of type `HistogramBucket` for enumerating `BucketCounts` and `ExplicitBounds`. - Removed `GetBucketCounts` and `GetExplicitBounds` methods from `MetricPoint`. +* Removed the public property `HistogramMeasurements` and added a public method + `GetHistogramBuckets` instead. Renamed the class `HistogramMeasurements` to + `HistogramBuckets` and added an enumerator of type `HistogramBucket` for + enumerating `BucketCounts` and `ExplicitBounds`. Removed `GetBucketCounts` and + `GetExplicitBounds` methods from `MetricPoint`. ([#2664](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2664)) ## 1.2.0-beta2 diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index 4dc0a5ab9dc..2e696a3bb8e 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -23,6 +23,7 @@ namespace OpenTelemetry.Metrics public struct MetricPoint { private readonly AggregationType aggType; + private readonly HistogramBuckets histogramBuckets; private long longVal; private long lastLongSum; private double doubleVal; @@ -51,15 +52,15 @@ internal MetricPoint( if (this.aggType == AggregationType.Histogram) { - this.HistogramBuckets = new HistogramBuckets(histogramBounds); + this.histogramBuckets = new HistogramBuckets(histogramBounds); } else if (this.aggType == AggregationType.HistogramSumCount) { - this.HistogramBuckets = new HistogramBuckets(null); + this.histogramBuckets = new HistogramBuckets(null); } else { - this.HistogramBuckets = null; + this.histogramBuckets = null; } } @@ -76,15 +77,13 @@ internal MetricPoint( public double DoubleValue { get; internal set; } - public readonly HistogramBuckets HistogramBuckets { get; } - internal MetricPointStatus MetricPointStatus { get; private set; } public long GetHistogramCount() { if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) { - return this.HistogramBuckets.Count; + return this.histogramBuckets.Count; } else { @@ -96,7 +95,7 @@ public double GetHistogramSum() { if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) { - return this.HistogramBuckets.Sum; + return this.histogramBuckets.Sum; } else { @@ -104,6 +103,18 @@ public double GetHistogramSum() } } + public HistogramBuckets GetHistogramBuckets() + { + if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) + { + return this.histogramBuckets; + } + else + { + throw new NotSupportedException($"{nameof(this.GetHistogramBuckets)} is not supported for this metric type."); + } + } + internal void Update(long number) { switch (this.aggType) @@ -179,20 +190,20 @@ internal void Update(double number) case AggregationType.Histogram: { int i; - for (i = 0; i < this.HistogramBuckets.ExplicitBounds.Length; i++) + for (i = 0; i < this.histogramBuckets.ExplicitBounds.Length; i++) { // Upper bound is inclusive - if (number <= this.HistogramBuckets.ExplicitBounds[i]) + if (number <= this.histogramBuckets.ExplicitBounds[i]) { break; } } - lock (this.HistogramBuckets.LockObject) + lock (this.histogramBuckets.LockObject) { - this.HistogramBuckets.CountVal++; - this.HistogramBuckets.SumVal += number; - this.HistogramBuckets.BucketCounts[i]++; + this.histogramBuckets.CountVal++; + this.histogramBuckets.SumVal += number; + this.histogramBuckets.BucketCounts[i]++; } break; @@ -200,10 +211,10 @@ internal void Update(double number) case AggregationType.HistogramSumCount: { - lock (this.HistogramBuckets.LockObject) + lock (this.histogramBuckets.LockObject) { - this.HistogramBuckets.CountVal++; - this.HistogramBuckets.SumVal += number; + this.histogramBuckets.CountVal++; + this.histogramBuckets.SumVal += number; } break; @@ -325,22 +336,22 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.Histogram: { - lock (this.HistogramBuckets.LockObject) + lock (this.histogramBuckets.LockObject) { - this.HistogramBuckets.Count = this.HistogramBuckets.CountVal; - this.HistogramBuckets.Sum = this.HistogramBuckets.SumVal; + this.histogramBuckets.Count = this.histogramBuckets.CountVal; + this.histogramBuckets.Sum = this.histogramBuckets.SumVal; if (outputDelta) { - this.HistogramBuckets.CountVal = 0; - this.HistogramBuckets.SumVal = 0; + this.histogramBuckets.CountVal = 0; + this.histogramBuckets.SumVal = 0; } - for (int i = 0; i < this.HistogramBuckets.BucketCounts.Length; i++) + for (int i = 0; i < this.histogramBuckets.BucketCounts.Length; i++) { - this.HistogramBuckets.AggregatedBucketCounts[i] = this.HistogramBuckets.BucketCounts[i]; + this.histogramBuckets.AggregatedBucketCounts[i] = this.histogramBuckets.BucketCounts[i]; if (outputDelta) { - this.HistogramBuckets.BucketCounts[i] = 0; + this.histogramBuckets.BucketCounts[i] = 0; } } @@ -352,14 +363,14 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.HistogramSumCount: { - lock (this.HistogramBuckets.LockObject) + lock (this.histogramBuckets.LockObject) { - this.HistogramBuckets.Count = this.HistogramBuckets.CountVal; - this.HistogramBuckets.Sum = this.HistogramBuckets.SumVal; + this.histogramBuckets.Count = this.histogramBuckets.CountVal; + this.histogramBuckets.Sum = this.histogramBuckets.SumVal; if (outputDelta) { - this.HistogramBuckets.CountVal = 0; - this.HistogramBuckets.SumVal = 0; + this.histogramBuckets.CountVal = 0; + this.histogramBuckets.SumVal = 0; } this.MetricPointStatus = MetricPointStatus.NoCollectPending; diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs index a08904be899..8a276f5d618 100644 --- a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs +++ b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs @@ -54,7 +54,7 @@ public void HistogramDistributeToAllBucketsDefault() Assert.Equal(22, count); int actualCount = 0; - foreach (var histogramMeasurement in histogramPoint.HistogramBuckets) + foreach (var histogramMeasurement in histogramPoint.GetHistogramBuckets()) { Assert.Equal(2, histogramMeasurement.BucketCount); actualCount++; @@ -92,7 +92,7 @@ public void HistogramDistributeToAllBucketsCustom() int index = 0; int actualCount = 0; var expectedBucketCounts = new long[] { 5, 2, 0 }; - foreach (var histogramMeasurement in histogramPoint.HistogramBuckets) + foreach (var histogramMeasurement in histogramPoint.GetHistogramBuckets()) { Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); index++; @@ -128,7 +128,7 @@ public void HistogramWithOnlySumCount() Assert.Equal(7, count); // There should be no enumeration of BucketCounts and ExplicitBounds for HistogramSumCount - var enumerator = histogramPoint.HistogramBuckets.GetEnumerator(); + var enumerator = histogramPoint.GetHistogramBuckets().GetEnumerator(); Assert.False(enumerator.MoveNext()); } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs index 152b242c4e5..d3e356072bc 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs @@ -397,7 +397,7 @@ public void ViewToProduceCustomHistogramBound() int index = 0; int actualCount = 0; var expectedBucketCounts = new long[] { 2, 1, 2, 2, 0, 0, 0, 0, 0, 0, 0 }; - foreach (var histogramMeasurement in histogramPoint.HistogramBuckets) + foreach (var histogramMeasurement in histogramPoint.GetHistogramBuckets()) { Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); index++; @@ -424,7 +424,7 @@ public void ViewToProduceCustomHistogramBound() index = 0; actualCount = 0; expectedBucketCounts = new long[] { 5, 2, 0 }; - foreach (var histogramMeasurement in histogramPoint.HistogramBuckets) + foreach (var histogramMeasurement in histogramPoint.GetHistogramBuckets()) { Assert.Equal(expectedBucketCounts[index], histogramMeasurement.BucketCount); index++;