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

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

Merged

Conversation

asafm
Copy link
Contributor

@asafm asafm commented Dec 7, 2023

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.

Copy link

codecov bot commented Dec 7, 2023

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
Copy link
Member

@asafm is this ready for review?

@asafm
Copy link
Contributor Author

asafm commented Dec 20, 2023

@asafm is this ready for review?

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

@asafm asafm marked this pull request as ready for review December 24, 2023 13:23
@asafm asafm requested a review from a team December 24, 2023 13:23
@asafm
Copy link
Contributor Author

asafm commented Dec 24, 2023

@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 🙏

@asafm asafm changed the title [DRAFT] Memory Mode: Adding 2nd part support for synchronous instruments - exponential histogram Memory Mode: Adding 2nd part support for synchronous instruments - exponential histogram Dec 24, 2023
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

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

@asafm
Copy link
Contributor Author

asafm commented Jan 7, 2024

@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
Copy link
Contributor Author

asafm commented Jan 9, 2024

@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
Copy link
Contributor Author

asafm commented Jan 9, 2024

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

@jack-berg
Copy link
Member

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

@jack-berg jack-berg merged commit 0641844 into open-telemetry:main Jan 12, 2024
17 of 18 checks passed
@asafm asafm deleted the memory-mode-sync-instruments-part2 branch January 14, 2024 09:28
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