grpc: Update tracing error tags for grpc status code by bryanwux · Pull Request #19603 · envoyproxy/envoy

bryanwux · 2022-01-19T06:50:44Z

Signed-off-by: Jiayu Wu jiayu1.wu@intel.com

Commit Message: Change tracing error tag behaviour for grpc codes #18877
Additional Description:

Below is the proposed change for tracing error tag behaviour. A brief description about whether it is an error for each response code has also been listed.

OK | 0 | error=false 
CANCELLED | 1 |  error=false Not a server error. The operation is cancelled by the client.
UNKNOWN | 2 |  error=true   Server error. Status value received from unknown address space. 
INVALID_ARGUMENT | 3 | error=false  Not a server error. The client enters an invalid argument. 
DEADLINE_EXCEEDED | 4 | error=true  Server error. Response from the server has been delayed long, even if it may be successful.
NOT_FOUND | 5 | error=false   Not a server error. The requested file or directory is not found.
ALREADY_EXISTS | 6 | error=false   Not a server error. The file or directory the client attempts to create already exists.
PERMISSION_DENIED | 7 | error=false   Not a server error. The client doesn't have permission to execute the operation. 
RESOURCE_EXHAUSTED | 8 | error=false   Not a server error. Some resource has been exhausted like the file system is out of space.
FAILED_PRECONDITION | 9 | error=false   Not a server error. The requested operation is rejected because it doesn't meet preconditions. 
ABORTED | 10 | error=false   Not a server error. The operation is aborted, typically due to concurrency issues.
OUT_OF_RANGE | 11 | error=false   Not a server error. The operation was attempted to read past the valid range.
UNIMPLEMENTED | 12 | error=true   Server error. The service is not implemented or not enabled.
INTERNAL | 13 | error=true   Server error. The system is broken, typically reserved for serious cases. 
UNAVAILABLE | 14 | error=true   Server error. The service is currently not available.
DATA_LOSS | 15 | error=true   Server error. Unrecoverable data loss or corruption.
UNAUTHENTICATED | 16 | error=false   Not a server error. The request does not have valid credentials for the operation.

Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]: #18877
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2022-01-19T06:50:48Z

Hi @bryanwux, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19603 was opened by bryanwux.

see: more, trace.

rojkov · 2022-01-19T09:14:01Z

Could you please fix DCO and check formatting?

CONTRIBUTING.md contains some useful info about it.

tyxia

Thanks for working on it!

tyxia · 2022-01-19T21:51:53Z

Nit: I think it is better that we have a local variable stores the value upfront instead of calling value() repeatedly, both from performance and readability perspective. e.g., :
const auto& status = grpc_status_code.value();

tyxia · 2022-01-19T21:54:33Z

Even though MaximumKnown = Unauthenticated in terms of enum value, MaximumKnown is used to represent the boundary of the grpc status code. for example you can do for (uint32_t i = 0; i <= Status::WellKnownGrpcStatus::MaximumKnown; ++i)

I think we should use Unauthenticated here.

tyxia · 2022-01-19T21:59:52Z

I don't think we should include this as error code. InvalidCode means status code in gRPC headers was invalid that we can not use grpc_status_code.

tyxia · 2022-01-19T22:02:29Z

We should check grpc_status_code.value() != Grpc::Status::WellKnownGrpcStatus::InvalidCode) here rather than !ok

NOT_OK is redundant once you added those new checks in this PR

tyxia · 2022-01-19T22:13:14Z

Not your code, we need to be careful about the implicit conversion to bool especially when we are dealing with absl::optional. Detailed reading of this open source version of Google coding guide can be found here https://abseil.io/tips/141

I would suggest to change it to grpc_status_code.has_value().

tyxia · 2022-01-19T22:19:09Z

/assign @tyxia

rojkov · 2022-01-24T08:40:44Z

/wait

Signed-off-by: Jiayu Wu <jiayu1.wu@intel.com>

bryanwux · 2022-01-25T10:46:58Z

Thank you for your @tyxia review. I have updated the code accordingly.

bryanwux · 2022-01-26T03:22:10Z

Could you please fix DCO and check formatting?

CONTRIBUTING.md contains some useful info about it.

Done.

tyxia

LGTM modulo one nit. Thanks!

tyxia · 2022-01-27T01:36:17Z

nit, for consistency you may want to move stauts assignment up

if (grpc_status_code.has_value()) { const auto& status = grpc_status_code.value(); if (status != Grpc::Status::WellKnownGrpcStatus::InvalidCode && ( ... == unkown || ...== DeadlineExceed ||... == Unimplemented ......)) { span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True); } }

The line 91 seems to be redundant: status is checked for equality with Grpc::Status::WellKnownGrpcStatus::InvalidCode below. Can we drop the line?

Signed-off-by: Jiayu Wu <jiayu1.wu@intel.com>

rojkov

Thanks! Added one comment.

