Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MetricPoint HistogramMeasurements #2664

Merged
merged 5 commits into from
Nov 23, 2021
+279 −195
Merged

Update MetricPoint HistogramMeasurements #2664

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f82ce0
Update HistogramMeasurement
utpilla Nov 23, 2021
4b8d421
Update CHANGELOG.md
utpilla Nov 23, 2021
22b0d85
Merge branch 'main' into utpilla/Update-MetricPoint-HistogramMeasurem…
cijothomas Nov 23, 2021
9ba6bb7
Address PR comments
utpilla Nov 23, 2021
e41e84e
Add public method GetHistogramBuckets
utpilla Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 25 additions & 27 deletions src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,42 +99,40 @@ public override ExportResult Export(in Batch<Metric> 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();
Expand Down
13 changes: 4 additions & 9 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
61 changes: 26 additions & 35 deletions src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
13 changes: 11 additions & 2 deletions src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ OpenTelemetry.Metrics.AggregationTemporalityAttribute.Supported.get -> OpenTelem
OpenTelemetry.Metrics.BaseExportingMetricReader
OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter<OpenTelemetry.Metrics.Metric> 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
Expand Down Expand Up @@ -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
Expand Down
13 changes: 11 additions & 2 deletions src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ OpenTelemetry.Metrics.AggregationTemporalityAttribute.Supported.get -> OpenTelem
OpenTelemetry.Metrics.BaseExportingMetricReader
OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter<OpenTelemetry.Metrics.Metric> 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
Expand Down Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
31 changes: 31 additions & 0 deletions src/OpenTelemetry/Metrics/HistogramMeasurement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// <copyright file="HistogramMeasurement.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

namespace OpenTelemetry.Metrics
{
public readonly struct HistogramMeasurement
Copy link
Member

Choose a reason for hiding this comment

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

the term "measurement" is misleading as it refers to the raw data point reported through the API. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#measurement

{
internal HistogramMeasurement(double explicitBound, long bucketCount)
{
this.ExplicitBound = explicitBound;
this.BucketCount = bucketCount;
}

public double ExplicitBound { get; }

public long BucketCount { get; }
}
}
41 changes: 40 additions & 1 deletion src/OpenTelemetry/Metrics/HistogramMeasurements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

namespace OpenTelemetry.Metrics
{
internal class HistogramMeasurements
public class HistogramMeasurements
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class HistogramMeasurements
public class HistogramMetricPoint

Copy link
Member

@reyang reyang Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is this a HistogramMetricPoint or it is a HistogramMetricPointBucket?

If it is a HistogramMetricPoint, maybe it should inherit from MetricPoint?

Copy link
Member

Choose a reason for hiding this comment

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

HistogramMetricPointBucket is correct.

Copy link
Member

Choose a reason for hiding this comment

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

HistogramBuckets - this sounds better name.

{
internal readonly long[] BucketCounts;

Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

If you look at the original one I did, I had a Count on the parent class which I used in MoveNext. The reason for that is it is better for perf to keep structs as small as possible. What I don't know is if that size-reduction provides more perf than caching these values in the struct to reference in each MoveNext invocation. Might be worth measuring and possibly adjusting (could be a follow-up).

At the least, I think we can remove bool isHistogramSumCount because this.numberOfBuckets will be 0 in that case so MoveNext will know to exit. Basically...

            public bool MoveNext()
            {
                if (this.index < this.numberOfBuckets)

...should be enough (I think)?

Copy link
Contributor Author

@utpilla utpilla Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I have removed isHistogramSumCount. I will do the benchmarking for size-reduction vs caching later on.

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;
}
}
}
}
Loading