Add Support for Prometheus Native Histograms #3987
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Fabian Stäber <fabian@fstab.de>
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
...-prometheus_native/src/main/java/io/micrometer/prometheusnative/PrometheusMeterRegistry.java
Show resolved
Hide resolved
...-prometheus_native/src/main/java/io/micrometer/prometheusnative/PrometheusLongTaskTimer.java
Outdated
Show resolved
Hide resolved
...-prometheus_native/src/main/java/io/micrometer/prometheusnative/PrometheusLongTaskTimer.java
Outdated
Show resolved
Hide resolved
...-prometheus_native/src/main/java/io/micrometer/prometheusnative/PrometheusLongTaskTimer.java
Show resolved
Hide resolved
...-prometheus_native/src/main/java/io/micrometer/prometheusnative/PrometheusMeterRegistry.java
Outdated
Show resolved
Hide resolved
...registry-prometheus_native/src/main/java/io/micrometer/prometheusnative/PrometheusMeter.java
Show resolved
Hide resolved
🛠 Lift Auto-fixSome of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1 # Download the patch
curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/micrometer/3987.diff -o lift-autofixes.diff
# Apply the patch with git
git apply lift-autofixes.diff
# Review the changes
git diffWant it all in a single command? Open a terminal in your project's directory and copy and paste the following command: curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/micrometer/3987.diff | git applyOnce you're satisfied, commit and push your changes in your project. Footnotes |
zeitlinger
reviewed
Jul 19, 2023
| @@ -0,0 +1 @@ | |||
| compatibleVersion=SKIP | |||
There was a problem hiding this comment.
where is the version number?
There was a problem hiding this comment.
This is for the API/ABI compatibility check. Since there isn't a previous version of this module to compare with, this configuration skips the compatibility check so the build doesn't fail.
Signed-off-by: Fabian Stäber <fabian@fstab.de>
|
Can this PR got reviewed and merged? |
shakuzen
reviewed
Mar 6, 2024
| Histogram.Builder builder = init(id, Histogram.newBuilder(prometheusProperties)); | ||
| double[] classicBuckets = distributionStatisticConfig.getServiceLevelObjectiveBoundaries(); | ||
| if (classicBuckets != null && classicBuckets.length == 0) { | ||
| builder = builder.classicOnly().withClassicBuckets(classicBuckets); |
There was a problem hiding this comment.
I'm curious why use classicOnly if it's only adding the configured SLOs? Wouldn't adding those to the native histogram be better?
|
|
||
| @SuppressWarnings("unchecked") | ||
| private <T extends MetricWithFixedMetadata.Builder<?, ?>> T init(Meter.Id id, T builder) { | ||
| return (T) builder.withName(PrometheusNaming.sanitizeMetricName(id.getName())) |
There was a problem hiding this comment.
Is it intentional to use PrometheusNaming directly here (and other places) rather than make an implementation of Micrometer's NamingConvention that delegates to it? See PrometheusNamingConvention from the original module. Users will sometimes want to configure a custom naming convention for a MeterRegistry to use.
| @@ -0,0 +1 @@ | |||
| compatibleVersion=SKIP | |||
There was a problem hiding this comment.
This is for the API/ABI compatibility check. Since there isn't a previous version of this module to compare with, this configuration skips the compatibility check so the build doesn't fail.
jonatan-ivanov
added a commit
to jonatan-ivanov/micrometer
that referenced
this pull request
Mar 11, 2024
Dependencies were upgraded to the 1.x versions of the Prometheus client artifacts in micrometer-registry-prometheus. This is a breaking change compared to the code that was there before. This is unavoidable due to the breaking changes in the Prometheus client from 0.x to 1.x. The previous code from micrometer-registry-prometheus was moved to a new module micrometer-registry-prometheus-simpleclient that is deprecated but available to give users a backward compatible option in case they need to make changes to use the new Prometheus client. The base package used in micrometer-registry-prometheus was changed to io.micrometer.prometheusmetrics to differentiate it from the prior package now used in micrometer-registry-prometheus-simpleclient. This avoids split packages and allows both modules to be used in the same application even. Tests were adapted to the new API and differences in the scrape output. Tests involving exemplars are commented out until exemplar support is added. PrometheusHistogram's cumulative buckets were converted to delta in the registry. The new Prometheus client wants delta buckets which is weird since it converts them back to cumulative internally. PrometheusNamingConvention behavior was changed, it does not append "_total" to counters anymore. The new Prometheus client validates Counter names and check if they end with "_total". If they don't, it appends "_total". If they do, it throws an exception: java.lang.IllegalArgumentException: 'cnt_total': Illegal metric name... Because of this behavior, if we want to use PrometheusNamingConvention, we need to modify it so that it does not append "_total". Known issues in the micrometer-registry-prometheus module after this PR: - Exemplars and Native Histograms do not work. - VictoriaMetrics histograms are not supported. - The OSGi test was failing when the new Prometheus client was used, so it was updated to use the -simpleclient module for now. See micrometer-metricsgh-3987 See micrometer-metricsgh-4406 Co-authored-by: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com>
jonatan-ivanov
added a commit
that referenced
this pull request
Mar 11, 2024
Dependencies were upgraded to the 1.x versions of the Prometheus client artifacts in micrometer-registry-prometheus. This is a breaking change compared to the code that was there before. This is unavoidable due to the breaking changes in the Prometheus client from 0.x to 1.x. The previous code from micrometer-registry-prometheus was moved to a new module micrometer-registry-prometheus-simpleclient that is deprecated but available to give users a backward compatible option in case they need to make changes to use the new Prometheus client. The base package used in micrometer-registry-prometheus was changed to io.micrometer.prometheusmetrics to differentiate it from the prior package now used in micrometer-registry-prometheus-simpleclient. This avoids split packages and allows both modules to be used in the same application even. Tests were adapted to the new API and differences in the scrape output. Tests involving exemplars are commented out until exemplar support is added. PrometheusHistogram's cumulative buckets were converted to delta in the registry. The new Prometheus client wants delta buckets which is weird since it converts them back to cumulative internally. PrometheusNamingConvention behavior was changed, it does not append "_total" to counters anymore. The new Prometheus client validates Counter names and check if they end with "_total". If they don't, it appends "_total". If they do, it throws an exception: java.lang.IllegalArgumentException: 'cnt_total': Illegal metric name... Because of this behavior, if we want to use PrometheusNamingConvention, we need to modify it so that it does not append "_total". Known issues in the micrometer-registry-prometheus module after this PR: - Exemplars and Native Histograms do not work. - VictoriaMetrics histograms are not supported. - The OSGi test was failing when the new Prometheus client was used, so it was updated to use the -simpleclient module for now. See gh-3987 See gh-4406 Co-authored-by: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am maintainer of the Prometheus Java client library, and we recently released the
1.0.0-alpha-1version. This is a complete re-write of the Prometheus client library. The main goal is supporting the new Prometheus Native Histograms, but we are also using the opportunity to get rid of legacy code.The Prometheus Java client library is a low-level metrics library, most users will use it through Micrometer or other abstractions. Our goal is to make the new library integrate well with Micrometer.
I would appreciate your feedback on the API before we mark the
1.0.0release, and I'm happy to make changes to the API to make it work well with Micrometer.This PR is an example of how the new Prometheus Java library could be used. It adds a new
io.micrometer:micrometer-registry-prometheus_nativeregistry as an alternative to the existingio.micrometer:micrometer-registry-prometheusregistry. My goal is to make you aware and get feedback, not that the PR gets merged as-is.I also created a complete end-to-end example:
The example application has a README showing how to build it, run it, and query a Prometheus native histogram.
There are a few subtle breaking changes related to the way Prometheus metric names are generated:
_totalsuffix is reserved for counters. The new library removes the_totalsuffix for other metric types. As a resultdisk_total_bytesgets renamed todisk_bytesbecause it's a gauge.http_server_requests_seconds_maxget renamed tohttp_server_requests_max_seconds.{.005, .01, .025, .05, .1, .25, .5, 1, 2.5, 5, 10}aligned with the default used in Prometheus client libraries for other programming languages.As these are breaking changes, I added this as a new
micrometer-registry-prometheus_nativeregistry rather than updating the existing one.Anyway, it would be great to hear from you, and I'm happy to adapt the API before releasing
1.0.0.