Please don't use git force-push. It makes reviewing new changes more difficult.

rojkov · 2022-01-31T08:01:45Z

The line 91 seems to be redundant: status is checked for equality with Grpc::Status::WellKnownGrpcStatus::InvalidCode below. Can we drop the line?

rojkov · 2022-02-01T07:56:36Z

/wait

Signed-off-by: Jiayu Wu <jiayu1.wu@intel.com>

bryanwux · 2022-02-02T15:55:28Z

/retest

repokitteh-read-only · 2022-02-02T15:55:32Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19603 (comment) was created by @bryanwux.

see: more, trace.

rojkov

Thank you! Looks good to me.

rojkov · 2022-02-03T07:24:12Z

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2022-02-03T07:24:16Z

@envoyproxy/senior-maintainers assignee is @alyssawilk

🐱

Caused by: a #19603 (comment) was created by @rojkov.

see: more, trace.

alyssawilk

Code looks good but two requests.
One can you edit your original table in the linked bug with a bit more detail as to why each error is considered a server error and non-error is a user error (or other) and two, can you add an entry to the docs for the current release (docs/root/version_history/current.rst) to cover the change in behavior? Thanks!
/wait

alyssawilk · 2022-02-15T17:31:21Z

still waiting on the issue update I think
/wait

Signed-off-by: Jiayu Wu <jiayu1.wu@intel.com>

bryanwux · 2022-02-23T13:32:46Z

Misclicked...

bryanwux · 2022-02-23T13:33:26Z

Code looks good but two requests. One can you edit your original table in the linked bug with a bit more detail as to why each error is considered a server error and non-error is a user error (or other) and two, can you add an entry to the docs for the current release (docs/root/version_history/current.rst) to cover the change in behavior? Thanks! /wait

Done. Apologies for the delay.

alyssawilk · 2022-02-28T16:54:26Z

Pr is still closed - I think you have to reopen

alyssawilk · 2022-02-28T17:11:10Z

ah, nm, I see you opened as a new PR.

rojkov self-assigned this Jan 19, 2022

tyxia requested changes Jan 19, 2022

View reviewed changes

repokitteh-read-only Bot assigned tyxia Jan 19, 2022

repokitteh-read-only Bot added waiting and removed waiting labels Jan 24, 2022

update tracing error tag for grpc status codes

f2e353a

Signed-off-by: Jiayu Wu <jiayu1.wu@intel.com>

tyxia reviewed Jan 27, 2022

View reviewed changes

remove error tag checking for Permission Denied in http tracer test

5324c8a

Signed-off-by: Jiayu Wu <jiayu1.wu@intel.com>

rojkov suggested changes Jan 31, 2022

View reviewed changes

repokitteh-read-only Bot added waiting and removed waiting labels Feb 1, 2022

remove redundant line

3e7beec

Signed-off-by: Jiayu Wu <jiayu1.wu@intel.com>

rojkov previously approved these changes Feb 3, 2022

View reviewed changes

repokitteh-read-only Bot assigned alyssawilk Feb 3, 2022

alyssawilk reviewed Feb 3, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Feb 3, 2022

bryanwux dismissed rojkov’s stale review via faf9b69 February 15, 2022 03:20

repokitteh-read-only Bot removed the waiting label Feb 15, 2022

repokitteh-read-only Bot added waiting and removed waiting labels Feb 15, 2022

bryanwux added 2 commits February 21, 2022 16:02

Merge branch 'envoyproxy:main' into main

4270b98

update release notes and documentation

dba1322

Signed-off-by: Jiayu Wu <jiayu1.wu@intel.com>

bryanwux closed this Feb 23, 2022

bryanwux reopened this Feb 23, 2022

rojkov mentioned this pull request Feb 28, 2022

update tracing error tag for grpc status codes #20090

Merged

Conversation

bryanwux commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Jan 19, 2022

Uh oh!

rojkov commented Jan 19, 2022

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

tyxia Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxia Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanwux Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxia commented Jan 19, 2022

Uh oh!

rojkov commented Jan 24, 2022

Uh oh!

bryanwux commented Jan 25, 2022

Uh oh!

bryanwux commented Jan 26, 2022

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov commented Feb 1, 2022

Uh oh!

bryanwux commented Feb 2, 2022

Uh oh!

repokitteh-read-only Bot commented Feb 2, 2022

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

rojkov commented Feb 3, 2022

Uh oh!

repokitteh-read-only Bot commented Feb 3, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Feb 15, 2022

Uh oh!

bryanwux commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwux commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Feb 28, 2022

bryanwux commented Jan 19, 2022 •

edited

Loading

tyxia Jan 19, 2022 •

edited

Loading

tyxia Jan 19, 2022 •

edited

Loading

bryanwux Jan 25, 2022 •

edited

Loading

bryanwux commented Feb 23, 2022 •

edited

Loading

bryanwux commented Feb 23, 2022 •

edited

Loading