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

Force buckets in a histogram to be monotonic for quantile estimation #2610

Merged
merged 3 commits into from Apr 14, 2017

Conversation

Projects
None yet
5 participants
@jjneely
Contributor

jjneely commented Apr 11, 2017

The assumption that bucket counts increase monotonically with increasing
upperBound may be violated during:

  • Recording rule evaluation of histogram_quantile, especially when rate()
    has been applied to the underlying bucket timeseries.
  • Evaluation of histogram_quantile computed over federated bucket
    timeseries, especially when rate() has been applied

This is because scraped data is not made available to RR evalution or
federation atomically, so some buckets are computed with data from the N
most recent scrapes, but the other buckets are missing the most recent
observations.

Monotonicity is usually guaranteed because if a bucket with upper bound
u1 has count c1, then any bucket with a higher upper bound u > u1 must
have counted all c1 observations and perhaps more, so that c >= c1.

Randomly interspersed partial sampling breaks that guarantee, and rate()
exacerbates it. Specifically, suppose bucket le=1000 has a count of 10 from
4 samples but the bucket with le=2000 has a count of 7, from 3 samples. The
monotonicity is broken. It is exacerbated by rate() because under normal
operation, cumulative counting of buckets will cause the bucket counts to
diverge such that small differences from missing samples are not a problem.
rate() removes this divergence.)

bucketQuantile depends on that monotonicity to do a binary search for the
bucket with the qth percentile count, so breaking the monotonicity
guarantee causes bucketQuantile() to return undefined (nonsense) results.

As a somewhat hacky solution until the Prometheus project is ready to
accept the changes required to make scrapes atomic, we calculate the
"envelope" of the histogram buckets, essentially removing any decreases
in the count between successive buckets.

Force buckets in a histogram to be monotonic for quantile estimation
The assumption that bucket counts increase monotonically with increasing
upperBound may be violated during:

  * Recording rule evaluation of histogram_quantile, especially when rate()
     has been applied to the underlying bucket timeseries.
  * Evaluation of histogram_quantile computed over federated bucket
     timeseries, especially when rate() has been applied

This is because scraped data is not made available to RR evalution or
federation atomically, so some buckets are computed with data from the N
most recent scrapes, but the other buckets are missing the most recent
observations.

Monotonicity is usually guaranteed because if a bucket with upper bound
u1 has count c1, then any bucket with a higher upper bound u > u1 must
have counted all c1 observations and perhaps more, so that c  >= c1.

Randomly interspersed partial sampling breaks that guarantee, and rate()
exacerbates it. Specifically, suppose bucket le=1000 has a count of 10 from
4 samples but the bucket with le=2000 has a count of 7, from 3 samples. The
monotonicity is broken. It is exacerbated by rate() because under normal
operation, cumulative counting of buckets will cause the bucket counts to
diverge such that small differences from missing samples are not a problem.
rate() removes this divergence.)

bucketQuantile depends on that monotonicity to do a binary search for the
bucket with the qth percentile count, so breaking the monotonicity
guarantee causes bucketQuantile() to return undefined (nonsense) results.

As a somewhat hacky solution until the Prometheus project is ready to
accept the changes required to make scrapes atomic, we calculate the
"envelope" of the histogram buckets, essentially removing any decreases
in the count between successive buckets.
@jjneely

This comment has been minimized.

Show comment
Hide comment
@jjneely

jjneely Apr 11, 2017

Contributor

The idea here was taken from #1887

Contributor

jjneely commented Apr 11, 2017

The idea here was taken from #1887

@juliusv

This comment has been minimized.

Show comment
Hide comment
@juliusv

juliusv Apr 11, 2017

Member

The idea generally seems sensible to me, but I'll let the guardians of the histograms (@beorn7 and @brian-brazil) judge this.

Member

juliusv commented Apr 11, 2017

The idea generally seems sensible to me, but I'll let the guardians of the histograms (@beorn7 and @brian-brazil) judge this.

@mattbostock

This comment has been minimized.

