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 2 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;
}
}
}
9 changes: 9 additions & 0 deletions src/OpenTelemetry/Metrics/ExponentialHistogramBuckets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ internal void SnapshotBuckets(CircularBufferBuckets buckets)
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>
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
40 changes: 20 additions & 20 deletions test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,26 @@ 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);

var index = expectedHistogram.PositiveBuckets.Offset;
foreach (var bucketCount in data.PositiveBuckets)
alanwest marked this conversation as resolved.
Show resolved Hide resolved
{
Assert.Equal(expectedHistogram.PositiveBuckets[index++], bucketCount);
}

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

[Theory]
[InlineData(AggregationType.Base2ExponentialHistogram, AggregationTemporality.Cumulative)]
[InlineData(AggregationType.Base2ExponentialHistogram, AggregationTemporality.Delta)]
Expand Down Expand Up @@ -284,25 +304,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);
}
}
}
}
81 changes: 81 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MetricSnapshotTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -163,5 +163,86 @@ 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

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());
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());

// 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());
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());

// 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());
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 } },
};
}
}
59 changes: 58 additions & 1 deletion test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,18 @@ public void AddViewWithInvalidHistogramBoundsThrowsArgumentException(double[] bo
Assert.Contains("Histogram boundaries must be in ascending order with distinct values", ex.Message);
}

[Theory]
[InlineData(-1)]
[InlineData(0)]
[InlineData(1)]
public void AddViewWithInvalidExponentialHistogramConfigThrowsArgumentException(int maxSize)
{
var ex = Assert.Throws<ArgumentException>(() => Sdk.CreateMeterProviderBuilder()
.AddView("name1", new Base2ExponentialBucketHistogramConfiguration { MaxSize = maxSize }));

Assert.Contains("Histogram max size is invalid", ex.Message);
}

[Theory]
[MemberData(nameof(MetricTestData.InvalidHistogramBoundaries), MemberType = typeof(MetricTestData))]
public void AddViewWithInvalidHistogramBoundsIgnored(double[] boundaries)
Expand Down Expand Up @@ -461,7 +473,8 @@ public void ViewWithHistogramConfigurationIgnoredWhenAppliedToNonHistogram()
var exportedItems = new List<Metric>();
using var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView("NotAHistogram", new ExplicitBucketHistogramConfiguration() { Name = "ImAHistogram" })
.AddView("NotAHistogram", new ExplicitBucketHistogramConfiguration() { Name = "ImAnExplicitBoundsHistogram" })
.AddView("NotAHistogram", new Base2ExponentialBucketHistogramConfiguration() { Name = "ImAnExponentialHistogram" })
.AddInMemoryExporter(exportedItems)
.Build();

Expand Down Expand Up @@ -569,6 +582,50 @@ public void ViewToProduceCustomHistogramBound()
Assert.Equal(boundaries.Length + 1, actualCount);
}

[Fact]
public void ViewToProduceExponentialHistogram()
{
var valuesToRecord = new[] { -10, 0, 1, 9, 10, 11, 19 };

using var meter = new Meter(Utils.GetCurrentMethodName());
var exportedItems = new List<Metric>();
using var meterProvider = Sdk.CreateMeterProviderBuilder()
.AddMeter(meter.Name)
.AddView("MyHistogram", new Base2ExponentialBucketHistogramConfiguration())
.AddInMemoryExporter(exportedItems)
.Build();

var histogram = meter.CreateHistogram<long>("MyHistogram");
var expectedHistogram = new Base2ExponentialBucketHistogram();
foreach (var value in valuesToRecord)
{
histogram.Record(value);
expectedHistogram.Record(value);
}

meterProvider.ForceFlush(MaxTimeToAllowForFlush);
Assert.Single(exportedItems);
var metric = exportedItems[0];

Assert.Equal("MyHistogram", metric.Name);

var metricPoints = new List<MetricPoint>();
foreach (ref readonly var mp in metric.GetMetricPoints())
{
metricPoints.Add(mp);
}

Assert.Single(metricPoints);
var metricPoint = metricPoints[0];

var count = metricPoint.GetHistogramCount();
var sum = metricPoint.GetHistogramSum();

AggregatorTest.AssertExponentialBucketsAreCorrect(expectedHistogram, metricPoint.GetExponentialHistogramData());
Assert.Equal(40, sum);
Assert.Equal(7, count);
}

[Theory]
[MemberData(nameof(MetricTestData.ValidHistogramMinMax), MemberType = typeof(MetricTestData))]
public void HistogramMinMax(double[] values, HistogramConfiguration histogramConfiguration, double expectedMin, double expectedMax)
Expand Down