Skip to content

Conversation

@alanwest
Copy link
Member

@alanwest alanwest commented Mar 14, 2023

Adds some tests for exponential histogram configuration. Also adds InMemory exporter support.

Base2ExponentialBucketHistogramConfiguration is still internal so not adding a changelog entry with this PR.

@alanwest alanwest requested a review from a team March 14, 2023 22:53
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #4303 (4b85dae) into main (5aad3e0) will increase coverage by 0.09%.
The diff coverage is 93.54%.

❗ Current head 4b85dae differs from pull request most recent head e3c3a2d. Consider uploading reports for the commit e3c3a2d to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4303      +/-   ##
==========================================
+ Coverage   84.38%   84.48%   +0.09%     
==========================================
  Files         298      298              
  Lines       11872    11875       +3     
==========================================
+ Hits        10018    10032      +14     
+ Misses       1854     1843      -11     
Impacted Files Coverage Δ
...cs/Base2ExponentialBucketHistogramConfiguration.cs 66.66% <66.66%> (-33.34%) ⬇️
...lemetry/Metrics/Base2ExponentialBucketHistogram.cs 100.00% <100.00%> (ø)
...enTelemetry/Metrics/ExponentialHistogramBuckets.cs 100.00% <100.00%> (+9.52%) ⬆️
.../OpenTelemetry/Metrics/ExponentialHistogramData.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 91.02% <100.00%> (ø)
...Telemetry/Metrics/MetricPointOptionalComponents.cs 83.33% <100.00%> (+3.33%) ⬆️

... and 8 files with indirect coverage changes

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[].

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

@alanwest alanwest merged commit 30e4054 into open-telemetry:main Mar 15, 2023
@alanwest alanwest deleted the alanwest/exp-hist-tests-and-inmemory-exporter branch March 15, 2023 20:20
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants