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.

Sign up for GitHub

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

Jump to bottom

Query-scheduler worker: don't treat cancel as an error #6767

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Query-scheduler worker: don't treat cancel as an error #6767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
* [FEATURE] Distributor: add an experimental flag `-distributor.reusable-ingester-push-worker` that can be used to pre-allocate a pool of workers to be used to send push requests to the ingesters. #6660
* [FEATURE] Distributor: Support enabling of automatically generated name suffixes for metrics ingested via OTLP, through the flag `-distributor.otel-metric-suffixes-enabled`. #6542
* [FEATURE] Ingester: ingester can now track which of the user's series the ingester actually owns according to the ring, and only consider owned series when checking for user series limit. This helps to avoid hitting the user's series limit when scaling up ingesters or changing user's ingester shard size. Feature is currently experimental, and disabled by default. It can be enabled by setting `-ingester.use-ingester-owned-series-for-limits` (to use owned series for limiting). This is currently limited to multi-zone ingester setup, with replication factor being equal to number of zones. #6718
* [ENHANCEMENT] Query-frontend: don't treat cancel as an error. #4648
* [ENHANCEMENT] Query-frontend: don't treat cancel as an error. #4648 #6767
* [ENHANCEMENT] Ingester: exported summary `cortex_ingester_inflight_push_requests_summary` tracking total number of inflight requests in percentile buckets. #5845
* [ENHANCEMENT] Query-scheduler: add `cortex_query_scheduler_enqueue_duration_seconds` metric that records the time taken to enqueue or reject a query request. #5879
* [ENHANCEMENT] Query-frontend: add `cortex_query_frontend_enqueue_duration_seconds` metric that records the time taken to enqueue or reject a query request. When query-scheduler is in use, the metric has the `scheduler_address` label to differentiate the enqueue duration by query-scheduler backend. #5879 #6087 #6120
Expand Down
14 changes: 8 additions & 6 deletions pkg/querier/worker/scheduler_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,15 @@ func (sp *schedulerProcessor) processQueriesOnSingleStream(workerCtx context.Con
}

if err := sp.querierLoop(execCtx, c, address, inflightQuery); err != nil {
// Do not log an error if the query-scheduler is shutting down.
if s, ok := status.FromError(err); !ok || !strings.Contains(s.Message(), schedulerpb.ErrSchedulerIsNotRunning.Error()) {
level.Error(sp.log).Log("msg", "error processing requests from scheduler", "err", err, "addr", address)
}
if !grpcutil.IsCanceled(err) {
// Do not log an error if the query-scheduler is shutting down.
if s, ok := status.FromError(err); !ok || !strings.Contains(s.Message(), schedulerpb.ErrSchedulerIsNotRunning.Error()) {
level.Error(sp.log).Log("msg", "error processing requests from scheduler", "err", err, "addr", address)
}

backoff.Wait()
continue
backoff.Wait()
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the intent of this change. However, we'll now not log anything if the query is cancelled because the scheduler told the querier to cancel the query, which might make debugging issues harder.

Perhaps we should add an info-level log statement around line 177 below if the query is explicitly cancelled by the scheduler? Even better would be to cancel the context created on line 146 with an error like "query cancelled by scheduler" and then make sure this is logged somewhere or attached to the query trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this happen if the user's client has a timeout shorter than Mimir takes?
This is the case I don't want to log for.
It should be visible as an event in the trace.

Copy link
Contributor

Choose a reason for hiding this comment

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

The querier won't know what caused the cancellation - it could be that the user cancelled the query, or it could be that something in the chain between the user and the querier timed out and cancelled the query, for example.

Copy link
Contributor Author

@bboreham bboreham Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Still sounds like I don't want to see the log here.
The component taking a decision about canceling can log it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's value in adding something to the querier's trace span to say "received cancellation from scheduler" - we've had a number of context cancellation-related issues in this part of the codebase lately, and being very explicit like this would have been helpful in debugging the issues.

}

backoff.Reset()
Expand Down
Loading