query: PromQL resets function does not detect reset.

[bwplotka]

Just for awareness. I noticed PromQL resets https://prometheus.io/docs/prometheus/latest/querying/functions/#resets function has no right to work. (:

This bug was introduced ~18.05.2020 by http://github.com/thanos-io/thanos/pull/2548

This is because for deduplication and downsampling precision we adjust resets on storage level:

• Query:

  thanos/pkg/query/iter.go

  Line 188 in 49dad93

  sit = downsample.NewApplyCounterResetsIterator(its...)

• Downsampling:

  thanos/pkg/compact/downsample/downsample.go

  Line 528 in 47f9a22

  it := NewApplyCounterResetsIterator(acs...)

Since it's used in downsampling it irreversibly blocked the reset function against such downsampled data.

First of all, I wonder... Why someone would use such low level detail like when counter resets and how many were happening? Maybe to detect restarts? 🤔

Still, I think we should fix it, help wanted 🤗 I noticed this when adding those tests #3631

[bwplotka]

Some useful disscussion on IRC:

bbrazil
resets is intended largely for debugging broken counters, however some people have found uses for it with gauges too
bwplotka
per: https://github.com/thanos-io/thanos/issues/3644
bbrazil
as resets will catch any time a series decreased, so can catch the down of a flap
bwplotka
yea ok
thanks for info (:
bbrazil
I'm having difficultly imaginging a good use case for using it on downsampled data, but that doesn't mean one doesn't exist
probably use the average or last rather than the counter. and it should be okay
bwplotka
what do you mean? For "resets" results? Interesting
bbrazil
basically resets is a gauge function, not a counter function

[bwplotka]

# Testdata for resets() and changes().
load 5m
	http_requests{path="/foo"}	1 2 3 0 1 0 0 1 2 0
	http_requests{path="/bar"}	1 2 3 4 5 1 2 3 4 5
	http_requests{path="/biz"}	0 0 0 0 0 1 1 1 1 1

# Tests for resets().
eval instant at 50m resets(http_requests[5m])
	{path="/foo"} 0
	{path="/bar"} 0
	{path="/biz"} 0

eval instant at 50m resets(http_requests[20m])
	{path="/foo"} 1
	{path="/bar"} 0
	{path="/biz"} 0

eval instant at 50m resets(http_requests[30m])
	{path="/foo"} 2
	{path="/bar"} 1
	{path="/biz"} 0

eval instant at 50m resets(http_requests[50m])
	{path="/foo"} 3
	{path="/bar"} 1
	{path="/biz"} 0

eval instant at 50m resets(nonexistent_metric[50m])

Tests that detect those.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Closing for now as promised, let us know if you need this to be reopened! 🤗

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[ee-github]

I would like to make a case here for the usefulness of fixing this bug, further and similar to the use-cases above mentioned by Brian Brazil.

When doing blackbox style monitoring of monotonically increasing functions, resets() is very valuable in detecting issues.

For example, the uptime counter of a network routing protocol like BGP. It ticks up until the session resets, at which point it restarts from zero. The resets() function catches that very well.

Just monitoring the binary/bool "up" status of said BGP peer is not sufficient because it can bounce quickly in-between scrape intervals and go undetected. However, such quick events cannot escape resets() of an uptime counter. Very valuable.

Please do fix! Beer 🍺 on me for the contributor!