Add support for http histogram buckets#4921
Conversation
snazy
left a comment
There was a problem hiding this comment.
I think the MeterFilter approach is reasonable, because I don’t see a Quarkus config property that enables percentile histograms just for http.server.requests.
One thing to tighten: the property/docs describe this as Prometheus histogram buckets, but the filter applies to all active registries. Could we either constrain it with @MeterFilterConstraint(applyTo = PrometheusMeterRegistry.class) or make the config/docs generic?
flyrain
left a comment
There was a problem hiding this comment.
LGTM overall. Thanks @MonkeyCanCode ! Left some minor comments.
| * <p>When set, one histogram bucket is published per boundary so backends can compute | ||
| * percentiles (e.g. p95, p99) at the configured durations. | ||
| */ | ||
| Optional<List<Duration>> histogramSlos(); |
There was a problem hiding this comment.
Why Optional<List<Duration>> here?
Optional appears superfluous as Lists can be empty.
There was a problem hiding this comment.
So when tried to use @WithDefault(""), this doesn't appear to be working as Smallrye appears to threat "" as null for List<Duration>.
There was a problem hiding this comment.
Smallrye Config requires Optional<List<>> (and Optional<Set<>> ) if the list/set is optional. It does not conflate missing list vs empty list. (oddly enough, you don't need this for maps).
| | `polaris.metrics.realm-id-tag.enable-in-http-metrics` | `false` | `boolean` | Whether to include the Realm ID tag in the HTTP server request metrics. <br><br>Beware that if the cardinality of this tag is too high, it can cause performance issues or even crash the server. | | ||
| | `polaris.metrics.realm-id-tag.http-metrics-max-cardinality` | `100` | `int` | The maximum number of Realm ID tag values allowed for the HTTP server request metrics. <br><br>This is used to prevent the number of tags from growing indefinitely and causing performance issues or crashing the server. <br><br>If the number of tags exceeds this value, a warning will be logged and no more HTTP server request metrics will be recorded. | | ||
| | `polaris.metrics.user-principal-tag.enable-in-api-metrics` | `false` | `boolean` | Whether to include the User Principal tag in the API request metrics. <br><br>Beware that if the cardinality of this tag is too high, it can cause performance issues or even crash the server. | | ||
| | `polaris.metrics.http-server-requests.histogram-slos` | | `list of duration` | Service Level Objective (SLO) boundaries for the HTTP server request duration histogram. <br><br>When set, one histogram bucket is published per boundary so backends can compute percentiles (e.g. p95, p99) at the configured durations. | |
There was a problem hiding this comment.
histogram-slos is okay, but I think histogram-bucket-boundaries is clearer
There was a problem hiding this comment.
I''m torn, because Micrometer uses the "SLO" terminology, e.g. DistributionStatisticConfig.builder().serviceLevelObjectives(), so this term may sound more familiar to people accustomed to Micrometer than "Bucket boundaries".
Co-authored-by: Robert Stupp <snazy@snazy.de>
Feature for #4802. As the cardinality is very high with Micrometer (e.g. register 69 metrics per endpoint per unique label), we proceeded with SLOs approach.
Sample metrics:
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)