-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Preserve reset hint during histogram deduplication #7092
base: main
Are you sure you want to change the base?
Conversation
f8cf2fe
to
6cd1b77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add a test case? 🤔
func newHistogramResetDetector(iterator chunkenc.Iterator) *histogramResetDetector { | ||
return &histogramResetDetector{ | ||
Iterator: iterator, | ||
i: -1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
6491ab9
to
759ce0c
Compare
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment is not valid anymore, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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.