Show comment
Hide comment
@mattbostock

mattbostock Apr 12, 2017

Contributor
Contributor

mattbostock commented Apr 12, 2017

@jjneely

This comment has been minimized.

Show comment
Hide comment
@jjneely

jjneely Apr 13, 2017

Contributor

If I take the rate(){1m} of histogram data (what I'd pass histogram_quantile() and build the CDF (so the bucket boundaries vs observations) I get fun data that looks like this.

prometheus-cdf-1

Where the red line represents where 0.99 of the observations fall. It should intersect the CDF curve at the bucket containing the 0.99 quantile. As you can see we have multiple choices and we use a binary search to find the bucket. So the binary search may find and return any number of bucket boundary options.

We've run this code in production for a week at this point. Yup, we know the data is still less than accurate when this case happens. However, this has removed a lot of the noisy, very high peaks from the graphs. So we get a much better idea of the trend of the data.

Contributor

jjneely commented Apr 13, 2017

If I take the rate(){1m} of histogram data (what I'd pass histogram_quantile() and build the CDF (so the bucket boundaries vs observations) I get fun data that looks like this.

prometheus-cdf-1

Where the red line represents where 0.99 of the observations fall. It should intersect the CDF curve at the bucket containing the 0.99 quantile. As you can see we have multiple choices and we use a binary search to find the bucket. So the binary search may find and return any number of bucket boundary options.

We've run this code in production for a week at this point. Yup, we know the data is still less than accurate when this case happens. However, this has removed a lot of the noisy, very high peaks from the graphs. So we get a much better idea of the trend of the data.

@juliusv

This comment has been minimized.

Show comment
Hide comment
@juliusv

juliusv Apr 13, 2017

Member

I've only seen @brian-brazil comment on https://groups.google.com/d/msg/prometheus-developers/JgWahlzEhl4/E9AF-cB8AgAJ on this.

Let's review my current understanding:

  • The current behavior gives you an erratic, spikey answer, but at least it's more obviously wrong, so users may notice with more likelihood.
  • The proposed behavior is more useful / usable, but people who don't know the implementation here might incorrectly assume that nothing is off at all.

I don't know what's worse. I'm leaning towards accepting it. @brian-brazil are there more considerations to this than my thoughts above?

Member

juliusv commented Apr 13, 2017

I've only seen @brian-brazil comment on https://groups.google.com/d/msg/prometheus-developers/JgWahlzEhl4/E9AF-cB8AgAJ on this.

Let's review my current understanding:

  • The current behavior gives you an erratic, spikey answer, but at least it's more obviously wrong, so users may notice with more likelihood.
  • The proposed behavior is more useful / usable, but people who don't know the implementation here might incorrectly assume that nothing is off at all.

I don't know what's worse. I'm leaning towards accepting it. @brian-brazil are there more considerations to this than my thoughts above?

@brian-brazil

This comment has been minimized.

Show comment
Hide comment
@brian-brazil

brian-brazil Apr 13, 2017

Member

The cases presented thus far are best cases. What's the worst case behaviour of this?

Member

brian-brazil commented Apr 13, 2017

The cases presented thus far are best cases. What's the worst case behaviour of this?

@beorn7

This comment has been minimized.

Show comment
Hide comment
@beorn7

beorn7 Apr 14, 2017

Member

I'm afraid I don't have time right now to dive deep enough into the topic to take part in the debate at Brian's standards. If nobody else makes a confident point for accepting this as is, the 1.6.0 release train might leave without it.

Member

beorn7 commented Apr 14, 2017

I'm afraid I don't have time right now to dive deep enough into the topic to take part in the debate at Brian's standards. If nobody else makes a confident point for accepting this as is, the 1.6.0 release train might leave without it.

@juliusv

This comment has been minimized.

Show comment
Hide comment
@juliusv

juliusv Apr 14, 2017

Member

One extreme case would for example be: the first bucket is already 1000 in a non-atomic scrape, all the others are still at an old value of 0, although their new value would be really high. Then all buckets would get the value of 1000 assigned and it would look like all requests fell into the first bucket, so you get an erroneously low 99th percentile.

Or the reverse, only the last bucket is already updated to 1000 and the others are still 0, although in reality there were more requests in those lower buckets. Then the quantile calculation will yield much larger values than it's supposed to.

But it's not clear to me that either of those is worse than the current behavior...

Member

juliusv commented Apr 14, 2017

One extreme case would for example be: the first bucket is already 1000 in a non-atomic scrape, all the others are still at an old value of 0, although their new value would be really high. Then all buckets would get the value of 1000 assigned and it would look like all requests fell into the first bucket, so you get an erroneously low 99th percentile.

Or the reverse, only the last bucket is already updated to 1000 and the others are still 0, although in reality there were more requests in those lower buckets. Then the quantile calculation will yield much larger values than it's supposed to.

But it's not clear to me that either of those is worse than the current behavior...

@juliusv

This comment has been minimized.

Show comment
Hide comment
@juliusv

juliusv Apr 14, 2017

Member

Actually the second case can already happen with the current implementation.

So all that can happen with the new behavior is that we can calculate way too low quantiles. But the current behavior just produces indeterministic gibberish.

Given the following reasons, I'm for including this in 1.6:

  • Two users really want this and are hurt by the current behavior, and one is already successfully running it in production.
  • The behavior seems better overall than the current behavior.
  • We don't tie ourselves to this decision forever, assuming that at some point we will have atomic scrapes and storage inserts, making this all moot.
Member

juliusv commented Apr 14, 2017

Actually the second case can already happen with the current implementation.

So all that can happen with the new behavior is that we can calculate way too low quantiles. But the current behavior just produces indeterministic gibberish.

Given the following reasons, I'm for including this in 1.6:

  • Two users really want this and are hurt by the current behavior, and one is already successfully running it in production.
  • The behavior seems better overall than the current behavior.
  • We don't tie ourselves to this decision forever, assuming that at some point we will have atomic scrapes and storage inserts, making this all moot.

@juliusv juliusv referenced this pull request Apr 14, 2017

Merged

Cut v1.6.0 #2619

@brian-brazil

This comment has been minimized.

Show comment
Hide comment
@brian-brazil

brian-brazil Apr 14, 2017

Member

Thinking on it, I don't think this is going to be really any worse.

Member

brian-brazil commented Apr 14, 2017

Thinking on it, I don't think this is going to be really any worse.

Show outdated Hide outdated promql/quantile.go
// * Recording rule evaluation of histogram_quantile, especially when rate()
// has been applied to the underlying bucket timeseries.
// * Evaluation of histogram_quantile computed over federated bucket
// timeseries, especially when rate() has been applied

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

Nit: End with a period.

@beorn7

beorn7 Apr 14, 2017

Member

Nit: End with a period.

Show outdated Hide outdated promql/quantile.go
// * Evaluation of histogram_quantile computed over federated bucket
// timeseries, especially when rate() has been applied
//
// This is because scraped data is not made available to RR evalution or

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

"...available to rule evaluation..." reads nicer. (RR is not really a commonly used abbreviation. Besides, alerting rules are suffering from the same problem.)

@beorn7

beorn7 Apr 14, 2017

Member

"...available to rule evaluation..." reads nicer. (RR is not really a commonly used abbreviation. Besides, alerting rules are suffering from the same problem.)

Show outdated Hide outdated promql/quantile.go
// timeseries, especially when rate() has been applied
//
// This is because scraped data is not made available to RR evalution or
// federation atomically, so some buckets are computed with data from the N

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

Spurious "N"?

@beorn7

beorn7 Apr 14, 2017

Member

Spurious "N"?

Show outdated Hide outdated promql/quantile.go
//
// Randomly interspersed partial sampling breaks that guarantee, and rate()
// exacerbates it. Specifically, suppose bucket le=1000 has a count of 10 from
// 4 samples but the bucket with le=2000 has a count of 7, from 3 samples. The

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

Inconsistent comma. Suggestion to remove it.

@beorn7

beorn7 Apr 14, 2017

Member

Inconsistent comma. Suggestion to remove it.

Show outdated Hide outdated promql/quantile.go
// rate() removes this divergence.)
//
// bucketQuantile depends on that monotonicity to do a binary search for the
// bucket with the qth percentile count, so breaking the monotonicity

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

qth percentile → φ-quantile (after lengthy discussions, we settled on this as the least misleading term).

@beorn7

beorn7 Apr 14, 2017

Member

qth percentile → φ-quantile (after lengthy discussions, we settled on this as the least misleading term).

Show outdated Hide outdated promql/quantile.go
// guarantee causes bucketQuantile() to return undefined (nonsense) results.
//
// As a somewhat hacky solution until the Prometheus project is ready to
// accept the changes required to make scrapes atomic, we calculate the

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

"read to accept" sounds like the way to atomic scrape is already clearly laid out or even implemented, just "the Prometheus project" is too stubborn to "accept" it.
Please just write "... until ingestion is atomic per scrape, we...". It's also shorter.

@beorn7

beorn7 Apr 14, 2017

Member

"read to accept" sounds like the way to atomic scrape is already clearly laid out or even implemented, just "the Prometheus project" is too stubborn to "accept" it.
Please just write "... until ingestion is atomic per scrape, we...". It's also shorter.

Show outdated Hide outdated promql/quantile.go
func ensureMonotonic(buckets buckets) {
max := buckets[0].count
for i := range buckets {

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

bucketsbuckets[1:]
No reason to touch buckets[0] again.

@beorn7

beorn7 Apr 14, 2017

Member

bucketsbuckets[1:]
No reason to touch buckets[0] again.

Show outdated Hide outdated promql/quantile.go
// As a somewhat hacky solution until the Prometheus project is ready to
// accept the changes required to make scrapes atomic, we calculate the
// "envelope" of the histogram buckets, essentially removing any decreases
// in the count between successive buckets.

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

We are not super strict with line length, but since this is such a large chunk of doc comments, could you format it to not exceed 80 characters per line so that it reads better in something like the diff view on Github?

@beorn7

beorn7 Apr 14, 2017

Member

We are not super strict with line length, but since this is such a large chunk of doc comments, could you format it to not exceed 80 characters per line so that it reads better in something like the diff view on Github?

Show outdated Hide outdated promql/quantile.go
buckets[i].count = max
} else if buckets[i].count > max {
max = buckets[i].count
}

This comment has been minimized.

@beorn7

beorn7 Apr 14, 2017

Member

To make it easier to read for humans (and faster to compute for machines), let's do the happy cases first? And perhaps switch is more idiomatic.

Something like

switch {
case buckets[i].count > max:
	max = buckets[i].count
case buckets[i].count < max:
	buckets[i].count = max
}
@beorn7

beorn7 Apr 14, 2017

Member

To make it easier to read for humans (and faster to compute for machines), let's do the happy cases first? And perhaps switch is more idiomatic.

Something like

switch {
case buckets[i].count > max:
	max = buckets[i].count
case buckets[i].count < max:
	buckets[i].count = max
}
@beorn7

I read @brian-brazil 's latest comment as "this is at least not making things worse". Since @juliusv is for merging it, and my look at it (and somewhat superficial analysis) makes me thing we should do this, let's get it into 1.6.

I have a few nits, though, see comments. ( @juliusv will be proud of me, I hope…)

jjneely added some commits Apr 14, 2017

ensureMonotonic: Use switch statement
Use switch statement rather than if/else for better readability.
Process the most frequent cases first.

Looks like all comments were addressed.

@juliusv

This comment has been minimized.

Show comment
Hide comment
@juliusv

juliusv Apr 14, 2017

Member

👍 Thanks!

Member

juliusv commented Apr 14, 2017

👍 Thanks!

@juliusv juliusv merged commit 896f951 into prometheus:master Apr 14, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment