Fix issue where successful streaming gRPC requests are incorrectly reported as cancelled

[charleskorn]

What this PR does

This PR fixes multiple related issues:

• It fixes the issue where query requests that stream chunks but return no series are reported as cancelled rather than successful in traces and metrics reported by queriers (77c191d)
• It also fixes a similar issue as above for other server streaming gRPC calls (ae15605)
• It fixes the issue where query requests that stream chunks and have some series are reported as cancelled (5f6d927 for streaming from ingesters, 91fd892 for streaming from store-gateways)

4453e0a separately refactors the ingester stream reader (SeriesChunksStreamReader) to follow the same structure as the store-gateway stream reader (storeGatewayStreamReader).

I'd suggest reviewing each commit individually.

Which issue(s) this PR fixes or relates to

(none)

Checklist

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

[charleskorn]

Putting back to draft to investigate flaky integration test introduced in this PR

[colega]

I'll trust the test. The "CloseAndExhaust" also looks good.

LGTM.

[pracucci]

The change makes sense to me. I'm just wondering if by draining the stream we may add extra latency, but on the other side the gRPC doc you linked is very clear that if we don't do it resources may be leaked.

Will wait for you to spot the issue causing flaky integration test (whether it's just an issue in the test or a more serious one).

[charleskorn]

I'm just wondering if by draining the stream we may add extra latency, but on the other side the gRPC doc you linked is very clear that if we don't do it resources may be leaked.

By the time we get to draining the stream, we've either:

• successfully reached the end of the stream, in which case draining the stream would effectively be a no-op
• encountered some kind of error has occurred that the gRPC library already knows about (eg. timeout or error returned by the server), in which case we've also reached the end of the stream and draining should again be a no-op
• encountered some kind of error on the client side and decided to abort the request, in which case we might have to do some work to drain the stream

Only in the last case would I expect any noticeable latency difference, and this should be fairly quick - worst case scenario, the client code closes the stream, the gRPC library sends a cancellation notification to the server and then the client has to wait for the server to acknowledge the cancellation and close the stream, or times out.

[charleskorn]

I've fixed the flaky test, which was due to flaky behaviour while streaming chunks - there was a race between:

• the gRPC stream being closed cleanly and thus the request being reported as a successful request
• the context used by the PromQL engine being cancelled once query evaluation had succeeded and thus the request being reported as cancelled

[pracucci]

Very good work. The changes make sense to me. I left a couple of nits that I'm going to self-commit so we merge this PR today and will be included in the next week release.

For a follow up PR: I would add a failling rule to forbid the usage of CloseSend() but use CloseAndExhaust() instead.

[pracucci]

Merging it to get it included in tomorrow's weekly release.

[charleskorn]

I'm just wondering if by draining the stream we may add extra latency, but on the other side the gRPC doc you linked is very clear that if we don't do it resources may be leaked.

By the time we get to draining the stream, we've either:

• ...
• encountered some kind of error on the client side and decided to abort the request, in which case we might have to do some work to drain the stream

Only in the last case would I expect any noticeable latency difference, and this should be fairly quick - worst case scenario, the client code closes the stream, the gRPC library sends a cancellation notification to the server and then the client has to wait for the server to acknowledge the cancellation and close the stream, or times out.

I need to correct myself here: when the client closes the stream (ie. calls CloseSend()), this does not necessarily mean the server will promptly terminate the stream: it's valid for the server to continue sending messages (and the client to continue receiving them) after this point. It would therefore be possible for a client to hang waiting for messages that may never come in CloseAndExhaust().

I've checked all of the places that are now using CloseAndExhaust() and don't believe any of them will behave poorly in this case, but it may be wasteful for the server to continue processing the request after the client has abandoned it. Before CloseAndExhaust() was introduced, most callers would have cancelled the context used to create the stream after calling CloseSend(), so that would have aborted the request and communicated the cancellation to the server.

The main scenario I'm concerned about is store-gateway query requests that are abandoned due to hitting a query limit, which I will address in #6505. (Ingester query requests are not susceptible to the same behaviour as these use DoUntilQuorum, and DoUntilQuorum will cancel all request contexts once one returns an error.)