-
Notifications
You must be signed in to change notification settings - Fork 978
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
Add exemplar support to _count #3996
Add exemplar support to _count #3996
Conversation
Prometheus did not support exemplars for Summaries and it did not support exemplars for anything else than buckets on Histograms. Since Prometheus now supports exemplars on every time series, we can add exemplars to Summary/Histogram _count, see: prometheus/prometheus#11982
This comment was marked as off-topic.
This comment was marked as off-topic.
@HaloFour If you are interested and willing to spend the time on this, would you mind reviewing and/or testing it? |
...crometer-registry-prometheus/src/main/java/io/micrometer/prometheus/PrometheusHistogram.java
Show resolved
Hide resolved
...s/micrometer-registry-prometheus/src/main/java/io/micrometer/prometheus/PrometheusTimer.java
Show resolved
Hide resolved
@@ -96,18 +113,43 @@ protected void recordNonNegative(long amount, TimeUnit unit) { | |||
if (histogram != null) { | |||
histogram.recordLong(TimeUnit.NANOSECONDS.convert(amount, unit)); | |||
} | |||
|
|||
if (!histogramExemplarsEnabled && exemplarSampler != null) { | |||
updateLastExemplar(TimeUtils.nanosToUnit(amount, this.baseTimeUnit()), exemplarSampler); |
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.
LongDoubleConversion: Conversion from long to double may lose precision; use an explicit cast to double if this was intentional
updateLastExemplar(TimeUtils.nanosToUnit(amount, this.baseTimeUnit()), exemplarSampler); | |
updateLastExemplar(TimeUtils.nanosToUnit((double) amount, this.baseTimeUnit()), exemplarSampler); |
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
Command | Usage |
---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
This comment was marked as off-topic.
This comment was marked as off-topic.
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.
Both in PrometheusDistributionSummary
and in PrometheusTimer
, the last sampled exemplars can be recorded in two ways:
PrometheusHistogram
records it so that the exemplar will not be sampled twice (onesample
call inPrometheusHistogram
):Histogram
use-case (there are buckets +_count
)- If that is not available,
PrometheusDistributionSummary
/PrometheusTimer
record it directly:Summary
use-case (no buckets, just_count
)
next = exemplarSampler.sample(exemplarValue, bucketFrom, bucketTo, prev); | ||
previusBucketExemplar = exemplars.get(index); | ||
previousLastExemplar = lastExemplar.get(); | ||
nextExemplar = exemplarSampler.sample(exemplarValue, bucketFrom, bucketTo, previusBucketExemplar); |
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.
Please notice that here and both in PrometheusDistributionSummary
/PrometheusTimer
the value of the last exemplar is the recorded value. I think this is expected, but the value for _count
should not be the value that was recorded but (in these cases) 1.0
.
Because of this the value is set in the registry (see my comment on it).
} | ||
else { | ||
samples.add(new Collector.MetricFamilySamples.Sample(conventionName + "_count", tagKeys, tagValues, | ||
count, copyExemplarWithNewValue(1.0, lastExemplar))); |
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.
Here I'm setting a new value for the Exemplar
. This is needed because the value of the last recorded Exemplar is the last recorded (and sampled) value by PrometheusDistributionSummary
/PrometheusTimer
. That conforms the "contract" for being the value of the .lastExemplar()
(which can be used elsewhere, i.e. in _sum
) but for _count
the value should be 1.0
since the counter is incremented with 1 in these cases.
Looks simple enough, and the scraped metrics look good. Are you thinking of adding the exemplar to I'm trying to figure out which version of Prometheus we're using and whether I can test the updated format with it. We're on 2.28.1 so I can't test it with our actual stack right now. |
I'm not sure how would that be used. Do you do a |
Since it is now EOL, I think we can proceed with shipping this feature in our upcoming minor release and see what feedback we get from users. At least for now, I would say it seems reasonable that we don't target versions of Prometheus that are not supported by the project itself. |
Micrometer introduced extended exemplars functionality that adds exemplars to _count too not only to histogram buckets, see: micrometer-metrics/micrometer#3996 Because of this, some verifications should be changed.
Micrometer introduced extended exemplars functionality that adds exemplars to _count too not only to histogram buckets, see: micrometer-metrics/micrometer#3996 Because of this, some verifications should be changed.
Micrometer introduced extended exemplars functionality that adds exemplars to _count too not only to histogram buckets, see: micrometer-metrics/micrometer#3996 Because of this, some verifications should be changed.
Prometheus did not support exemplars for Summaries and it did not support exemplars for anything else than buckets on Histograms. Since Prometheus now supports exemplars on every time series, we can add exemplars to Summary/Histogram _count, see: prometheus/prometheus#11982
Also related: #3460
Since the support for this was added in Prometheus sever in
2.43.0
and the previous behavior was dropping the whole scrape response, this change (as-is in this draft) will break users who are using older Prometheus servers (2.43.0
was released on 2023-03-21).We should figure out how to handle this based on the Prometheus support terms: it seems the oldest version that is still supported by Prometheus maintainers but does not have this functionality will reach EOL by 2023-07-31 (in a ~week).