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.

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

/active_series: generate correct request shards for incoming GET requests, handle gRPC errors #7133

Merged
merged 10 commits into from
Jan 17, 2024

Conversation

flxbk
Copy link
Contributor

@flxbk flxbk commented Jan 15, 2024

What this PR does

This PR addresses two issues I discovered while testing the active series API.

  • Shard requests generated for incoming active series requests with method GET and a selector parameter in the query string were broken and did not include the modified shard selector. Therefore, the results would always contain the set of series duplicated as many times as there are request shards.
  • Handling context.Canceled is not sufficient for grpc related calls, we also need to handle status.Code(err) == codes.Canceled. Following up on a review comment, I have moved and reused code from pkg/storegateway which already solves this issue.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@flxbk flxbk force-pushed the felix/active-series-shard-query branch 2 times, most recently from 6dc4f3f to 4c479b2 Compare January 15, 2024 18:08
@flxbk flxbk changed the title /active_series: fix sharded requests for incoming GET request /active_series: handle codes.Canceled, fix request shards for incoming GET requests Jan 15, 2024
@flxbk flxbk force-pushed the felix/active-series-shard-query branch from 4c479b2 to f781c91 Compare January 15, 2024 18:26
@flxbk flxbk force-pushed the felix/active-series-shard-query branch from f781c91 to 9ad2c8d Compare January 16, 2024 11:17
@flxbk flxbk changed the title /active_series: handle codes.Canceled, fix request shards for incoming GET requests /active_series: handle codes.Canceled, generate correct request shards for incoming GET requests Jan 16, 2024
@flxbk flxbk force-pushed the felix/active-series-shard-query branch from 9ad2c8d to 2fb29fe Compare January 16, 2024 11:53
@flxbk flxbk changed the title /active_series: handle codes.Canceled, generate correct request shards for incoming GET requests /active_series: generate correct request shards for incoming GET requests, handle codes.Canceled Jan 16, 2024
@@ -1848,7 +1850,7 @@ func (d *Distributor) ActiveSeries(ctx context.Context, matchers []*labels.Match

stream, err := client.ActiveSeries(ctx, req)
if err != nil {
if errors.Is(err, context.Canceled) {
if errors.Is(err, context.Canceled) || status.Code(err) == codes.Canceled {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to reuse this piece of code:

func wrapContextError(err error) error {
if err == nil {
return nil
}
// Get the gRPC status from the error.
type grpcErrorStatus interface{ GRPCStatus() *grpcstatus.Status }
var grpcError grpcErrorStatus
if !errors.As(err, &grpcError) {
return err
}
switch status := grpcError.GRPCStatus(); {
case status.Code() == codes.Canceled:
return &grpcContextError{grpcErr: err, grpcStatus: status, stdErr: context.Canceled}
case status.Code() == codes.DeadlineExceeded:
return &grpcContextError{grpcErr: err, grpcStatus: status, stdErr: context.DeadlineExceeded}
default:
return err
}
}
// grpcContextError is a custom error used to wrap gRPC errors which maps to golang standard context errors.
type grpcContextError struct {
// grpcErr is the gRPC error wrapped by grpcContextError.
grpcErr error
// grpcStatus is the gRPC status associated with the gRPC error. It's guaranteed to be non-nil.
grpcStatus *grpcstatus.Status
// stdErr is the equivalent golang standard context error.
stdErr error
}
func (e *grpcContextError) Error() string {
return e.grpcErr.Error()
}
func (e *grpcContextError) Unwrap() error {
return e.grpcErr
}
func (e *grpcContextError) Is(err error) bool {
return errors.Is(e.stdErr, err) || errors.Is(e.grpcErr, err)
}
func (e *grpcContextError) As(target any) bool {
if errors.As(e.stdErr, target) {
return true
}
return errors.As(e.grpcErr, target)
}
func (e *grpcContextError) GRPCStatus() *grpcstatus.Status {
return e.grpcStatus
}

So the check would be just:

if errors.Is(coolgrpc.WrapContextError, context.Canceled)

cc @pracucci who wrote that code in case he wants to express an opinion on reusing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved this code to util and reused it in this PR. Happy to have your suggestions in case you think we can improve naming and/or the location of this code (should it be a separate package?).

@flxbk flxbk force-pushed the felix/active-series-shard-query branch from a18eb83 to 46e8bff Compare January 16, 2024 14:29
@flxbk flxbk force-pushed the felix/active-series-shard-query branch from 46e8bff to ed10a8c Compare January 16, 2024 15:40
@flxbk flxbk marked this pull request as ready for review January 16, 2024 19:42
@flxbk flxbk requested a review from a team as a code owner January 16, 2024 19:42
@flxbk flxbk changed the title /active_series: generate correct request shards for incoming GET requests, handle codes.Canceled /active_series: generate correct request shards for incoming GET requests, handle gRPC errors Jan 17, 2024
@flxbk flxbk merged commit 52c39fb into main Jan 17, 2024
28 checks passed
@flxbk flxbk deleted the felix/active-series-shard-query branch January 17, 2024 17:02
grafanabot pushed a commit that referenced this pull request Jan 17, 2024
…requests, handle gRPC errors (#7133)

* codes.Canceled is not an error either

* add test for GET-request based shards

* generate shard requests correctly

* read response body only if it's not nil

* add test for context propagation

* add org id to generated requests

* use wrapped grpc error

* add integration test

* add license header and build tag

* remove stray parens

(cherry picked from commit 52c39fb)
flxbk added a commit that referenced this pull request Jan 17, 2024
…requests, handle gRPC errors (#7133) (#7156)

* codes.Canceled is not an error either

* add test for GET-request based shards

* generate shard requests correctly

* read response body only if it's not nil

* add test for context propagation

* add org id to generated requests

* use wrapped grpc error

* add integration test

* add license header and build tag

* remove stray parens

(cherry picked from commit 52c39fb)

Co-authored-by: Felix Beuke <felix.j.beuke@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants