feat: Return application level errors as a part of the gRPC response

[erezrokah]

TLDR: We should consider having application level errors as a part of the gRPC responses, to make it easier to identify them.

For example in the following line:

plugin-sdk/clients/destination.go

Line 303 in ab7ca97

return fmt.Errorf("failed to CloseAndRecv client: %w", err)

failed to CloseAndRecv is not necessarily correct, as err is whatever is returned from the following function:

plugin-sdk/internal/servers/destinations.go

Line 65 in ab7ca97

func (s *DestinationServer) Write2(msg pb.Destination_Write2Server) error {

.

Not sure what's the best solution (need to do some research), but we could add an error(s) field to gRPC responses so we have a clear separation between application level errors and gRPC/communication level ones.

Also due to grpc/grpc-go#2934 we can't unwrap errors.

[erezrokah]

Looks like we could use WithDetails for this https://pkg.go.dev/google.golang.org/grpc/internal/status#Status.WithDetails

[yevgenypats]

I understand the need for application level errors but I don't think it should have anything with grpc errors. depending on which errors we want to send we need to add it to the protobuf. grpc errors are mostly used when something bad happened on the server.

[erezrokah]

I understand the need for application level errors

Most errors returned from

plugin-sdk/internal/servers/destinations.go

Line 65 in 62692b9

func (s *DestinationServer) Write2(msg pb.Destination_Write2Server) error {

are application level.

The only ones that could be related to gRPC protocol are:

plugin-sdk/internal/servers/destinations.go

Line 73 in 62692b9

return fmt.Errorf("write: failed to receive msg: %w", err)

plugin-sdk/internal/servers/destinations.go

Line 101 in 62692b9

return fmt.Errorf("failed to receive msg: %w", err)

, but also not necessarily depending on what error caused the stream to close

[yevgenypats]

There can be also grpc errror returned after this function is returned that we actually saw when buffer is too big so there is no good way to distinguish both over the grpc error field.

[erezrokah]

no good way to distinguish both over the grpc error field

Agree there could be both application errors and gRPC errors. Right now we're mixing both so the purpose of this issue is to discuss a solution so we can distinguish between them.

One approach is in #487, to use the status package as it essentially wraps the errors that are sent, along with the errdetails package. More in https://cloud.google.com/apis/design/errors#error_details

[erezrokah]

Closing as won't fix as we've improved error reporting