Memory Mode: Adding 2nd part support for synchronous instruments - exponential histogram

[asafm]

Epic

This is the 3rd PR out of several that implement #5105. See the complete plan here.

What was done here?

Added support for MemoryMode - specifically REUSABLE_DATA - for Exponential Histogram.

• Switched to use a reusable array, which grows as needed but never shrinks physically (only the size parameter can change). This reusable array is DynamicPrimitiveLongList, which replaces PrimitiveLongList for REUSABLE_DATA mode. It allows accessing the primitive values through long getLong(i) and setLong(i,longValue)
  • This was done to avoid memory allocations of arrays upon size changes for those arrays (primarily when the number of buckets changes for the histogram)
  • This meant modifying the marshaling code to efficiently access those values, instead of using the List methods which uses auto-boxing -- meaning more allocations.
    • A test was added to verify the marshalling code works properly with the reusable data structure of DynamicPrimitiveLongList
• AggregatorFactory now has a required MemoryMode parameter, enabling the aggregator factories to relay the memory mode used by the MetricsReader to the Aggregator classes, as they need to behave differently upon the memory mode. Specifically, in this PR, only DoubleBase2ExponentialHistogramAggregator takes it into account. Future PRs will augment all other Aggregators classes. They have all been modified, but are currently ignoring this value.
• DoubleBase2ExponentialHistogramAggregator has been changed to support REUSABLE_DATA. In that memory mode, the point returned by the aggregation in doAggregateThenMaybeReset() is now a reusable MutableExponentialHistogramPointData. We create this point instance once upon initialization, and then setting all of its values and returning it as a result. We know it is safe, since the MetricsReader implementations which support REUSABLE_DATA knows that the list of data point received can not be used concurrently.
  • The MutableExponentialHistogramPointData fields are rather simple like the immutable version, aside from the ExponentialHistogramBuckets ones: the positive and the negative. Here, as well, we had to create a mutable version called MutableExponentialHistogramBuckets.
    • We take the instance of MutableExponentialHistogramBuckets from the reusable point, and simply set the values, retrieved from the DoubleBase2ExponentialHistogramBuckets like scale, offset and totalCount. The biggest difference is how we retrieve the bucket counts. In the mutable version, the bucket counts were a List<Long>, which behind the scenes was an implementation of List<Long> holding an array of primitive longs - immutable and not-resizable.
      In the mutable version, the bucket counts are stored in DynamicPrimitiveLongList, which grow in size when needed (i.e. bucket count changes). DoubleBase2ExponentialHistogramBuckets now has a method that returns the bucket counts into a reusable DynamicPrimitiveLongList.
    • Another change we had to do inside DoubleBase2ExponentialHistogramBuckets is the place we keep and update the bucket counts. In the immutable version, we kept allocating a new AdaptingCircularBufferCounter each time the number of bucket changed - inside it meant allocating a new array with fixed size of maxBuckets. Now in the mutable version, we have an additional instance of AdaptingCircularBufferCounter we keep, and we reuse it. Each time we need to change the number of buckets, we do it on the inactive AdaptingCircularBufferCounter instance, and then swap them.
  • I extracted EmptyExponentialHistogramBuckets into its own file, since I needed to use it at MutableExponentialHistogramPointData, when I initialize the positive and negative buckets. This is to avoid using null, hence consistent with other mutable classes.

Next PRs

• The performance (memory allocation rate) test - we adapt them to test a pluggable instrument, and have two of them: the existing async instrument test, and the new exponential histogram test
• The rest of the aggregators.

[codecov]

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (d9f9812) 91.01% compared to head (9b0d48e) 91.01%.

Files	Patch %	Lines
...nal/data/MutableExponentialHistogramPointData.java	82.05%	2 Missing and 12 partials ⚠️
...ernal/data/MutableExponentialHistogramBuckets.java	77.77%	1 Missing and 5 partials ⚠️
...lemetry/sdk/internal/DynamicPrimitiveLongList.java	97.67%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@ ## main #6058 +/- ## ========================================== Coverage 91.01% 91.01% - Complexity 5702 5775 +73  ========================================== Files 630 634 +4 Lines 16710 16930 +220 Branches 1656 1693 +37 ========================================== + Hits 15208 15409 +201  - Misses 1047 1049 +2  - Partials 455 472 +17 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jack-berg]

@asafm is this ready for review?

[asafm]

@asafm is this ready for review?

I'll give it one more pass, fill the description and I'll ping you .

[asafm]

@jack-berg Ready for review now. I've added a detailed overview of what was added in the PR, so it's good to read this first before diving into the code 🙏

[jack-berg]

Looks pretty good to me! Just a handful of minor comments.

[asafm]

@jack-berg Thanks for the review. I fixed all but one comment, which I didn't know what to do. I replied to it.

[asafm]

@jack-berg I pushed the fix to all remaining comments.
Also forced to add 2 unit tests for the data classes to get pass code coverage tests.

[asafm]

@jack-berg How do you rebuild the markdown links check?

[jack-berg]

Thanks! @open-telemetry/java-approvers I'll plan on merging this in on 1/10/24 unless anyone indicates they plan on reviewing.