Preserve reset hint during histogram deduplication #7092
Conversation
fpetkovski
force-pushed
the
dedup-histogram-reset
branch
4 times, most recently
from
f8cf2fe
to
6cd1b77
Compare
| func newHistogramResetDetector(iterator chunkenc.Iterator) *histogramResetDetector { | ||
| return &histogramResetDetector{ | ||
| Iterator: iterator, | ||
| i: -1, |
There was a problem hiding this comment.
I see the only purpose of this is to check whether Next() has been called? Maybe we could convert it into a simple boolean? Otherwise this might overflow.
There was a problem hiding this comment.
The variable tracks whether we are at the first sample. We could use a boolean pointer because we need to represent 3 states, one being Next not being called at all. AFAIK a chunk has 120 samples so we should not have a risk of overflow, and even if it does the side-effect will be that we recalculate a reset one more time in the engine.
|
Will do, looks like the test got lost when I moved the struct to a new package. |
|
Any update on this? |
|
I'll get it done this week, got tangled up in something else. |
Penalty based deduplication can over-skip samples because when switching between replicas, because it uses a penalty of 2 times the estimated scrape interval. This works okay for float counters because the reset is detected based on the value of the counter. With native histograms, detecting a reset is much more expensive which is why there is a hint stored in the first sample of a chunk, which indicates whether the chunk has been created because of a reset. It can easily happen that the dedup iterator switches replicas, and skips the first sample of a chunk because of the added penalty. In this case we will not read the hint and will assume that no histogram reset happened. This commit fixes the issue by explicitly calling DetectReset if a replica stream has been switched. The result is then stored and set in the histogram returned by AtHistogram and AtFloatHistogram. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
fpetkovski
force-pushed
the
dedup-histogram-reset
branch
2 times, most recently
from
6491ab9
to
759ce0c
Compare
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
fpetkovski
force-pushed
the
dedup-histogram-reset
branch
from
759ce0c
to
7491896
Compare
| @@ -1033,7 +1036,8 @@ func (s *mockedSeriesIterator) At() (t int64, v float64) { | |||
|
|
|||
| // TODO(rabenhorst): Needs to be implemented for native histogram support. | |||
There was a problem hiding this comment.
the comment is not valid anymore, right?
Penalty based deduplication can over-skip samples when switching between replicas because it uses a penalty of 2 times the estimated scrape interval. This works okay for float counters because the reset is detected based on the value of the counter. With native histograms, detecting a reset is much more expensive which is why there is a hint stored in the first sample of a chunk, indicating whether the chunk has been created because of a reset.
It can easily happen that the dedup iterator switches replicas, and skips the first sample of a chunk because of the added penalty. In this case we will not read the hint and will assume that no histogram reset happened.
This commit fixes the issue by explicitly calling DetectReset if a replica stream has been switched. The result is then stored and set in the histogram returned by AtHistogram and AtFloatHistogram.
Changes
Fix counter reset detection during deduplication of native histograms.
Verification
I added a unit test, and also verified this against a case we saw in production.