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

Support configuring exponential histograms at the meter level #5459

Open
shakuzen opened this issue Sep 2, 2024 · 4 comments
Open

Support configuring exponential histograms at the meter level #5459

shakuzen opened this issue Sep 2, 2024 · 4 comments
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module registry: otlp OpenTelemetry Protocol (OTLP) registry-related
Milestone

Comments

@shakuzen
Copy link
Member

shakuzen commented Sep 2, 2024

This is a follow up to #3861 and requests from users such as #3959 (comment).

Currently, exponential histograms can only be configured at the registry level for the OTLP registry via OtlpConfig. This is fine for users who want to use exponential histograms for everything that has percentileHistogram enabled. However, more fine grained control gives more flexibility.

As a workaround, configuring any SLO on a meter's DistributionStatisticConfig will cause exponential histograms to not be used (they don't support arbitrary additional buckets).

Another aspect of configuration would be the (max) bucket count, which again is currently controlled by OtlpConfig for all meters in the registry.

@shakuzen shakuzen added enhancement A general enhancement module: micrometer-core An issue that is related to our core module registry: otlp OpenTelemetry Protocol (OTLP) registry-related labels Sep 2, 2024
@shakuzen shakuzen added this to the 1.14.0-M3 milestone Sep 2, 2024
@shakuzen shakuzen modified the milestones: 1.14.0-M3, 1.14.0-RC1 Sep 6, 2024
@lenin-jaganathan
Copy link
Contributor

lenin-jaganathan commented Sep 23, 2024

At this point, to align with the micrometer's design and isolate the implementation-specific things away from common APIs I would like to propose having maxBuckets() as part of the DistributionStatisticConfig.

The justification is that maxBuckets is a generic concept that works fine with every kind of histogram out there and can be re-used for new histogram types as well. This also provides a switch for implementations/frameworks to make high-level restrictions on the number of buckets that can be associated with the histograms.

EDIT: Even for the concern of this problem of customizing exponential histograms per meter basis, the scale is not a set value, it is a dynamic value determined by the histogram and doesn't need per meter config and it doesn't have any resource issues. The factor that determines the scale at which the histogram operates is the maxBuckets which can be different for different metrics (e.g: server/client metrics might have wider distribution and might require higher buckets but something like a cache read might have a smaller range and fewer buckets). Bucket count is the single most important thing that can be configured to manage the accuracy and resource usage.

@shakuzen shakuzen modified the milestones: 1.14.0-RC1, 1.x Oct 1, 2024
@lenin-jaganathan
Copy link
Contributor

@shakuzen I am willing to work on this before our GA if we can brainstorm on an approach.

@shakuzen
Copy link
Member Author

Thanks for the offer to help @lenin-jaganathan. It's appreciated. We're past RC1 now for 1.14, and it's our general policy that we won't make API changes for the GA release after a release candidate is made. So I think this enhancement will have to wait until 1.15.0, but that means we have more time to get the design right and hopefully get more feedback on the feature from users. This lost some priority due to the lack of feedback from any users on trying out exponential histograms, but perhaps that's at least partially because users want per meter customization before they try exponential histograms. It's hard to say since we haven't heard anything from users since exponential histograms was released in a milestone, despite the interest there seemed to be for the feature before it was merged.

As for the design, I'm happy to discuss. I think the problem with making max buckets general configuration is that it doesn't make sense with fixed bucket histograms. The way to control the number of buckets there is with expected min/max values, which is also much more intuitive than knowing how many buckets is sufficient for accurate percentile estimation for a given histogram. This is why previously I suggested making it specific to exponential histograms (e.g. maxExponentialBuckets or similar). What do you think?

@lenin-jaganathan
Copy link
Contributor

It feels like we are kind of in a catch 22 here. We need feedback and users need more fine tuning configuration. Irrespective of this we (in our org) are planning to go with whatever is available as of now. It would have been much better if we had control over the number of buckets per meter. And I think atleast for us it is the only thing we are looking for as an improvement on the current implementation.

I am not keen on exposing something like a exponentialHistogramBuckets on meter api's because it seems too specific to an implementation.

Why I said maxBuckets is that, we had use-case where we wanted services to export only a limited number of buckets to the back-end. Some backends had hard limit on number of buckets, so we had a custom solution that checks if the buckets generated is within limit if not it keeps only the first n buckets. Also, this provides a full backward compatibility with our current configuration, we can keep the existing behaviour as is.

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 registry: otlp OpenTelemetry Protocol (OTLP) registry-related
Projects
None yet
Development

No branches or pull requests

2 participants