fix(sync): Extract error message from gRPC error#477
fix(sync): Extract error message from gRPC error#477erezrokah wants to merge 2 commits intocloudquery:mainfrom
Conversation
| if err == io.EOF { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("failed to fetch resources from stream: %w", err) |
There was a problem hiding this comment.
failed to fetch resources from stream is incorrect (similar to #462 (comment)), as error can be anything returned from
plugin-sdk/internal/servers/source.go
Line 93 in cdcef5d
⏱️ Benchmark resultsComparing with cdcef5d
|
|
Next step to probably do #462 (comment) |
yevgenypats
left a comment
There was a problem hiding this comment.
What is the value of extracting it rather than printing it as is (without more code) with the grpc= given this error comes from grpc so basically we are hiding more information just because the string is a bit long?
This is not a gRPC error. This is an application level error and we almost always print Also explained in #477 (comment) |
Related to #462 and also #477. Two reasons for this change: 1. We shouldn't be using `codes.Internal` as it's generated internally by gRPC so we won't be able to distinguish between gRPC protocol errors and application level errors, see https://github.com/grpc/grpc-go/blob/eeb9afa1f6b6388152955eeca8926e36ca94c768/codes/codes.go#L166. 2. We don't really do anything with the code at the moment, it's only printed in a confusing way (see #477) ---
Right now we don't have a way to send application level errors (as those are mostly done via logs apart from config errors). We can implement this type of behaviour and add a field to the protocol but it's not the right place to do it over grpc errors. |
Agree we should not do it as a part of gRPC errors, hence #487. We don't need to add anything to the protocol as This PR doesn't change error handling, its meant to only remove the status print |
|
Closing since I think the fixes in #530 should be enough for now. We can re-visit this in the future |
Summary
Related to cloudquery/cloudquery#4707.
Errors returned from a gRPC call have both a status and a message. We don't really use the status, but when we wrap the error it always gets printed, see:
https://github.com/grpc/grpc-go/blob/eeb9afa1f6b6388152955eeca8926e36ca94c768/internal/status/status.go#L140
We can easily extract just the message and print only that.
Before:
After:
We should do this for all gRPC calls, but I'll do that in follow up PRs so I can test it
Use the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)