Loki: Fix memchunk headblock filter #8591
Conversation
DylanGuedes
changed the title
| @@ -1005,7 +1005,7 @@ func (hb *headBlock) Iterator(ctx context.Context, direction logproto.Direction, | |||
| baseHash := pipeline.BaseLabels().Hash() | |||
| process := func(e entry) { | |||
| // apply time filtering | |||
| if e.t < mint || e.t >= maxt { | |||
| if e.t < mint || e.t > maxt { | |||
There was a problem hiding this comment.
I think in all cases timestamp filtering Loki is inclusive of the start time and exclusive of the end time.
I'm not 100% sure what's driving this change but I think it is likely correct as is?
There was a problem hiding this comment.
Lol, sorry should have slowed down a little, it looks like the change you are making would follow the convention I just outlined.
Is there more context for this change though?
There was a problem hiding this comment.
context:
- we're releasing a new version that updates Go version to Go 1.20
- go 1.20 linter complained that this part of the code was shadowing the
ttvariable - once I added the
tt := ttassignment the tests started to break. Removing the parallel flag also exposed the problem
There was a problem hiding this comment.
all that said: this isn't a major problem, I talked with Owen and it is a nanosecond precision, so not really important. however, it is blocking the new Loki version.
There was a problem hiding this comment.
Looking at this again, I think I stand by my initial feedback, I think this change breaks important behavior. (it's actually testing if the timestamp is outside the bounds)
There was a problem hiding this comment.
ts == maxt is technically outside of the bounds since end time is not inclusive.
There was a problem hiding this comment.
right but what if maxt == mint? ex: maxt=1, mint1= and you have an entry with ts=1.
since start time is inclusive I'd expect it to have precedence over end time not being inclusive.
also, that test that I removed the Parallel call is supposed to fail; it is only passing because of the variable shadowing in conjunction with the parallel call.
There was a problem hiding this comment.
I think when maxt == mint (such as an instant query), we mutate the endts = endts+1nanosecond prior to running the query
Reverts #8591 This altered an internal API. We execute queries that are inclusive of start and exclusive of end: `[start,end)` and shouldn't break this. We use other ways to ensure `start==end` still works, such as by using the lookback-period to alter the range of queries: https://github.com/grafana/loki/blob/main/pkg/logql/evaluator.go#L179-L181
**What this PR does / why we need it**: - Use the 0.28.1 build image - Update Go version to 1.20.1 - Use Alpine 3.16.4 - Fix linter issues due to updated govet in 1.20.1 - Modify images to not create `/etc/nsswitch.conf` file anymore (available by default on Alpine 3.16.4) - Remove impossible test cases for `TestMemChunk_IteratorBounds`(see #8591 (comment) for context) Signed-off-by: Michel Hollands <[email protected]> Signed-off-by: Christian Haudum <[email protected]> Co-authored-by: Christian Haudum <[email protected]> Co-authored-by: DylanGuedes <[email protected]>
**What this PR does / why we need it**: - Use the 0.28.1 build image - Update Go version to 1.20.1 - Use Alpine 3.16.4 - Fix linter issues due to updated govet in 1.20.1 - Modify images to not create `/etc/nsswitch.conf` file anymore (available by default on Alpine 3.16.4) - Remove impossible test cases for `TestMemChunk_IteratorBounds`(see #8591 (comment) for context) Signed-off-by: Michel Hollands <[email protected]> Signed-off-by: Christian Haudum <[email protected]> Co-authored-by: Christian Haudum <[email protected]> Co-authored-by: DylanGuedes <[email protected]> (cherry picked from commit 4f8d324)
**What this PR does / why we need it**: - Use the 0.28.1 build image - Update Go version to 1.20.1 - Use Alpine 3.16.4 - Fix linter issues due to updated govet in 1.20.1 - Modify images to not create `/etc/nsswitch.conf` file anymore (available by default on Alpine 3.16.4) - Remove impossible test cases for `TestMemChunk_IteratorBounds`(see grafana#8591 (comment) for context) Signed-off-by: Michel Hollands <[email protected]> Signed-off-by: Christian Haudum <[email protected]> Co-authored-by: Christian Haudum <[email protected]> Co-authored-by: DylanGuedes <[email protected]>
What this PR does / why we need it:
Fix our memchunk headblock filter to not skip entries that have timestamp=maxT.