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

Declare HdrHistogram as a runtime dependency #3263

Merged

Conversation

shakuzen
Copy link
Member

@shakuzen shakuzen commented Jul 5, 2022

HdrHistogram is not exposed in the API. It is an implementation detail of client-side percentiles. The scope of the dependency can be changed accordingly.

This was found while working on #1599.

HdrHistogram is not exposed in the API. It is an implementation detail of client-side percentiles. The scope of the dependency can be changed accordingly.
@shakuzen shakuzen added enhancement A general enhancement module: micrometer-core An issue that is related to our core module labels Jul 5, 2022
@shakuzen shakuzen added this to the 1.10.0-M3 milestone Jul 5, 2022
@shakuzen
Copy link
Member Author

shakuzen commented Jul 5, 2022

I may have been too eager with this change. HdrHistogram's DoubleHistogram and DoubleRecorder is part of the (package-private) API in the public class TimeWindowPercentileHistogram:

@Override
DoubleRecorder newBucket() {
return new DoubleRecorder(percentilePrecision(distributionStatisticConfig));
}
@Override
void recordDouble(DoubleRecorder bucket, double value) {
bucket.recordValue(value);
}
@Override
void recordLong(DoubleRecorder bucket, long value) {
bucket.recordValue(value);
}
@Override
void resetBucket(DoubleRecorder bucket) {
bucket.reset();
}
@Override
DoubleHistogram newAccumulatedHistogram(DoubleRecorder[] ringBuffer) {
return new DoubleHistogram(percentilePrecision(distributionStatisticConfig));
}

Though I'm not sure if that is a problem for changing the scope here or not. Outside of the local package, I don't think there is reference to any HdrHistogram classes in API that users would be able to compile against.

@shakuzen shakuzen modified the milestones: 1.10.0-M3, 1.10 backlog Jul 11, 2022
@shakuzen shakuzen changed the title Mark HdrHistogram correctly as a runtime dependency Declare HdrHistogram as a runtime dependency Aug 3, 2022
@shakuzen
Copy link
Member Author

shakuzen commented Aug 3, 2022

With #3325 in now, I think we can give this a try in 1.10. User code should not compile against Micrometer API that has the HdrHistogram API in it with normal usage that I can think of. If client-side percentiles are being used, HdrHistogram is still needed at runtime. This pull request still declares HdrHistogram as needed at runtime.

@shakuzen shakuzen modified the milestones: 1.10 backlog, 1.10.0-M4 Aug 3, 2022
@shakuzen shakuzen merged commit 2c39c9f into micrometer-metrics:main Aug 3, 2022
@shakuzen shakuzen deleted the hdr-histogram-runtime-dep branch August 3, 2022 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants