-
Notifications
You must be signed in to change notification settings - Fork 841
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
Memory Mode: Adding 3rd and last part support for synchronous instruments - exponential histogram #6136
Conversation
…moryMode.java Co-authored-by: jack-berg <[email protected]>
…lder.java Co-authored-by: jack-berg <[email protected]>
…t1' into memory-mode-sync-instruments-part1
…t: Exponential Histogram
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
@jack-berg Ready for review |
The markdown links check fails, but I don't know how to fix it. |
There was a problem hiding this 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(); |
There was a problem hiding this comment.
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;
}
..
There was a problem hiding this comment.
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
Added ProfileBenchmark.
@jack-berg All set |
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?
InstrumentGarbageCollectionBenchmark
andInstrumentGarbageCollectionBenchmarkTest
.I will convert it from draft to PR once the previous PR is merged.