From 5c325baa433f02d41e24688ec46321368f811ce9 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Mon, 22 Nov 2021 00:14:05 -0800 Subject: [PATCH 1/6] Update MetricPoint for Histograms --- .../ConsoleMetricExporter.cs | 26 ++-- .../Implementation/MetricItemExtensions.cs | 12 +- .../Implementation/PrometheusSerializerExt.cs | 13 +- .../.publicApi/net461/PublicAPI.Unshipped.txt | 6 +- .../netstandard2.0/PublicAPI.Unshipped.txt | 6 +- .../Metrics/HistogramMeasurements.cs | 42 +++++++ src/OpenTelemetry/Metrics/MetricPoint.cs | 116 ++++++++++++------ .../MetricTests.cs | 8 +- .../HttpClientTests.netcore31.cs | 8 +- .../Metrics/AggregatorTest.cs | 37 ++++-- .../Metrics/MetricViewTests.cs | 40 +++--- 11 files changed, 218 insertions(+), 96 deletions(-) create mode 100644 src/OpenTelemetry/Metrics/HistogramMeasurements.cs diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs index e1ec1e3ca2d..3465f413143 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs @@ -95,38 +95,42 @@ public override ExportResult Export(in Batch batch) if (metricType.IsHistogram()) { var bucketsBuilder = new StringBuilder(); - bucketsBuilder.Append($"Sum: {metricPoint.DoubleValue} Count: {metricPoint.LongValue} \n"); + var sum = metricPoint.GetHistogramSum(); + var count = metricPoint.GetHistogramCount(); + bucketsBuilder.Append($"Sum: {sum} Count: {count} \n"); - if (metricPoint.ExplicitBounds != null) + var explicitBounds = metricPoint.GetExplicitBounds(); + if (explicitBounds != null) { - for (int i = 0; i < metricPoint.ExplicitBounds.Length + 1; i++) + var bucketCounts = metricPoint.GetBucketCounts(); + for (int i = 0; i < explicitBounds.Length + 1; i++) { if (i == 0) { bucketsBuilder.Append("(-Infinity,"); - bucketsBuilder.Append(metricPoint.ExplicitBounds[i]); + bucketsBuilder.Append(explicitBounds[i]); bucketsBuilder.Append(']'); bucketsBuilder.Append(':'); - bucketsBuilder.Append(metricPoint.BucketCounts[i]); + bucketsBuilder.Append(bucketCounts[i]); } - else if (i == metricPoint.ExplicitBounds.Length) + else if (i == explicitBounds.Length) { bucketsBuilder.Append('('); - bucketsBuilder.Append(metricPoint.ExplicitBounds[i - 1]); + bucketsBuilder.Append(explicitBounds[i - 1]); bucketsBuilder.Append(','); bucketsBuilder.Append("+Infinity]"); bucketsBuilder.Append(':'); - bucketsBuilder.Append(metricPoint.BucketCounts[i]); + bucketsBuilder.Append(bucketCounts[i]); } else { bucketsBuilder.Append('('); - bucketsBuilder.Append(metricPoint.ExplicitBounds[i - 1]); + bucketsBuilder.Append(explicitBounds[i - 1]); bucketsBuilder.Append(','); - bucketsBuilder.Append(metricPoint.ExplicitBounds[i]); + bucketsBuilder.Append(explicitBounds[i]); bucketsBuilder.Append(']'); bucketsBuilder.Append(':'); - bucketsBuilder.Append(metricPoint.BucketCounts[i]); + bucketsBuilder.Append(bucketCounts[i]); } bucketsBuilder.AppendLine(); diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs index 942c01f20ec..1857c899ddb 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs @@ -249,14 +249,16 @@ internal static OtlpMetrics.Metric ToOtlpMetric(this Metric metric) dataPoint.Count = (ulong)metricPoint.LongValue; dataPoint.Sum = metricPoint.DoubleValue; - if (metricPoint.BucketCounts != null) + var bucketCounts = metricPoint.GetBucketCounts(); + if (bucketCounts != null) { - for (int i = 0; i < metricPoint.BucketCounts.Length; i++) + var explicitBounds = metricPoint.GetExplicitBounds(); + for (int i = 0; i < bucketCounts.Length; i++) { - dataPoint.BucketCounts.Add((ulong)metricPoint.BucketCounts[i]); - if (i < metricPoint.BucketCounts.Length - 1) + dataPoint.BucketCounts.Add((ulong)bucketCounts[i]); + if (i < bucketCounts.Length - 1) { - dataPoint.ExplicitBounds.Add(metricPoint.ExplicitBounds[i]); + dataPoint.ExplicitBounds.Add(explicitBounds[i]); } } } diff --git a/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs b/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs index 948b3826a7e..59b727a86eb 100644 --- a/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs +++ b/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs @@ -91,11 +91,11 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) var tags = metricPoint.Tags; var timestamp = metricPoint.EndTime.ToUnixTimeMilliseconds(); - if (metricPoint.BucketCounts != null) + var bucketCounts = metricPoint.GetBucketCounts(); + if (bucketCounts != null) { // Histogram buckets - var bucketCounts = metricPoint.BucketCounts; - var explicitBounds = metricPoint.ExplicitBounds; + var explicitBounds = metricPoint.GetExplicitBounds(); long totalCount = 0; for (int idxBound = 0; idxBound < explicitBounds.Length + 1; idxBound++) { @@ -133,6 +133,9 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) } // Histogram sum + var count = metricPoint.GetHistogramCount(); + var sum = metricPoint.GetHistogramSum(); + cursor = WriteMetricName(buffer, cursor, metric.Name, metric.Unit); cursor = WriteAsciiStringNoEscape(buffer, cursor, "_sum"); @@ -156,7 +159,7 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) buffer[cursor++] = unchecked((byte)' '); - cursor = WriteDouble(buffer, cursor, metricPoint.DoubleValue); + cursor = WriteDouble(buffer, cursor, sum); buffer[cursor++] = unchecked((byte)' '); cursor = WriteLong(buffer, cursor, timestamp); @@ -187,7 +190,7 @@ public static int WriteMetric(byte[] buffer, int cursor, Metric metric) buffer[cursor++] = unchecked((byte)' '); - cursor = WriteLong(buffer, cursor, metricPoint.LongValue); + cursor = WriteLong(buffer, cursor, count); 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 1d52a1586c6..55d8d3d762b 100644 --- a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt @@ -56,10 +56,12 @@ OpenTelemetry.Metrics.Metric.Name.get -> string OpenTelemetry.Metrics.Metric.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality OpenTelemetry.Metrics.Metric.Unit.get -> string OpenTelemetry.Metrics.MetricPoint -OpenTelemetry.Metrics.MetricPoint.BucketCounts.get -> long[] OpenTelemetry.Metrics.MetricPoint.DoubleValue.get -> double OpenTelemetry.Metrics.MetricPoint.EndTime.get -> System.DateTimeOffset -OpenTelemetry.Metrics.MetricPoint.ExplicitBounds.get -> double[] +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 OpenTelemetry.Metrics.MetricPoint.MetricPoint() -> void OpenTelemetry.Metrics.MetricPoint.StartTime.get -> System.DateTimeOffset diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt index 1d52a1586c6..55d8d3d762b 100644 --- a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt @@ -56,10 +56,12 @@ OpenTelemetry.Metrics.Metric.Name.get -> string OpenTelemetry.Metrics.Metric.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality OpenTelemetry.Metrics.Metric.Unit.get -> string OpenTelemetry.Metrics.MetricPoint -OpenTelemetry.Metrics.MetricPoint.BucketCounts.get -> long[] OpenTelemetry.Metrics.MetricPoint.DoubleValue.get -> double OpenTelemetry.Metrics.MetricPoint.EndTime.get -> System.DateTimeOffset -OpenTelemetry.Metrics.MetricPoint.ExplicitBounds.get -> double[] +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 OpenTelemetry.Metrics.MetricPoint.MetricPoint() -> void OpenTelemetry.Metrics.MetricPoint.StartTime.get -> System.DateTimeOffset diff --git a/src/OpenTelemetry/Metrics/HistogramMeasurements.cs b/src/OpenTelemetry/Metrics/HistogramMeasurements.cs new file mode 100644 index 00000000000..f362c1ba032 --- /dev/null +++ b/src/OpenTelemetry/Metrics/HistogramMeasurements.cs @@ -0,0 +1,42 @@ +// +// 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 +{ + internal class HistogramMeasurements + { + internal readonly long[] BucketCounts; + + internal readonly long[] AggregatedBucketCounts; + + internal readonly double[] ExplicitBounds; + + internal long CountVal; + + internal long Count; + + internal double SumVal; + + internal double Sum; + + internal HistogramMeasurements(double[] histogramBounds) + { + this.ExplicitBounds = histogramBounds; + this.BucketCounts = histogramBounds != null ? new long[histogramBounds.Length + 1] : null; + this.AggregatedBucketCounts = histogramBounds != null ? new long[histogramBounds.Length + 1] : null; + } + } +} diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index ee45e369a60..f707ac87495 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -23,8 +23,8 @@ namespace OpenTelemetry.Metrics public struct MetricPoint { private readonly AggregationType aggType; - private readonly object lockObject; - private readonly long[] bucketCounts; + private readonly HistogramMeasurements histogramMeasurements; + private readonly object lockObjectForHistogram; private long longVal; private long lastLongSum; private double doubleVal; @@ -53,24 +53,18 @@ internal MetricPoint( if (this.aggType == AggregationType.Histogram) { - this.ExplicitBounds = histogramBounds; - this.BucketCounts = new long[this.ExplicitBounds.Length + 1]; - this.bucketCounts = new long[this.ExplicitBounds.Length + 1]; - this.lockObject = new object(); + this.histogramMeasurements = new HistogramMeasurements(histogramBounds); + this.lockObjectForHistogram = new object(); } else if (this.aggType == AggregationType.HistogramSumCount) { - this.ExplicitBounds = null; - this.BucketCounts = null; - this.bucketCounts = null; - this.lockObject = new object(); + this.histogramMeasurements = new HistogramMeasurements(null); + this.lockObjectForHistogram = new object(); } else { - this.ExplicitBounds = null; - this.BucketCounts = null; - this.bucketCounts = null; - this.lockObject = null; + this.histogramMeasurements = null; + this.lockObjectForHistogram = null; } } @@ -87,11 +81,55 @@ internal MetricPoint( public double DoubleValue { get; internal set; } - public long[] BucketCounts { get; internal set; } + internal MetricPointStatus MetricPointStatus { get; private set; } - public double[] ExplicitBounds { get; internal set; } + public long[] GetBucketCounts() + { + if (this.aggType == AggregationType.Histogram) + { + return this.histogramMeasurements.AggregatedBucketCounts; + } + else + { + throw new InvalidOperationException($"{nameof(this.GetBucketCounts)} is not supported for this metric type."); + } + } - internal MetricPointStatus MetricPointStatus { get; private set; } + public double[] GetExplicitBounds() + { + if (this.aggType == AggregationType.Histogram) + { + return this.histogramMeasurements.ExplicitBounds; + } + else + { + throw new InvalidOperationException($"{nameof(this.GetExplicitBounds)} is not supported for this metric type."); + } + } + + public long GetHistogramCount() + { + if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) + { + return this.histogramMeasurements.Count; + } + else + { + throw new InvalidOperationException($"{nameof(this.GetHistogramCount)} is not supported for this metric type."); + } + } + + public double GetHistogramSum() + { + if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) + { + return this.histogramMeasurements.Sum; + } + else + { + throw new InvalidOperationException($"{nameof(this.GetHistogramSum)} is not supported for this metric type."); + } + } internal void Update(long number) { @@ -168,20 +206,20 @@ internal void Update(double number) case AggregationType.Histogram: { int i; - for (i = 0; i < this.ExplicitBounds.Length; i++) + for (i = 0; i < this.histogramMeasurements.ExplicitBounds.Length; i++) { // Upper bound is inclusive - if (number <= this.ExplicitBounds[i]) + if (number <= this.histogramMeasurements.ExplicitBounds[i]) { break; } } - lock (this.lockObject) + lock (this.lockObjectForHistogram) { - this.longVal++; - this.doubleVal += number; - this.bucketCounts[i]++; + this.histogramMeasurements.CountVal++; + this.histogramMeasurements.SumVal += number; + this.histogramMeasurements.BucketCounts[i]++; } break; @@ -189,10 +227,10 @@ internal void Update(double number) case AggregationType.HistogramSumCount: { - lock (this.lockObject) + lock (this.lockObjectForHistogram) { - this.longVal++; - this.doubleVal += number; + this.histogramMeasurements.CountVal++; + this.histogramMeasurements.SumVal += number; } break; @@ -314,22 +352,22 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.Histogram: { - lock (this.lockObject) + lock (this.lockObjectForHistogram) { - this.LongValue = this.longVal; - this.DoubleValue = this.doubleVal; + this.histogramMeasurements.Count = this.histogramMeasurements.CountVal; + this.histogramMeasurements.Sum = this.histogramMeasurements.SumVal; if (outputDelta) { - this.longVal = 0; - this.doubleVal = 0; + this.histogramMeasurements.CountVal = 0; + this.histogramMeasurements.SumVal = 0; } - for (int i = 0; i < this.bucketCounts.Length; i++) + for (int i = 0; i < this.histogramMeasurements.BucketCounts.Length; i++) { - this.BucketCounts[i] = this.bucketCounts[i]; + this.histogramMeasurements.AggregatedBucketCounts[i] = this.histogramMeasurements.BucketCounts[i]; if (outputDelta) { - this.bucketCounts[i] = 0; + this.histogramMeasurements.BucketCounts[i] = 0; } } @@ -341,14 +379,14 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.HistogramSumCount: { - lock (this.lockObject) + lock (this.lockObjectForHistogram) { - this.LongValue = this.longVal; - this.DoubleValue = this.doubleVal; + this.histogramMeasurements.Count = this.histogramMeasurements.CountVal; + this.histogramMeasurements.Sum = this.histogramMeasurements.SumVal; if (outputDelta) { - this.longVal = 0; - this.doubleVal = 0; + this.histogramMeasurements.CountVal = 0; + this.histogramMeasurements.SumVal = 0; } this.MetricPointStatus = MetricPointStatus.NoCollectPending; diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 9e22ff0ad00..5fa27849d0b 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -100,8 +100,12 @@ public async Task RequestMetricIsCaptured() Assert.Single(metricPoints); var metricPoint = metricPoints[0]; - Assert.Equal(1L, metricPoint.LongValue); - Assert.True(metricPoint.DoubleValue > 0); + + var count = metricPoint.GetHistogramCount(); + var sum = metricPoint.GetHistogramSum(); + + Assert.Equal(1L, count); + Assert.True(sum > 0); /* var bucket = metric.Buckets diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs index e668b953bc6..0931b314c5b 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs @@ -159,8 +159,12 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut Assert.Single(metricPoints); var metricPoint = metricPoints[0]; - Assert.Equal(1L, metricPoint.LongValue); - Assert.Equal(activity.Duration.TotalMilliseconds, metricPoint.DoubleValue); + + var count = metricPoint.GetHistogramCount(); + var sum = metricPoint.GetHistogramSum(); + + Assert.Equal(1L, count); + Assert.Equal(activity.Duration.TotalMilliseconds, sum); var attributes = new KeyValuePair[metricPoint.Tags.Count]; int i = 0; diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs index 870e60d3040..2d0e531bf68 100644 --- a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs +++ b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs @@ -49,10 +49,13 @@ public void HistogramDistributeToAllBucketsDefault() histogramPoint.Update(10000000); histogramPoint.TakeSnapshot(true); - Assert.Equal(22, histogramPoint.LongValue); - for (int i = 0; i < histogramPoint.BucketCounts.Length; i++) + var count = histogramPoint.GetHistogramCount(); + var bucketCounts = histogramPoint.GetBucketCounts(); + + Assert.Equal(22, count); + for (int i = 0; i < bucketCounts.Length; i++) { - Assert.Equal(2, histogramPoint.BucketCounts[i]); + Assert.Equal(2, bucketCounts[i]); } } @@ -75,15 +78,19 @@ public void HistogramDistributeToAllBucketsCustom() histogramPoint.TakeSnapshot(true); + var count = histogramPoint.GetHistogramCount(); + var sum = histogramPoint.GetHistogramSum(); + var bucketCounts = histogramPoint.GetBucketCounts(); + // Sum of all recordings - Assert.Equal(40, histogramPoint.DoubleValue); + Assert.Equal(40, sum); // Count = # of recordings - Assert.Equal(7, histogramPoint.LongValue); - Assert.Equal(boundaries.Length + 1, histogramPoint.BucketCounts.Length); - Assert.Equal(5, histogramPoint.BucketCounts[0]); - Assert.Equal(2, histogramPoint.BucketCounts[1]); - Assert.Equal(0, histogramPoint.BucketCounts[2]); + 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]); } [Fact] @@ -102,13 +109,17 @@ public void HistogramWithOnlySumCount() histogramPoint.TakeSnapshot(true); + var count = histogramPoint.GetHistogramCount(); + var sum = histogramPoint.GetHistogramSum(); + // Sum of all recordings - Assert.Equal(40, histogramPoint.DoubleValue); + Assert.Equal(40, sum); // Count = # of recordings - Assert.Equal(7, histogramPoint.LongValue); - Assert.Null(histogramPoint.BucketCounts); - Assert.Null(histogramPoint.ExplicitBounds); + Assert.Equal(7, count); + + Assert.Throws(() => histogramPoint.GetBucketCounts()); + Assert.Throws(() => histogramPoint.GetExplicitBounds()); } } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs index c9dfe79cc61..1daf90a7b52 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs @@ -388,15 +388,20 @@ public void ViewToProduceCustomHistogramBound() Assert.Single(metricPointsDefault); var histogramPoint = metricPointsDefault[0]; - Assert.Equal(40, histogramPoint.DoubleValue); - Assert.Equal(7, histogramPoint.LongValue); - Assert.Equal(Metric.DefaultHistogramBounds.Length + 1, histogramPoint.BucketCounts.Length); - Assert.Equal(2, histogramPoint.BucketCounts[0]); - Assert.Equal(1, histogramPoint.BucketCounts[1]); - Assert.Equal(2, histogramPoint.BucketCounts[2]); - Assert.Equal(2, histogramPoint.BucketCounts[3]); - Assert.Equal(0, histogramPoint.BucketCounts[4]); - Assert.Equal(0, histogramPoint.BucketCounts[5]); + 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]); List metricPointsCustom = new List(); foreach (ref var mp in metricCustom.GetMetricPoints()) @@ -407,12 +412,17 @@ public void ViewToProduceCustomHistogramBound() Assert.Single(metricPointsCustom); histogramPoint = metricPointsCustom[0]; - Assert.Equal(40, histogramPoint.DoubleValue); - Assert.Equal(7, histogramPoint.LongValue); - Assert.Equal(boundaries.Length + 1, histogramPoint.BucketCounts.Length); - Assert.Equal(5, histogramPoint.BucketCounts[0]); - Assert.Equal(2, histogramPoint.BucketCounts[1]); - Assert.Equal(0, histogramPoint.BucketCounts[2]); + 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]); } [Fact] From d72a8cf4809a85f0b90f20a75626db809cc33e32 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Mon, 22 Nov 2021 00:19:46 -0800 Subject: [PATCH 2/6] Update CHANGELOG.md --- src/OpenTelemetry/CHANGELOG.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 4f94318c16c..03846184556 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -6,12 +6,16 @@ `Keys`+`Values` ([#2642](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2642)) +* Added new public methods in `MetricPoint`: `GetBucketCounts`, + `GetExplicitBounds`, `GetHistogramCount`, and `GetHistogramSum` + ([#2657](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2657)) + ## 1.2.0-beta2 Released 2021-Nov-19 -* Renamed `HistogramConfiguration` to `ExplicitBucketHistogramConfiguration` - and changed its member `BucketBounds` to `Boundaries`. +* Renamed `HistogramConfiguration` to `ExplicitBucketHistogramConfiguration` and + changed its member `BucketBounds` to `Boundaries`. ([#2638](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2638)) * Metrics with the same name but from different meters are allowed. From 3118ef54f6c6de9aba7734fb36651f69fb04fc3e Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Mon, 22 Nov 2021 00:24:58 -0800 Subject: [PATCH 3/6] Update CHANGELOG.md --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 03846184556..ad1801b3ae4 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -6,7 +6,7 @@ `Keys`+`Values` ([#2642](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2642)) -* Added new public methods in `MetricPoint`: `GetBucketCounts`, +* Refactored `MetricPoint` and added public methods: `GetBucketCounts`, `GetExplicitBounds`, `GetHistogramCount`, and `GetHistogramSum` ([#2657](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2657)) From de125195f2308f67901d80d9157e76932120da71 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Mon, 22 Nov 2021 12:26:44 -0800 Subject: [PATCH 4/6] Move lockObject from MetricPoint to HistogramMeasurements --- src/OpenTelemetry/Metrics/HistogramMeasurements.cs | 3 +++ src/OpenTelemetry/Metrics/MetricPoint.cs | 12 ++++-------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/OpenTelemetry/Metrics/HistogramMeasurements.cs b/src/OpenTelemetry/Metrics/HistogramMeasurements.cs index f362c1ba032..a59b325a5ca 100644 --- a/src/OpenTelemetry/Metrics/HistogramMeasurements.cs +++ b/src/OpenTelemetry/Metrics/HistogramMeasurements.cs @@ -24,6 +24,8 @@ internal class HistogramMeasurements internal readonly double[] ExplicitBounds; + internal readonly object LockObject; + internal long CountVal; internal long Count; @@ -37,6 +39,7 @@ internal HistogramMeasurements(double[] histogramBounds) this.ExplicitBounds = histogramBounds; this.BucketCounts = histogramBounds != null ? new long[histogramBounds.Length + 1] : null; this.AggregatedBucketCounts = histogramBounds != null ? new long[histogramBounds.Length + 1] : null; + this.LockObject = new object(); } } } diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index f707ac87495..ab5a4f64717 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -24,7 +24,6 @@ public struct MetricPoint { private readonly AggregationType aggType; private readonly HistogramMeasurements histogramMeasurements; - private readonly object lockObjectForHistogram; private long longVal; private long lastLongSum; private double doubleVal; @@ -54,17 +53,14 @@ internal MetricPoint( if (this.aggType == AggregationType.Histogram) { this.histogramMeasurements = new HistogramMeasurements(histogramBounds); - this.lockObjectForHistogram = new object(); } else if (this.aggType == AggregationType.HistogramSumCount) { this.histogramMeasurements = new HistogramMeasurements(null); - this.lockObjectForHistogram = new object(); } else { this.histogramMeasurements = null; - this.lockObjectForHistogram = null; } } @@ -215,7 +211,7 @@ internal void Update(double number) } } - lock (this.lockObjectForHistogram) + lock (this.histogramMeasurements.LockObject) { this.histogramMeasurements.CountVal++; this.histogramMeasurements.SumVal += number; @@ -227,7 +223,7 @@ internal void Update(double number) case AggregationType.HistogramSumCount: { - lock (this.lockObjectForHistogram) + lock (this.histogramMeasurements.LockObject) { this.histogramMeasurements.CountVal++; this.histogramMeasurements.SumVal += number; @@ -352,7 +348,7 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.Histogram: { - lock (this.lockObjectForHistogram) + lock (this.histogramMeasurements.LockObject) { this.histogramMeasurements.Count = this.histogramMeasurements.CountVal; this.histogramMeasurements.Sum = this.histogramMeasurements.SumVal; @@ -379,7 +375,7 @@ internal void TakeSnapshot(bool outputDelta) case AggregationType.HistogramSumCount: { - lock (this.lockObjectForHistogram) + lock (this.histogramMeasurements.LockObject) { this.histogramMeasurements.Count = this.histogramMeasurements.CountVal; this.histogramMeasurements.Sum = this.histogramMeasurements.SumVal; From 5739710956ec8d50aa1054200160f3708d39181d Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Mon, 22 Nov 2021 12:29:15 -0800 Subject: [PATCH 5/6] Throw NotSupportedException --- src/OpenTelemetry/Metrics/MetricPoint.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index ab5a4f64717..e90c8b77972 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -87,7 +87,7 @@ public long[] GetBucketCounts() } else { - throw new InvalidOperationException($"{nameof(this.GetBucketCounts)} is not supported for this metric type."); + throw new NotSupportedException($"{nameof(this.GetBucketCounts)} is not supported for this metric type."); } } @@ -99,7 +99,7 @@ public double[] GetExplicitBounds() } else { - throw new InvalidOperationException($"{nameof(this.GetExplicitBounds)} is not supported for this metric type."); + throw new NotSupportedException($"{nameof(this.GetExplicitBounds)} is not supported for this metric type."); } } @@ -111,7 +111,7 @@ public long GetHistogramCount() } else { - throw new InvalidOperationException($"{nameof(this.GetHistogramCount)} is not supported for this metric type."); + throw new NotSupportedException($"{nameof(this.GetHistogramCount)} is not supported for this metric type."); } } @@ -123,7 +123,7 @@ public double GetHistogramSum() } else { - throw new InvalidOperationException($"{nameof(this.GetHistogramSum)} is not supported for this metric type."); + throw new NotSupportedException($"{nameof(this.GetHistogramSum)} is not supported for this metric type."); } } From 002ec79f6cc062fac92d5c4bd82387cbd996c9c8 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Mon, 22 Nov 2021 12:34:59 -0800 Subject: [PATCH 6/6] Update unit test --- test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs index 2d0e531bf68..fc6f3aed88a 100644 --- a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs +++ b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs @@ -118,8 +118,8 @@ public void HistogramWithOnlySumCount() // Count = # of recordings Assert.Equal(7, count); - Assert.Throws(() => histogramPoint.GetBucketCounts()); - Assert.Throws(() => histogramPoint.GetExplicitBounds()); + Assert.Throws(() => histogramPoint.GetBucketCounts()); + Assert.Throws(() => histogramPoint.GetExplicitBounds()); } } }