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.

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

Binary search for large bucket count histograms #3252

Merged
merged 51 commits into from
Sep 2, 2022
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
a69121c
Binary search for large bucket count histograms
mic-max May 4, 2022
6e446cd
Update CHANGELOG.md
mic-max May 4, 2022
e420a15
netcoreapp3.1 was complaining
mic-max May 4, 2022
e8913b2
ci rerun
mic-max May 4, 2022
4ade35a
ci rerun
mic-max May 4, 2022
52e2c43
Update MetricTestData.cs
mic-max May 4, 2022
f7e88af
use 400 buckets
mic-max May 12, 2022
51d7219
Update ExplicitBucketHistogramConfiguration.cs
mic-max May 4, 2022
1bb61e9
separate nan part
mic-max May 12, 2022
685b46e
Address PR comments
mic-max May 12, 2022
773fefd
Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…
mic-max May 12, 2022
de60b0e
update to 140
mic-max May 12, 2022
020c42e
remove double.nan from invalid hist bounds
mic-max May 12, 2022
daf1a80
Refactor and perf to histogram bucket index find
mic-max May 25, 2022
5bc4998
fine tune bound limit to switch to binary search
mic-max May 26, 2022
e84127a
included benchmark results in comment
mic-max May 26, 2022
0e939b6
new bench result
mic-max May 26, 2022
fb1ad6b
histogram stress test update
mic-max Jun 9, 2022
99bd6f1
Merge remote-tracking branch 'upstream/main' into hist-binary
mic-max Jun 28, 2022
05cc3c1
fix changelog
mic-max Jun 28, 2022
15c3464
spacing fix
mic-max Jun 28, 2022
a2c0e5d
sealed bucket class
mic-max Jul 1, 2022
ef28da0
ci
mic-max Jul 1, 2022
1810ddd
Merge branch 'main' into hist-binary
mic-max Jul 1, 2022
543a6d5
double.negative infinity
mic-max Jul 5, 2022
19ddae3
Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…
mic-max Jul 5, 2022
f2a20f7
update order of operations
mic-max Jul 5, 2022
c9f3a3d
remove stress test change
mic-max Jul 5, 2022
a201b86
ci
mic-max Jul 5, 2022
d0d92a7
Add histogram binary mode tests
mic-max Jul 6, 2022
7f8d32b
ci
mic-max Jul 6, 2022
53ad017
Merge branch 'main' into hist-binary
utpilla Jul 6, 2022
b41879c
Merge branch 'main' into hist-binary
mic-max Jul 8, 2022
dbc5f3e
pr review changes
mic-max Jul 11, 2022
3046ac7
Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…
mic-max Jul 11, 2022
881caca
Merge branch 'main' into hist-binary
mic-max Jul 11, 2022
370fb1a
allocated column - hist benchmark
mic-max Jul 12, 2022
07689ac
Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…
mic-max Jul 12, 2022
4fe19f9
Merge branch 'main' into hist-binary
mic-max Jul 12, 2022
4d97b4e
Merge branch 'main' into hist-binary
utpilla Jul 15, 2022
8307bf8
Merge branch 'main' into hist-binary
mic-max Jul 26, 2022
3b2f762
CI
mic-max Jul 26, 2022
2030736
Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…
mic-max Jul 26, 2022
42b1def
Merge branch 'main' into hist-binary
utpilla Jul 27, 2022
adb6945
Merge branch 'main' into hist-binary
utpilla Jul 28, 2022
90a95e6
Merge branch 'hist-binary' of https://github.com/mic-max/opentelemetr…
mic-max Aug 2, 2022
21de441
Merge branch 'main' into hist-binary
mic-max Aug 3, 2022
7806dbc
Merge branch 'main' into hist-binary
mic-max Aug 9, 2022
e7212ac
Merge branch 'main' into hist-binary
mic-max Aug 25, 2022
9a3025c
Merge branch 'main' into hist-binary
mic-max Aug 31, 2022
2bb1212
Merge branch 'main' into hist-binary
cijothomas Sep 2, 2022
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/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Use binary search for histograms with 50 or more supplied boundaries.
([#3252](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3252))

* `TracerProviderSDK` modified for spans with remote parent. For such spans
activity will be created irrespective of SamplingResult, to maintain context
propagation.
Expand Down Expand Up @@ -55,7 +58,7 @@ Released 2022-May-16

* Exposed public setters for `LogRecord.State`, `LogRecord.StateValues`,
and `LogRecord.FormattedMessage`.
([#3217](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3217))
([#3217](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3217))

## 1.3.0-beta.1

Expand Down
95 changes: 95 additions & 0 deletions src/OpenTelemetry/Metrics/HistogramBuckets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
// </copyright>

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;

namespace OpenTelemetry.Metrics
{
Expand All @@ -24,6 +26,8 @@ namespace OpenTelemetry.Metrics
// Note: Does not implement IEnumerable<> to prevent accidental boxing.
public class HistogramBuckets
{
internal const int DefaultBoundaryCountForBinarySearch = 50;

internal readonly double[] ExplicitBounds;

internal readonly long[] RunningBucketCounts;
Expand All @@ -36,9 +40,38 @@ public class HistogramBuckets

internal int IsCriticalSectionOccupied = 0;

private readonly BucketLookupNode bucketLookupTreeRoot;

private readonly Func<double, int> findHistogramBucketIndex;

internal HistogramBuckets(double[] explicitBounds)
{
this.ExplicitBounds = explicitBounds;
this.findHistogramBucketIndex = this.FindBucketIndexLinear;
mic-max marked this conversation as resolved.
Show resolved Hide resolved
if (explicitBounds != null && explicitBounds.Length >= DefaultBoundaryCountForBinarySearch)
{
this.bucketLookupTreeRoot = ConstructBalancedBST(explicitBounds, 0, explicitBounds.Length);
this.findHistogramBucketIndex = this.FindBucketIndexBinary;

static BucketLookupNode ConstructBalancedBST(double[] values, int min, int max)
mic-max marked this conversation as resolved.
Show resolved Hide resolved
{
if (min == max)
{
return null;
}

int median = min + ((max - min) / 2);
return new BucketLookupNode
{
Index = median,
UpperBoundInclusive = values[median],
LowerBoundExclusive = median > 0 ? values[median - 1] : double.NegativeInfinity,
Left = ConstructBalancedBST(values, min, median),
Right = ConstructBalancedBST(values, median + 1, max),
};
}
}

this.RunningBucketCounts = explicitBounds != null ? new long[explicitBounds.Length + 1] : null;
this.SnapshotBucketCounts = explicitBounds != null ? new long[explicitBounds.Length + 1] : new long[0];
}
Expand All @@ -57,6 +90,55 @@ internal HistogramBuckets Copy()
return copy;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal int FindBucketIndex(double value)
{
return this.findHistogramBucketIndex(value);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal int FindBucketIndexBinary(double value)
{
BucketLookupNode current = this.bucketLookupTreeRoot;

Debug.Assert(current != null, "Bucket root was null.");

do
{
if (value <= current.LowerBoundExclusive)
{
current = current.Left;
}
else if (value > current.UpperBoundInclusive)
{
current = current.Right;
}
else
{
return current.Index;
}
}
while (current != null);

return this.ExplicitBounds.Length;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal int FindBucketIndexLinear(double value)
{
int i;
for (i = 0; i < this.ExplicitBounds.Length; i++)
{
// Upper bound is inclusive
if (value <= this.ExplicitBounds[i])
{
break;
}
}

return i;
}

/// <summary>
/// Enumerates the elements of a <see cref="HistogramBuckets"/>.
/// </summary>
Expand Down Expand Up @@ -104,5 +186,18 @@ public bool MoveNext()
return false;
}
}

private sealed class BucketLookupNode
{
public double UpperBoundInclusive { get; set; }

public double LowerBoundExclusive { get; set; }
utpilla marked this conversation as resolved.
Show resolved Hide resolved

public int Index { get; set; }

public BucketLookupNode Left { get; set; }

public BucketLookupNode Right { get; set; }
}
}
}
10 changes: 1 addition & 9 deletions src/OpenTelemetry/Metrics/MetricPoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -324,15 +324,7 @@ internal void Update(double number)

case AggregationType.Histogram:
{
int i;
for (i = 0; i < this.histogramBuckets.ExplicitBounds.Length; i++)
{
// Upper bound is inclusive
if (number <= this.histogramBuckets.ExplicitBounds[i])
{
break;
}
}
int i = this.histogramBuckets.FindBucketIndex(number);

var sw = default(SpinWait);
while (true)
Expand Down
61 changes: 31 additions & 30 deletions test/Benchmarks/Metrics/HistogramBenchmarks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,49 +25,50 @@

/*
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK=6.0.200
[Host] : .NET 6.0.2 (6.0.222.6406), X64 RyuJIT
DefaultJob : .NET 6.0.2 (6.0.222.6406), X64 RyuJIT


| Method | BoundCount | Mean | Error | StdDev | Allocated |
|---------------------------- |----------- |----------:|---------:|---------:|----------:|
| HistogramHotPath | 10 | 45.27 ns | 0.384 ns | 0.359 ns | - |
| HistogramWith1LabelHotPath | 10 | 89.99 ns | 0.373 ns | 0.312 ns | - |
| HistogramWith3LabelsHotPath | 10 | 185.34 ns | 3.184 ns | 3.667 ns | - |
| HistogramWith5LabelsHotPath | 10 | 266.69 ns | 1.391 ns | 1.301 ns | - |
| HistogramWith7LabelsHotPath | 10 | 323.20 ns | 1.834 ns | 1.531 ns | - |
| HistogramHotPath | 20 | 48.69 ns | 0.347 ns | 0.307 ns | - |
| HistogramWith1LabelHotPath | 20 | 93.84 ns | 0.696 ns | 0.651 ns | - |
| HistogramWith3LabelsHotPath | 20 | 189.82 ns | 1.208 ns | 1.071 ns | - |
| HistogramWith5LabelsHotPath | 20 | 269.23 ns | 2.027 ns | 1.693 ns | - |
| HistogramWith7LabelsHotPath | 20 | 329.92 ns | 1.272 ns | 1.128 ns | - |
| HistogramHotPath | 50 | 55.73 ns | 0.339 ns | 0.317 ns | - |
| HistogramWith1LabelHotPath | 50 | 100.38 ns | 0.455 ns | 0.425 ns | - |
| HistogramWith3LabelsHotPath | 50 | 200.02 ns | 1.011 ns | 0.844 ns | - |
| HistogramWith5LabelsHotPath | 50 | 279.94 ns | 1.595 ns | 1.492 ns | - |
| HistogramWith7LabelsHotPath | 50 | 346.88 ns | 1.064 ns | 0.943 ns | - |
| HistogramHotPath | 100 | 66.39 ns | 0.167 ns | 0.148 ns | - |
| HistogramWith1LabelHotPath | 100 | 114.98 ns | 1.340 ns | 1.253 ns | - |
| HistogramWith3LabelsHotPath | 100 | 220.52 ns | 1.723 ns | 1.528 ns | - |
| HistogramWith5LabelsHotPath | 100 | 299.10 ns | 1.950 ns | 1.629 ns | - |
| HistogramWith7LabelsHotPath | 100 | 356.25 ns | 2.153 ns | 1.798 ns | - |
Intel Core i7-1065G7 CPU 1.30GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.203
[Host] : .NET 6.0.5 (6.0.522.21309), X64 RyuJIT
DefaultJob : .NET 6.0.5 (6.0.522.21309), X64 RyuJIT


| Method | BoundCount | Mean | Error | StdDev | Allocated |
|---------------------------- |----------- |----------:|----------:|----------:|----------:|
| HistogramHotPath | 10 | 55.07 ns | 0.664 ns | 1.091 ns | - |
| HistogramWith1LabelHotPath | 10 | 108.66 ns | 1.324 ns | 1.174 ns | - |
| HistogramWith3LabelsHotPath | 10 | 193.79 ns | 3.261 ns | 3.349 ns | - |
| HistogramWith5LabelsHotPath | 10 | 279.44 ns | 4.608 ns | 3.848 ns | - |
| HistogramWith7LabelsHotPath | 10 | 334.28 ns | 6.650 ns | 5.895 ns | - |
| HistogramHotPath | 49 | 68.27 ns | 0.744 ns | 0.581 ns | - |
| HistogramWith1LabelHotPath | 49 | 125.55 ns | 2.265 ns | 2.518 ns | - |
| HistogramWith3LabelsHotPath | 49 | 207.95 ns | 4.023 ns | 3.951 ns | - |
| HistogramWith5LabelsHotPath | 49 | 293.45 ns | 5.689 ns | 5.842 ns | - |
| HistogramWith7LabelsHotPath | 49 | 362.19 ns | 5.610 ns | 6.003 ns | - |
| HistogramHotPath | 50 | 69.64 ns | 1.422 ns | 1.330 ns | - |
| HistogramWith1LabelHotPath | 50 | 118.15 ns | 2.040 ns | 1.908 ns | - |
| HistogramWith3LabelsHotPath | 50 | 250.31 ns | 4.617 ns | 9.326 ns | - |
| HistogramWith5LabelsHotPath | 50 | 335.31 ns | 3.904 ns | 3.461 ns | - |
| HistogramWith7LabelsHotPath | 50 | 398.02 ns | 6.815 ns | 6.374 ns | - |
| HistogramHotPath | 1000 | 94.05 ns | 1.890 ns | 2.100 ns | - |
| HistogramWith1LabelHotPath | 1000 | 148.57 ns | 2.055 ns | 1.822 ns | - |
| HistogramWith3LabelsHotPath | 1000 | 661.78 ns | 11.599 ns | 20.314 ns | - |
| HistogramWith5LabelsHotPath | 1000 | 761.54 ns | 15.049 ns | 16.727 ns | - |
| HistogramWith7LabelsHotPath | 1000 | 830.14 ns | 16.063 ns | 17.853 ns | - |
*/

namespace Benchmarks.Metrics
{
public class HistogramBenchmarks
{
private const int MaxValue = 1000;
private const int MaxValue = 10000;
private readonly Random random = new();
private readonly string[] dimensionValues = new string[] { "DimVal1", "DimVal2", "DimVal3", "DimVal4", "DimVal5", "DimVal6", "DimVal7", "DimVal8", "DimVal9", "DimVal10" };
private Histogram<long> histogram;
private MeterProvider provider;
private Meter meter;
private double[] bounds;

[Params(10, 20, 50, 100)]
// Note: Values related to `HistogramBuckets.DefaultHistogramCountForBinarySearch`
[Params(10, 49, 50, 1000)]
public int BoundCount { get; set; }

[GlobalSetup]
Expand Down
40 changes: 40 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,46 @@ public void HistogramDistributeToAllBucketsCustom()
Assert.Equal(boundaries.Length + 1, actualCount);
}

[Fact]
public void HistogramBinaryBucketTest()
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to add e2e test for Histogram targeting the edges (default bounds, bound.length >50 etc.)

{
// Arrange
// Bounds = (-Inf, 0] (0, 1], ... (49, +Inf)
var boundaries = new double[HistogramBuckets.DefaultBoundaryCountForBinarySearch];
for (var i = 0; i < boundaries.Length; i++)
{
boundaries[i] = i;
}

var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.Histogram, null, null, boundaries);

// Act
histogramPoint.Update(-1);
mic-max marked this conversation as resolved.
Show resolved Hide resolved
histogramPoint.Update(boundaries[0]);
histogramPoint.Update(boundaries[boundaries.Length - 1]);
for (var i = 0.5; i < boundaries.Length; i++)
{
histogramPoint.Update(i);
}

histogramPoint.TakeSnapshot(true);

// Assert
var index = 0;
foreach (var histogramMeasurement in histogramPoint.GetHistogramBuckets())
{
var expectedCount = 1;

if (index == 0 || index == boundaries.Length - 1)
{
expectedCount = 2;
}

Assert.Equal(expectedCount, histogramMeasurement.BucketCount);
index++;
}
}

[Fact]
public void HistogramWithOnlySumCount()
{
Expand Down