Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers #6408

aknuds1 · 2023-10-17T16:50:29Z

What this PR does

In Ingester.QueryStream, if experimental flag -ingester.chunks-query-ignore-cancellation is enabled, ignore context cancellation wrt. chunk queriers, the way it worked prior to #6085. If the context is canceled though (or the error returned from Prometheus stems from a context error), emit a debug log message/trace span with full diagnostics for troubleshooting.

This is meant as a temporary "feature", while we troubleshoot unexpected Ingester.QueryStream cancellations.

I attempted adding a test, but realized that if I call Ingester.QueryStream with a canceled stream context, the method fails independently of the querier since responding via the stream will fail. Therefore writing a test is really tricky, and would take some time (would need some sort of stubbing most likely).

Which issue(s) this PR fixes or relates to

Checklist

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

pkg/ingester/ingester.go

treid314

I think this is fine as a test just to allow us to see that original error, but wouldn't the underlying query still fail?

aknuds1 · 2023-10-18T10:17:55Z

I think this is fine as a test just to allow us to see that original error, but wouldn't the underlying query still fail?

@treid314 you're right, I actually realized the same thing last night. The drawback is that we don't get at the original error, unless it's from the stream context, but we don't want to fail the query either. I will revert to passing an independent context (context.Background()) instead. I will also put a flag around this behaviour, and make sure there are tests.

In Ingester.QueryStream, ignore context cancellation wrt. chunk queriers, the way it worked prior to #6085. If the context is canceled though, log it with full diagnostics. Signed-off-by: Arve Knudsen <[email protected]>

Signed-off-by: Arve Knudsen <[email protected]>

pracucci · 2023-10-18T10:50:31Z

pkg/ingester/ingester.go

@@ -1625,6 +1627,8 @@ func (i *Ingester) QueryStream(req *client.QueryRequest, stream client.Ingester_
 	spanlog, ctx := spanlogger.NewWithLogger(stream.Context(), i.logger, "Ingester.QueryStream")
 	defer spanlog.Finish()

+	deadline, deadlineSet := ctx.Deadline()


[nit] I would move it before the call to dumpContextError()

I moved it into dumpContextError since I don't see any downside to doing so (and the code gets simpler).

pracucci · 2023-10-18T10:53:00Z

pkg/ingester/ingester.go

+	} else {
+		timeout = "not set"
+	}
+	level.Warn(spanlog).Log("msg", "query context error", "cause", context.Cause(ctx), "timeout", timeout,


The querier legitimately cancel the context once it reaches the quorum (e.g. send request to all ingester zones, cancel outstanding requests once reached successful answers from 2 out of 3 zones). I think this will generate a lot of noise in the logs. I suggest you to to put this debug info only in the tracing span (and not log it as well), if your purpose will be to investigate it through tracing anyway.

It's a bit difficult to say before we try in practice (I'm more used to logs than traces), but obtaining this info from traces is probably enough.

@pracucci I've changed to level.Debug instead, since that what happens elsewhere in Ingester.QueryStream, and it should cause only a trace to be emitted unless debug logs are enabled. That should be sufficient, or would it be too noisy even for debug logs?

pracucci · 2023-10-18T10:54:27Z

pkg/ingester/ingester.go

