Skip to content

[SPARK-59935] Fix Prometheus _sum and quantiles of histograms and timers - #922

Closed
dongjoon-hyun wants to merge 2 commits into
apache:mainfrom
dongjoon-hyun:SPARK-59935
Closed

dongjoon-hyun wants to merge 2 commits into
apache:mainfrom
dongjoon-hyun:SPARK-59935

Conversation

@dongjoon-hyun

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This PR fixes the Prometheus summaries of Dropwizard histograms and timers in PrometheusPullModelHandler.

  • Track the sum of all recorded values in the new SummingHistogram and SummingTimer, which the operator now uses, and export _sum only for them.
  • Export the median and the 99.9th percentile as the 0.5 and 0.999 quantiles of non-nanos histograms, instead of the mean and the 99th percentile.

Why are the changes needed?

_sum was the mean of the reservoir multiplied by the count. Since the mean covers only roughly the last 5 minutes, _sum could decrease between scrapes, which breaks rate(). This becomes visible once #918 records real latencies.

Does this PR introduce any user-facing change?

Yes. The values of _sum and of the quantiles above change compared to 1.0.0 (2026-07-26), while metric names and types are unchanged. The migration guide is updated.

How was this patch tested?

Pass the CIs with the newly added test cases.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Opus 5.5

@peter-toth peter-toth left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @dongjoon-hyun!

_sum now comes from a running total in SummingHistogram and SummingTimer instead of the reservoir mean times the count. The non-nanos histograms now export the real median and 99.9th percentile. I checked that a SummingTimer created after registerSource reaches the Prometheus handler as itself, so _sum shows up through the real registration path. The one problem is the placeholder ticket in the migration guide. Separately, the operator_sdk_* histograms record whole seconds, so a reconcile under one second adds 0 to the new _sum. That's older than this PR, so I filed SPARK-59949 for it.

Blocking

  • 1. Placeholder ticket in the migration guide: The new entry links SPARK-XXXXX instead of SPARK-59935. inline

Comment thread docs/migration_guide.md Outdated
decrease between scrapes, so `rate()` of it and the average derived from it were wrong. Also, the
`0.5` and `0.999` quantiles of the `operator_sdk_*` histograms are the median and the 99.9th
percentile instead of the mean and the 99th percentile. A histogram or timer of a custom metrics
source has no `_sum` unless it is a `SummingHistogram` or a `SummingTimer` ([SPARK-XXXXX](https://issues.apache.org/jira/browse/SPARK-XXXXX)).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding 1. The entry still links the SPARK-XXXXX placeholder.

Suggested change
source has no `_sum` unless it is a `SummingHistogram` or a `SummingTimer` ([SPARK-XXXXX](https://issues.apache.org/jira/browse/SPARK-XXXXX)).
source has no `_sum` unless it is a `SummingHistogram` or a `SummingTimer` ([SPARK-59935](https://issues.apache.org/jira/browse/SPARK-59935)).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review, @peter-toth. I applied your suggestion in 27894eb. Thank you for filing SPARK-59949 for the whole seconds of the operator_sdk_* histograms, too.

@dongjoon-hyun

Copy link
Copy Markdown
Member Author

Thank you, @peter-toth !

@dongjoon-hyun dongjoon-hyun added this to the 1.1.0 milestone Oct 2, 2026
@dongjoon-hyun

Copy link
Copy Markdown
Member Author

Merged to main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants