refactor(bigquery/storage/managedwriter): refactor error metrics #8314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a small refactor to how opencensus metrics are reported when receiving responses from the server. There's effectively two cases where we consider an error to have occurred on receive: * Invoking gRPC Recv() on the connection emitted an error, typically a transport issue * The service embedded an error in the response, which is more akin to an application error (failed to commit data, offset mismatch, etc). This CL ensures we increment the existing AppendResponseError count metric in both causes, and deals with the unlikely scenario where we're unable to tag the report with the status code. When that happens, we simply record the metric uncoded.
product-auto-label
bot
added
size: s
shollyman
requested review from
GaoleMeng and
alvarowolfx
and removed request for
GaoleMeng
alvarowolfx
reviewed
Jul 24, 2023
| @@ -494,18 +494,29 @@ func connRecvProcessor(ctx context.Context, co *connection, arc storagepb.BigQue | |||
| resp, err := arc.Recv() | |||
| co.release(nextWrite) | |||
| if err != nil { | |||
| // The Recv() itself yielded an error. We increment AppendResponseErrors by one, tagged by the status | |||
| // code. | |||
| status := grpcstatus.Convert(err) | |||
There was a problem hiding this comment.
I wonder if status can be nil here as this err is coming from a grpc stream method. Might worth add the extra check
There was a problem hiding this comment.
an io.EOF will probably cause grpcstatus.Convert to return nil
There was a problem hiding this comment.
It wraps the error as a code.Unknown if it doesn't embed an rpc status error, ex: https://github.com/grpc/grpc-go/blob/faab8736bf73291f92b867d5dae31c927d53d508/status/status.go#L96-L125
alvarowolfx
approved these changes
Jul 25, 2023
shollyman
added
the
automerge
gcf-merge-on-green
bot
removed
the
automerge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a small refactor to how opencensus metrics are reported when receiving responses from the server.
There's effectively two cases where we consider an error to have occurred on receive:
Invoking gRPC Recv() on the connection emitted an error, typically a transport issue
The service embedded an error in the response, which is more akin to an application error (failed to commit data, offset mismatch, etc).
This CL ensures we increment the existing AppendResponseError count metric in both causes, and deals with the unlikely scenario where we're unable to tag the report with the status code. When that happens, we simply record the metric uncoded.
Towards: #8311