Handle warnings from remote engines #6619
Conversation
The remote engine implementation currently converts warnings to errors. This prevents using partial response with the distributed engine since every warning will cause a query to fail. Signed-off-by: Filip Petkovski <[email protected]>
| @@ -245,7 +250,8 @@ func (r *remoteQuery) Exec(ctx context.Context) *promql.Result { | |||
| } | |||
|
|
|||
| if warn := msg.GetWarnings(); warn != "" { | |||
| return &promql.Result{Err: errors.New(warn)} | |||
| warnings = append(warnings, errors.New(warn)) | |||
| continue | |||
There was a problem hiding this comment.
Actually I am wondering if things need to change since remote engine (can be any implementation) might return both warning and results the same time? Can we safely ignore the results?
There was a problem hiding this comment.
Agreed, it seems like either way we'd skip to the next iteration couple lines below if there are no series?
There was a problem hiding this comment.
I thought we can only have either a warning or a series in a response, at least that's how I understand the oneOf clause in the proto:
thanos/pkg/api/query/querypb/query.proto
Lines 57 to 65 in 221881b
There was a problem hiding this comment.
You're right, I didn't realize it's oneof field, then this makes sense, thanks 👍
There was a problem hiding this comment.
This is the proto def in Thanos, right? It is defined by us.
For the Prometheus query api itself, it can return both warning and results the same time?
I wonder if this oneof makes sense or not. At least from what I understand Prometheus query api can have both fields set in the response
There was a problem hiding this comment.
Yes, the remote engine talks to our grpc API, and that one will send warnings in separate messages even if the result contains series data: https://github.com/thanos-io/thanos/blob/main/pkg/api/query/grpc.go#L131-L135.
We don't have a remote engine to query HTTP endpoints yet. If we decide to add it, we would need to handle cases where warnings and series are sent in the same response.
The remote engine implementation currently converts warnings to errors. This prevents using partial response with the distributed engine since every warning will cause a query to fail. Signed-off-by: Filip Petkovski <[email protected]>
The remote engine implementation currently converts warnings to errors. This prevents using partial response with the distributed engine since every warning will cause a query to fail. Signed-off-by: Filip Petkovski <[email protected]>
The remote engine implementation currently converts warnings to errors. This prevents using partial response with the distributed engine since every warning will cause a query to fail.
Changes
Verification