Check for errors when iterating through chunks or samples in response to a query

[charleskorn]

What this PR does

This PR fixes an issue where ingesters could ignore errors encountered while iterating through chunks or samples in response to a query request.

Note to reviewers: I haven't added a test for this case as introducing an error does not seem straightforward with how we test the ingester query methods. I'm open to feedback or suggestions on how to do this though.

Which issue(s) this PR fixes or relates to

(none)

Checklist

• [n/a] Tests updated
• [n/a] Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[aknuds1]

The fix looks correct to me, but I'd also like to see tests. Not sure off the top of my head how to implement tests though, I'll try to see if I can figure out how.

[dimitarvdimitrov]

Thanks for the fix. I agree that it's hard to test this. I think the direct usage of *userTSDB makes it difficult to inject behaviour.

What if we cancelled the context of the Query operation? Would that be susceptible to a race condition? It's a bit of a contrived test, but it might help uncover this bug.

[charleskorn]

What if we cancelled the context of the Query operation? Would that be susceptible to a race condition? It's a bit of a contrived test, but it might help uncover this bug.

I think that wouldn't guarantee we're exercising the changes in this PR - there are a number of different places that might respond to the context cancellation depending on when it occurs.

[charleskorn]

Given the approval from @dimitarvdimitrov (and offline discussion with @aknuds1), I'm going to set this to merge once CI is passing - but if anyone has any post-merge ideas on how to test this (or any other feedback), I'd love to hear them.

[dimitarvdimitrov]

What if we cancelled the context of the Query operation? Would that be susceptible to a race condition? It's a bit of a contrived test, but it might help uncover this bug.

I think that wouldn't guarantee we're exercising the changes in this PR - there are a number of different places that might respond to the context cancellation depending on when it occurs.

my approach would be to have a for loop in the test and run it enough times to reliably trigger the bug at least once during one go test. Not the best approach and it's fine by me if you choose not to add tests for this change

[charleskorn]

my approach would be to have a for loop in the test and run it enough times to reliably trigger the bug at least once during one go test. Not the best approach and it's fine by me if you choose not to add tests for this change

https://github.com/grafana/mimir/compare/charleskorn/test-for-6451 is the most reliable test I've been able to find. I don't love it: it's tied very closely to the current structure of the Ingester type, and I'd need to refactor the other two code paths (samples and non-streaming chunks) to be able to do something similar for them. I don't think it's worth it, but open to other opinions or ideas.

[pstibrany]

Reverted in #6576.