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

Add exponential histogram tests and in-memory exporter support #4303

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions src/OpenTelemetry/Metrics/Base2ExponentialBucketHistogram.cs
Original file line number Diff line number Diff line change
Expand Up @@ -235,4 +235,15 @@ internal ExponentialHistogramData GetExponentialHistogramData()
{
return this.SnapshotExponentialHistogramData;
}

internal Base2ExponentialBucketHistogram Copy()
{
Debug.Assert(this.PositiveBuckets.Capacity == this.NegativeBuckets.Capacity, "Capacity of positive and negative buckets are not equal.");
var copy = new Base2ExponentialBucketHistogram(this.PositiveBuckets.Capacity, this.SnapshotExponentialHistogramData.Scale);
copy.SnapshotSum = this.SnapshotSum;
copy.SnapshotMin = this.SnapshotMin;
copy.SnapshotMax = this.SnapshotMax;
copy.SnapshotExponentialHistogramData = this.SnapshotExponentialHistogramData.Copy();
return copy;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,31 @@ namespace OpenTelemetry.Metrics;
/// <summary>
/// Stores configuration for a histogram metric stream with base-2 exponential bucket boundaries.
/// </summary>
internal sealed class Base2ExponentialBucketHistogramConfiguration : MetricStreamConfiguration
internal sealed class Base2ExponentialBucketHistogramConfiguration : HistogramConfiguration
{
private int maxSize = 160;

/// <summary>
/// Gets or sets the maximum number of buckets in each of the positive and negative ranges, not counting the special zero bucket.
/// </summary>
/// <remarks>
/// The default value is 160.
/// The default value is 160. The minimum size is 2.
/// </remarks>
public int MaxSize { get; set; } = 160;
public int MaxSize
{
get
{
return this.maxSize;
}

set
{
if (value < 2)
{
throw new ArgumentException($"Histogram max size is invalid. Minimum size is 2.", nameof(value));
}

this.maxSize = value;
}
}
}
21 changes: 13 additions & 8 deletions src/OpenTelemetry/Metrics/ExponentialHistogramBuckets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ namespace OpenTelemetry.Metrics;

public sealed class ExponentialHistogramBuckets
{
private int size;
private long[] buckets = Array.Empty<long>();

internal ExponentialHistogramBuckets()
Expand All @@ -29,7 +28,7 @@ internal ExponentialHistogramBuckets()

public int Offset { get; private set; }

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

internal void SnapshotBuckets(CircularBufferBuckets buckets)
{
Expand All @@ -39,24 +38,30 @@ internal void SnapshotBuckets(CircularBufferBuckets buckets)
}

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

internal ExponentialHistogramBuckets Copy()
{
var copy = new ExponentialHistogramBuckets();
copy.Offset = this.Offset;
copy.buckets = new long[this.buckets.Length];
Array.Copy(this.buckets, copy.buckets, this.buckets.Length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy size?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out this was actually more insidious... I removed size altogether. Size was initialized wrong... this was never testing anything correctly.

This was a result of a refactor in my prior PR where I changed ExponentialHistogramBuckets from having a handle an instance of CircularBufferBuckets to simply a long[].

return copy;
}

/// <summary>
/// Enumerates the bucket counts of an exponential histogram.
/// </summary>
// Note: Does not implement IEnumerator<> to prevent accidental boxing.
public struct Enumerator
{
private readonly int size;
private readonly long[] buckets;
private int index;

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

@utpilla utpilla Mar 15, 2023

Choose a reason for hiding this comment

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

Shouldn't this be size? We are now always going enumerate over the full length of the CircularBufferBuckets instead of just enumerating from CircularBufferBuckets.begin to CircularBufferBuckets.end.

{
this.Current = this.buckets[this.index++];
return true;
Expand Down
14 changes: 12 additions & 2 deletions src/OpenTelemetry/Metrics/ExponentialHistogramData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,17 @@ internal ExponentialHistogramData()

public long ZeroCount { get; internal set; }

public ExponentialHistogramBuckets PositiveBuckets { get; }
public ExponentialHistogramBuckets PositiveBuckets { get; private set; }

public ExponentialHistogramBuckets NegativeBuckets { get; }
public ExponentialHistogramBuckets NegativeBuckets { get; private set; }

internal ExponentialHistogramData Copy()
{
var copy = new ExponentialHistogramData();
copy.Scale = this.Scale;
copy.ZeroCount = this.ZeroCount;
copy.PositiveBuckets = this.PositiveBuckets.Copy();
copy.NegativeBuckets = this.NegativeBuckets.Copy();
return copy;
}
}
2 changes: 1 addition & 1 deletion src/OpenTelemetry/Metrics/MeterProviderSdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ internal MeterProviderSdk(
metricStreamConfig.ViewId = i;
}

if (metricStreamConfig is ExplicitBucketHistogramConfiguration
if (metricStreamConfig is HistogramConfiguration
&& instrument.GetType().GetGenericTypeDefinition() != typeof(Histogram<>))
{
metricStreamConfig = null;
Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry/Metrics/MetricPointOptionalComponents.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@ internal sealed class MetricPointOptionalComponents
internal MetricPointOptionalComponents Copy()
{
MetricPointOptionalComponents copy = new MetricPointOptionalComponents();
copy.HistogramBuckets = this.HistogramBuckets.Copy();
copy.HistogramBuckets = this.HistogramBuckets?.Copy();
copy.Base2ExponentialBucketHistogram = this.Base2ExponentialBucketHistogram?.Copy();
if (this.Exemplars != null)
{
Array.Copy(this.Exemplars, copy.Exemplars, this.Exemplars.Length);
}

// TODO: Copy Base2ExponentialBucketHistogram
return copy;
}
}
Expand Down
59 changes: 39 additions & 20 deletions test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,45 @@ public void HistogramWithOnlySumCount()
Assert.False(enumerator.MoveNext());
}

internal static void AssertExponentialBucketsAreCorrect(Base2ExponentialBucketHistogram expectedHistogram, ExponentialHistogramData data)
{
Assert.Equal(expectedHistogram.Scale, data.Scale);
Assert.Equal(expectedHistogram.ZeroCount, data.ZeroCount);
Assert.Equal(expectedHistogram.PositiveBuckets.Offset, data.PositiveBuckets.Offset);
Assert.Equal(expectedHistogram.NegativeBuckets.Offset, data.NegativeBuckets.Offset);

expectedHistogram.Snapshot();
var expectedData = expectedHistogram.GetExponentialHistogramData();

var actual = new List<long>();
foreach (var bucketCount in data.PositiveBuckets)
alanwest marked this conversation as resolved.
Show resolved Hide resolved
{
actual.Add(bucketCount);
}

var expected = new List<long>();
foreach (var bucketCount in expectedData.PositiveBuckets)
{
expected.Add(bucketCount);
}

Assert.Equal(expected, actual);

actual = new List<long>();
foreach (var bucketCount in data.NegativeBuckets)
{
actual.Add(bucketCount);
}

expected = new List<long>();
foreach (var bucketCount in expectedData.NegativeBuckets)
{
expected.Add(bucketCount);
}

Assert.Equal(expected, actual);
}

[Theory]
[InlineData(AggregationType.Base2ExponentialHistogram, AggregationTemporality.Cumulative)]
[InlineData(AggregationType.Base2ExponentialHistogram, AggregationTemporality.Delta)]
Expand Down Expand Up @@ -284,25 +323,5 @@ internal void ExponentialHistogramTests(AggregationType aggregationType, Aggrega
}
}
}

private static void AssertExponentialBucketsAreCorrect(Base2ExponentialBucketHistogram expectedHistogram, ExponentialHistogramData data)
{
Assert.Equal(expectedHistogram.Scale, data.Scale);
Assert.Equal(expectedHistogram.ZeroCount, data.ZeroCount);
Assert.Equal(expectedHistogram.PositiveBuckets.Offset, data.PositiveBuckets.Offset);
Assert.Equal(expectedHistogram.NegativeBuckets.Offset, data.NegativeBuckets.Offset);

var index = expectedHistogram.PositiveBuckets.Offset;
foreach (var bucketCount in data.PositiveBuckets)
{
Assert.Equal(expectedHistogram.PositiveBuckets[index++], bucketCount);
}

index = expectedHistogram.NegativeBuckets.Offset;
foreach (var bucketCount in data.NegativeBuckets)
{
Assert.Equal(expectedHistogram.PositiveBuckets[index++], bucketCount);
}
}
}
}
99 changes: 99 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricSnapshotTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -163,5 +163,104 @@ public void VerifySnapshot_Histogram()
Assert.Equal(2, snapshot2.MetricPoints[0].GetHistogramCount());
Assert.Equal(15, snapshot2.MetricPoints[0].GetHistogramSum());
}