@@ -2006,7 +2030,7 @@ func (i *Ingester) sendStreamingQuerySeries(ctx context.Context, q storage.Chunk

 	// Ensure no error occurred while iterating the series set.
 	if err := ss.Err(); err != nil {
-		return nil, 0, err
+		return nil, 0, errors.Wrap(err, "iterating ChunkSeriesSet")


[nit] I think calling it "iterating" is a bit misleading. I understand this is coming from the comment above, but in practice what's happening here is that we're "sending" not "iterating".

Same comment applies above.

I wasn't actually looking at the comment, I was just relating to the fact that ss.Err() returns any error having occurred during iteration of the ss iterator. I don't understand how "sending" would be more descriptive, since any error from ss.Err() would be from failed ChunkSeries iteration, not sending. A failed send should result in an error returned from client.SendQueryStream, no?

pkg/ingester/ingester.go

…re-ctx

Signed-off-by: Arve Knudsen <[email protected]>

…re-ctx

Signed-off-by: Arve Knudsen <[email protected]>

…re-ctx

Signed-off-by: Arve Knudsen <[email protected]>

CHANGELOG.md

pkg/ingester/ingester.go

…re-ctx

Signed-off-by: Arve Knudsen <[email protected]>

dimitarvdimitrov

LGTM with the understanding that this will be removed as soon as we understand the context cancelations better

…or chunk queriers (#6408) * Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers In Ingester.QueryStream, support ignoring of context cancellation wrt. chunk queriers, the way it worked prior to #6085. If the context is canceled though, (span) log it with full diagnostics. Also wrap some chunk querying errors. --------- Signed-off-by: Arve Knudsen <[email protected]> (cherry picked from commit f737a96)

…or chunk queriers (#6408) (#6440) * Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers In Ingester.QueryStream, support ignoring of context cancellation wrt. chunk queriers, the way it worked prior to #6085. If the context is canceled though, (span) log it with full diagnostics. Also wrap some chunk querying errors. --------- Signed-off-by: Arve Knudsen <[email protected]> (cherry picked from commit f737a96) Co-authored-by: Arve Knudsen <[email protected]>

dimitarvdimitrov · 2023-10-26T09:57:51Z

since we found the root cause to be what grafana/mimir-prometheus#546 fixed, can we revert this PR @aknuds1 ?

aknuds1 · 2023-10-27T06:23:43Z

@dimitarvdimitrov I was planning on reverting this next week (have a reminder set), after we've tested that fix for a bit. Sound good?

…lation for chunk queriers (#6408)" This reverts commit f737a96. Signed-off-by: Arve Knudsen <[email protected]>

…lation for chunk queriers (#6408)" (#6555) This reverts commit f737a96. Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 added bug Something isn't working component/ingester labels Oct 17, 2023

aknuds1 changed the title ~~Ingester.QueryStream: Ignore context cancellation for chunk queriers~~ WIP: Ingester.QueryStream: Ignore context cancellation for chunk queriers Oct 17, 2023

aknuds1 changed the title ~~WIP: Ingester.QueryStream: Ignore context cancellation for chunk queriers~~ Ingester.QueryStream: Ignore context cancellation for chunk queriers Oct 17, 2023

aknuds1 force-pushed the arve/chunk-query-ignore-ctx branch 2 times, most recently from fb959b4 to 734d491 Compare October 17, 2023 17:23

leizor reviewed Oct 17, 2023

View reviewed changes

pkg/ingester/ingester.go Outdated Show resolved Hide resolved

treid314 approved these changes Oct 17, 2023

View reviewed changes

aknuds1 added 2 commits October 18, 2023 12:20

Ingester.QueryStream: Pass independent context for chunk queriers

2cffe7a

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 force-pushed the arve/chunk-query-ignore-ctx branch from 734d491 to 2cffe7a Compare October 18, 2023 10:28

pracucci reviewed Oct 18, 2023

View reviewed changes

aknuds1 added 5 commits October 18, 2023 14:07

Merge remote-tracking branch 'origin/main' into arve/chunk-query-igno…

48e51b3

…re-ctx

Let dumpContextError figure out timeout

dee03b6

Signed-off-by: Arve Knudsen <[email protected]>

Improve error messages

c53f8be

Signed-off-by: Arve Knudsen <[email protected]>

Merge remote-tracking branch 'origin/main' into arve/chunk-query-igno…

04fc5d4

…re-ctx

dumpContextError: Log with debug level instead of warn

1885ca5

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 force-pushed the arve/chunk-query-ignore-ctx branch from 100c39e to 1885ca5 Compare October 18, 2023 16:17

aknuds1 added 4 commits October 19, 2023 11:26

Merge remote-tracking branch 'origin/main' into arve/chunk-query-igno…

173acb0

…re-ctx

Put functionality behind flag

6b1323e

Signed-off-by: Arve Knudsen <[email protected]>

Add changelog entry

27644b0

Signed-off-by: Arve Knudsen <[email protected]>

Fix comparison

4b8e651

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 marked this pull request as ready for review October 19, 2023 09:43

aknuds1 requested review from a team as code owners October 19, 2023 09:43

aknuds1 requested review from dimitarvdimitrov and pracucci October 19, 2023 09:44

dimitarvdimitrov reviewed Oct 19, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/ingester/ingester.go Outdated Show resolved Hide resolved

pkg/ingester/ingester.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into arve/chunk-query-igno…

6d9b694

…re-ctx

aknuds1 added 3 commits October 19, 2023 12:40

Use context.WithoutCancel

c942adf

Signed-off-by: Arve Knudsen <[email protected]>

Remove experimental from help text

76a30ea

Signed-off-by: Arve Knudsen <[email protected]>

Document feature better

96557ef

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 requested a review from dimitarvdimitrov October 19, 2023 11:27

aknuds1 added enhancement New feature or request and removed bug Something isn't working labels Oct 19, 2023

dimitarvdimitrov approved these changes Oct 19, 2023

View reviewed changes

aknuds1 changed the title ~~Ingester.QueryStream: Ignore context cancellation for chunk queriers~~ Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers Oct 19, 2023

aknuds1 merged commit f737a96 into main Oct 19, 2023
30 checks passed

aknuds1 deleted the arve/chunk-query-ignore-ctx branch October 19, 2023 12:09

aknuds1 added the backport r258 label Oct 19, 2023

grafanabot mentioned this pull request Oct 19, 2023

[r258] Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers #6440

Merged

aknuds1 added a commit that referenced this pull request Nov 3, 2023

Revert "Ingester.QueryStream: Add support for ignoring context cancel…

b11dde4

…lation for chunk queriers (#6408)" This reverts commit f737a96. Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 mentioned this pull request Nov 3, 2023

Revert "Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers (#6408)" #6555

Merged

2 tasks

aknuds1 added a commit that referenced this pull request Nov 3, 2023

Revert "Ingester.QueryStream: Add support for ignoring context cancel…

5db00b2

…lation for chunk queriers (#6408)" (#6555) This reverts commit f737a96. Signed-off-by: Arve Knudsen <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers #6408

Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers #6408

aknuds1 commented Oct 17, 2023 •

edited

Loading

treid314 left a comment

aknuds1 commented Oct 18, 2023 •

edited

Loading

pracucci Oct 18, 2023

aknuds1 Oct 18, 2023

pracucci Oct 18, 2023

aknuds1 Oct 18, 2023

aknuds1 Oct 18, 2023 •

edited

Loading

pracucci Oct 18, 2023

aknuds1 Oct 18, 2023

dimitarvdimitrov left a comment

dimitarvdimitrov commented Oct 26, 2023

aknuds1 commented Oct 27, 2023 •

edited

Loading

Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers #6408

Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers #6408

Conversation

aknuds1 commented Oct 17, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

treid314 left a comment

Choose a reason for hiding this comment

aknuds1 commented Oct 18, 2023 • edited Loading

pracucci Oct 18, 2023

Choose a reason for hiding this comment

aknuds1 Oct 18, 2023

Choose a reason for hiding this comment

pracucci Oct 18, 2023

Choose a reason for hiding this comment

aknuds1 Oct 18, 2023

Choose a reason for hiding this comment

aknuds1 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

pracucci Oct 18, 2023

Choose a reason for hiding this comment

aknuds1 Oct 18, 2023

Choose a reason for hiding this comment

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

dimitarvdimitrov commented Oct 26, 2023

aknuds1 commented Oct 27, 2023 • edited Loading

aknuds1 commented Oct 17, 2023 •

edited

Loading

aknuds1 commented Oct 18, 2023 •

edited

Loading

aknuds1 Oct 18, 2023 •

edited

Loading

aknuds1 commented Oct 27, 2023 •

edited

Loading