Summary DataPoint SlidingWindow only uses 'current' slice, resulting in periodic NaN values

[yrn1]

In io.prometheus.metrics.core.metrics.SlidingWindow, a ringBuffer of T is maintained. In case of Summary, T is CKMSQuantiles. But this ringBuffer is never actually fully used: only the 'current' element of the buffer is ever used (all other methods are private):

public synchronized T current() {
    return rotate();
}

public synchronized void observe(double value) {
    observeFunction.accept(rotate(), value);
}

E.g. in Summary.DataPoint#makeQuantiles(), only quantileValues.current() is used:

for (int i = 0; i < getQuantiles().size(); i++) {
    CKMSQuantiles.Quantile quantile = getQuantiles().get(i);
    quantiles[i] = new Quantile(quantile.quantile, quantileValues.current().get(quantile.quantile));
}

When this current slice happens to be just rotated, then DataPoint.makeQuantiles() will result in a NaN value.
Isn't the purpose of having a SlidingWindow to the full window when calculating a value, instead of only using the current slice?

[fstab]

Good catch, thanks for the bug report. I'll fix it asap, hoping to find the time tomorrow.

[fstab]

I think the observe() function should be like this

    public synchronized void observe(double value) {
        rotate();
        for (T t : ringBuffer) {
            observeFunction.accept(t, value);
        } 

but I want to write a unit test first, because there should definitely be a test.

[yrn1]

I think observe is correct as it is: you want to put some observed data in the current ringbuffer bucket. Isn't it at scrape time that you want to take all those buckets into account? I.e. in DataPoint#makeQuantiles(), you want to actually use the full sliding window (so all of the ringBuffer inside it)? I mean, why does the SlidingWindow have a ringBuffer: so you can reset only a small part of the SlidingWindow (i.e. one ringBuffer entry), without touching the rest of the data in it, right? But when you want to say something about the data in the SlidingWindow (like when prometheus scrapes), you want it to be about all data in the SlidingWindow, not just a single ringBuffer entry, right? Or am I missing something vital here?

[fstab]

Here's an explanation: Imagine you have maxAge of 5 minutes. Then you expect all observations to be present for 5 minutes. So if you query the current bucket, you want to get all observations between now and 5 minutes ago.

For example, if you rotate every minute, then you have:

• The current bucket represents all observations within the last 5 minutes, i.e. all observations between now and 5 minutes ago.
• The current+1 bucket has all observations within the last 4 minutes. After the next rotate it will become the current bucket and contain all observations within the last 5 minutes.
• The current+2 bucket has all observations within the last 3 minutes. After 2 rotates it will become the current bucket and contain all observations within the last 5 minutes.
• ...

So you have to observe values in all buckets.

Makes sense?

#896 has a fix.

[fstab]

Ah, I read your comment again and understand what you mean. Yes, the solution you are describing would work, but only if you can aggregate all buckets at scrape time. Aggregating Summaries is not possible, that's why it can't be implemented that way. We need to aggregate at observe time, not at scrape time.

[yrn1]

Ah, yes, got it. Thanks!