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 3rd and last part support for synchronous instruments - exponential histogram #6136

Merged

Conversation

asafm
Copy link
Contributor

@asafm asafm commented Jan 10, 2024

Epic

This is the 4th PR out of several that implement #5105. See the complete plan here.
Specifically, this is the 3rd and last part that completes the implementation of MemoryMode for the synchronous instrument: exponential histogram.

What was done here?

  • Expanded the existing JMH-based benchmark test to cover any instrument, hence renamed it to InstrumentGarbageCollectionBenchmark and InstrumentGarbageCollectionBenchmarkTest.
    • It's now "plugin" based, and currently, two instruments are tested: asynchronous counter and exponential histogram.
    • Reduced the number of iterations and batch size to a number tested to discover at least a 5% diff in the allocation rate. This makes the test much faster
  • I fixed the last GC issue at Base2ExponentialHistogramIndexer, which allocated lots of implicit classes by not using the local variable.

I will convert it from draft to PR once the previous PR is merged.

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dbaba2f) 91.03% compared to head (82337b5) 91.04%.
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6136   +/-   ##
=========================================
  Coverage     91.03%   91.04%           
  Complexity     5648     5648           
=========================================
  Files           619      619           
  Lines         16443    16443           
  Branches       1663     1663           
=========================================
+ Hits          14969    14970    +1     
  Misses         1010     1010           
+ Partials        464      463    -1     

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

@asafm asafm marked this pull request as ready for review January 14, 2024 09:37
@asafm asafm requested a review from a team January 14, 2024 09:37
@asafm
Copy link
Contributor Author

asafm commented Jan 14, 2024

@jack-berg Ready for review

@asafm
Copy link
Contributor Author

asafm commented Jan 14, 2024

The markdown links check fails, but I don't know how to fix it.

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.

Just one small recommendation.

InstantiationException,
IllegalAccessException {
instrumentTester =
testInstrumentType.instrumentTesterClass.getDeclaredConstructor().newInstance();
Copy link
Member

Choose a reason for hiding this comment

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

#nit: If you change instrumentTesterClass from Class<? extends InstrumentTester> to Supplier<InstrumentTester>, and configure each element in the TestInstrumentType enum with a factory method for creating new instances of InstrumentTester:

public enum TestInstrumentType {
  ASYNC_COUNTER(AsyncCounterTester::new),
  EXPONENTIAL_HISTOGRAM(ExponentialHistogramTester::new);

  final Supplier<InstrumentTester> instrumentTesterSupplier;

  TestInstrumentType(Supplier<InstrumentTester> instrumentTesterSupplier) {
    this.instrumentTesterSupplier = instrumentTesterSupplier;
  }
 
  ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Much better.
I did it with an abstract method since error prone recommended it, and it does seem cleaner.

I also added the ProfileBenchmark and linked to it from the test

@asafm
Copy link
Contributor Author

asafm commented Jan 21, 2024

@jack-berg All set

@jack-berg jack-berg merged commit 737dfef into open-telemetry:main Jan 25, 2024
18 checks passed
@asafm asafm deleted the memory-mode-sync-instruments-part3 branch January 25, 2024 16:47
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