[Fact]
public void VerifySnapshot_ExponentialHistogram()
{
var expectedHistogram = new Base2ExponentialBucketHistogram();
var exportedMetrics = new List<Metric>();
var exportedSnapshots = new List<MetricSnapshot>();

using var meter = new Meter(Utils.GetCurrentMethodName());
var histogram = meter.CreateHistogram<int>("histogram");
using var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView("histogram", new Base2ExponentialBucketHistogramConfiguration())
.AddInMemoryExporter(exportedMetrics)
.AddInMemoryExporter(exportedSnapshots)
.Build();

// FIRST EXPORT
expectedHistogram.Record(10);
histogram.Record(10);
meterProvider.ForceFlush();

// Verify Metric 1
Assert.Single(exportedMetrics);
var metric1 = exportedMetrics[0];
var metricPoints1Enumerator = metric1.GetMetricPoints().GetEnumerator();
Assert.True(metricPoints1Enumerator.MoveNext());
ref readonly var metricPoint1 = ref metricPoints1Enumerator.Current;
Assert.Equal(1, metricPoint1.GetHistogramCount());
Assert.Equal(10, metricPoint1.GetHistogramSum());
Copy link
Contributor

Choose a reason for hiding this comment

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

Test for min and max as well.

Copy link
Member Author

@alanwest alanwest Mar 15, 2023

Choose a reason for hiding this comment

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

Yes, should probably take care of this on this PR. I noted a bug on the explicit bounds histograms where min/max is actually not captured currently in the snapshot, was gonna fix in another PR, but might just do it here since probably small.

Copy link
Member Author

Choose a reason for hiding this comment

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

Min/max is now tested... gotta take off for the evening, so will fix explicit in a follow up

metricPoint1.TryGetHistogramMinMaxValues(out var min, out var max);
Assert.Equal(10, min);
Assert.Equal(10, max);
AggregatorTest.AssertExponentialBucketsAreCorrect(expectedHistogram, metricPoint1.GetExponentialHistogramData());

// Verify Snapshot 1
Assert.Single(exportedSnapshots);
var snapshot1 = exportedSnapshots[0];
Assert.Single(snapshot1.MetricPoints);
Assert.Equal(1, snapshot1.MetricPoints[0].GetHistogramCount());
Assert.Equal(10, snapshot1.MetricPoints[0].GetHistogramSum());
snapshot1.MetricPoints[0].TryGetHistogramMinMaxValues(out min, out max);
Assert.Equal(10, min);
Assert.Equal(10, max);
AggregatorTest.AssertExponentialBucketsAreCorrect(expectedHistogram, snapshot1.MetricPoints[0].GetExponentialHistogramData());

// Verify Metric == Snapshot
Assert.Equal(metric1.Name, snapshot1.Name);
Assert.Equal(metric1.Description, snapshot1.Description);
Assert.Equal(metric1.Unit, snapshot1.Unit);
Assert.Equal(metric1.MeterName, snapshot1.MeterName);
Assert.Equal(metric1.MetricType, snapshot1.MetricType);
Assert.Equal(metric1.MeterVersion, snapshot1.MeterVersion);

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

// Verify Metric 1 after second export
// This value is expected to be updated.
Assert.Equal(2, metricPoint1.GetHistogramCount());
Assert.Equal(15, metricPoint1.GetHistogramSum());
metricPoint1.TryGetHistogramMinMaxValues(out min, out max);
Assert.Equal(5, min);
Assert.Equal(10, max);

// Verify Metric 2
Assert.Equal(2, exportedMetrics.Count);
var metric2 = exportedMetrics[1];
var metricPoints2Enumerator = metric2.GetMetricPoints().GetEnumerator();
Assert.True(metricPoints2Enumerator.MoveNext());
ref readonly var metricPoint2 = ref metricPoints2Enumerator.Current;
Assert.Equal(2, metricPoint2.GetHistogramCount());
Assert.Equal(15, metricPoint2.GetHistogramSum());
metricPoint1.TryGetHistogramMinMaxValues(out min, out max);
Assert.Equal(5, min);
Assert.Equal(10, max);
AggregatorTest.AssertExponentialBucketsAreCorrect(expectedHistogram, metricPoint2.GetExponentialHistogramData());

// Verify Snapshot 1 after second export
// This value is expected to be unchanged.
Assert.Equal(1, snapshot1.MetricPoints[0].GetHistogramCount());
Assert.Equal(10, snapshot1.MetricPoints[0].GetHistogramSum());
snapshot1.MetricPoints[0].TryGetHistogramMinMaxValues(out min, out max);
Assert.Equal(10, min);
Assert.Equal(10, max);

// Verify Snapshot 2
Assert.Equal(2, exportedSnapshots.Count);
var snapshot2 = exportedSnapshots[1];
Assert.Single(snapshot2.MetricPoints);
Assert.Equal(2, snapshot2.MetricPoints[0].GetHistogramCount());
Assert.Equal(15, snapshot2.MetricPoints[0].GetHistogramSum());
snapshot2.MetricPoints[0].TryGetHistogramMinMaxValues(out min, out max);
Assert.Equal(5, min);
Assert.Equal(10, max);
AggregatorTest.AssertExponentialBucketsAreCorrect(expectedHistogram, snapshot2.MetricPoints[0].GetExponentialHistogramData());
}
}
}
2 changes: 2 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricTestData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,15 @@ public static IEnumerable<object[]> ValidHistogramMinMax
new object[] { new double[] { double.NegativeInfinity, 0, double.PositiveInfinity }, new HistogramConfiguration(), double.NegativeInfinity, double.PositiveInfinity },
new object[] { new double[] { 1 }, new HistogramConfiguration(), 1, 1 },
new object[] { new double[] { 5, 100, 4, 101, -2, 97 }, new ExplicitBucketHistogramConfiguration() { Boundaries = new double[] { 10, 20 } }, -2, 101 },
new object[] { new double[] { 5, 100, 4, 101, -2, 97 }, new Base2ExponentialBucketHistogramConfiguration(), -2, 101 },
};

public static IEnumerable<object[]> InvalidHistogramMinMax
=> new List<object[]>
{
new object[] { new double[] { 1 }, new HistogramConfiguration() { RecordMinMax = false } },
new object[] { new double[] { 1 }, new ExplicitBucketHistogramConfiguration() { Boundaries = new double[] { 10, 20 }, RecordMinMax = false } },
new object[] { new double[] { 1 }, new Base2ExponentialBucketHistogramConfiguration() { RecordMinMax = false } },
};
}
}
Loading