Skip to content
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

fix: Fix panic on requesting out-of-order Pattern samples #13010

Merged
merged 2 commits into from
May 28, 2024

Conversation

benclive
Copy link
Contributor

What this PR does / why we need it:
This PR fixes a panic that can occur when samples are out-of-order in the Pattern chunks.
This happens because the calculated slice length can be negative if the timestamp at hi-1 is less than the timestamp at lo due to subtracting currentStep (generated from timestamp@lo) from the timestamp@hi-1.

	currentStep := truncateTimestamp(c.Samples[lo].Timestamp, step)
	aggregatedSamples := make([]logproto.PatternSample, 0, ((c.Samples[hi-1].Timestamp-currentStep)/step)+1)

Implementation Notes

  • I opted to return nil if the timestamps are out-of-order which may be incorrect behaviour (but does solve the panic).
  • The samples should never be out of order now since I added a sort to the Add method.
  • An alternative is to simply reject samples that are older than the latest during Add - let me know if you prefer this approach.

@benclive benclive requested a review from a team as a code owner May 22, 2024 10:52
@@ -51,7 +51,7 @@ func (c Chunk) ForRange(start, end, step model.Time) []logproto.PatternSample {
}
first := c.Samples[0].Timestamp
last := c.Samples[len(c.Samples)-1].Timestamp
if start >= end || first >= end || last < start {
if last < first || start >= end || first >= end || last < start {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might not cover all the cases since the in-between samples could be out-of-order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - I'll add some more test cases

@pull-request-size pull-request-size bot added size/M and removed size/S labels May 22, 2024
@benclive
Copy link
Contributor Author

Rewrote this so it will reject out-of-order samples for the Pattern instead of trying to sort this chunk.
Firstly, this makes the sample generation code much simpler. Secondly, this is a fairly rare case and since Patterns don't need to be 100% exact, its OK to simply drop these values.

Copy link
Contributor

@ashwanthgoli ashwanthgoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cyriltovena cyriltovena merged commit 2171f64 into main May 28, 2024
60 checks passed
@cyriltovena cyriltovena deleted the fix-panic-on-out-of-order-samples branch May 28, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants