Skip to content

Fix exponential histogram bucket snapshot #4313

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

Merged
merged 4 commits into from
Mar 21, 2023
Merged

Fix exponential histogram bucket snapshot #4313

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2c6877
Fix exponential histogram bucket snapshot
alanwest Mar 17, 2023
12bef54
Simplify fix
alanwest Mar 17, 2023
e96326b
PR comments
alanwest Mar 18, 2023
2d76301
Merge branch 'main' into alanwest/fix-exp-histogram
alanwest Mar 21, 2023
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
5 changes: 4 additions & 1 deletion src/OpenTelemetry/Metrics/CircularBufferBuckets.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,10 @@ internal void Copy(long[] dst)

if (this.trait != null)
{
Array.Copy(this.trait, dst, this.Capacity);
for (var i = 0; i < this.Size; ++i)
{
dst[i] = this[this.Offset + i];
}
}
}

Expand Down
11 changes: 8 additions & 3 deletions src/OpenTelemetry/Metrics/ExponentialHistogramBuckets.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@ namespace OpenTelemetry.Metrics;
public sealed class ExponentialHistogramBuckets
{
private long[] buckets = Array.Empty<long>();
private int size = 0;

internal ExponentialHistogramBuckets()
{
}

public int Offset { get; private set; }

public Enumerator GetEnumerator() => new(this.buckets);
public Enumerator GetEnumerator() => new(this.buckets, this.size);

internal void SnapshotBuckets(CircularBufferBuckets buckets)
{
Expand All @@ -37,13 +38,15 @@ internal void SnapshotBuckets(CircularBufferBuckets buckets)
this.buckets = new long[buckets.Capacity];
}

this.size = buckets.Size;
this.Offset = buckets.Offset;
buckets.Copy(this.buckets);
}

internal ExponentialHistogramBuckets Copy()
{
var copy = new ExponentialHistogramBuckets();
copy.size = this.size;
copy.Offset = this.Offset;
copy.buckets = new long[this.buckets.Length];
Array.Copy(this.buckets, copy.buckets, this.buckets.Length);
Expand All @@ -58,10 +61,12 @@ public struct Enumerator
{
private readonly long[] buckets;
private int index;
private int size;

internal Enumerator(long[] buckets)
internal Enumerator(long[] buckets, int size)
{
this.index = 0;
this.size = size;
this.buckets = buckets;
this.Current = default;
}
Expand All @@ -81,7 +86,7 @@ internal Enumerator(long[] buckets)
/// collection.</returns>
public bool MoveNext()
Copy link
Contributor

@utpilla utpilla Mar 18, 2023

Choose a reason for hiding this comment

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

I think the unit tests should also cover a scale-down scenario when the CircularBufferBuckets offset is updated.

Copy link
Member Author

@alanwest alanwest Mar 18, 2023

Choose a reason for hiding this comment

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

It's not very obvious, but the existing tests actually do cover this scenario.

Even this simple test that only records two values requires a scale down. When the first value is recorded (10) the initial scale of 20 is fine, but when the second value is recorded (5) a scale down is necessary to fit this range of values within the default 160 buckets.

opentelemetry-dotnet/test/OpenTelemetry.Tests/Metrics/MetricSnapshotTests.cs

Lines 238 to 241 in d924663

// SECOND EXPORT
expectedHistogram.Record(5);
histogram.Record(5);
meterProvider.ForceFlush();

Copy link
Contributor

@utpilla utpilla Mar 20, 2023

Choose a reason for hiding this comment

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

Got it. Thanks!

Copy link
Member Author

@alanwest alanwest Mar 21, 2023

Choose a reason for hiding this comment

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

I'm going to merge this PR, but I will be coming back to the tests. Clearly there's improvements to make as the bug this PR fixes is not really covered by any tests yet.

{
if (this.index < this.buckets.Length)
if (this.index < this.size)
{
this.Current = this.buckets[this.index++];
return true;
Expand Down