update tracing error tag for grpc status codes by bryanwux · Pull Request #20090 · envoyproxy/envoy

bryanwux · 2022-02-23T18:02:22Z

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 each response code should be indicated as an error 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:]

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

repokitteh-read-only · 2022-02-23T18:02:25Z

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: #20090 was opened by bryanwux.

see: more, trace.

repokitteh-read-only · 2022-02-23T18:47:21Z

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

🐱

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

see: more, trace.

KfreeZ · 2022-02-25T05:51:28Z

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


does these cover all the error status?

Yes, detailed introduction can be found here: https://grpc.github.io/grpc/core/md_doc_statuscodes.html

What do you think of converting to a switch, so if gRPC adds new error codes and we pick up on import, we compile fail?

Also can we link https://grpc.github.io/grpc/core/md_doc_statuscodes.html here in the comment as well?

What do you think of converting to a switch, so if gRPC adds new error codes and we pick up on import, we compile fail?

Yes, I agree a switch makes more sense in this case.

Also can we link https://grpc.github.io/grpc/core/md_doc_statuscodes.html here in the comment as well?

Done

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

rojkov · 2022-02-28T08:09:18Z

Looks like this is a continuation for #19603 which got mistakenly closed.

/assign @alyssawilk

alyssawilk

Sorry for the delay in review - I was out and it got misplaced when reopened
/wait

alyssawilk · 2022-02-28T17:13:19Z

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


What do you think of converting to a switch, so if gRPC adds new error codes and we pick up on import, we compile fail?

alyssawilk · 2022-02-28T17:14:14Z

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


Also can we link https://grpc.github.io/grpc/core/md_doc_statuscodes.html here in the comment as well?

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

  absl::optional<Grpc::Status::GrpcStatus> grpc_status_code = Grpc::Common::getGrpcStatus(headers);
-  if (grpc_status_code && grpc_status_code.value() != Grpc::Status::WellKnownGrpcStatus::Ok) {
-    span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True);
+  if (grpc_status_code.has_value()) {


I think we're going to want to runtime guard this change, as described in CONTRIBUTING.md

Let's also add a comment where the tracing error code is defined to be more clear about what error means in this case (upstream or envoy error, not client error)

repokitteh-read-only · 2022-03-01T09:49:11Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

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

bryanwux · 2022-03-01T16:20:35Z

/retest

repokitteh-read-only · 2022-03-01T16:20:38Z

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

🐱

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

see: more, trace.

alyssawilk

Looking better and better!

alyssawilk · 2022-03-01T19:05:47Z

+        case Grpc::Status::WellKnownGrpcStatus::OutOfRange:
+        case Grpc::Status::WellKnownGrpcStatus::Unauthenticated:
+          break;
+        default:


so if you have a default we still won't catch new error codes when added. Can we avoid this?

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

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

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

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

bryanwux · 2022-03-04T08:58:36Z

Sorry @lizan @snowp , requestd review was done by mistake, please ignore.

alyssawilk

Looking good! Sorry for the review delay I was out Friday.
Just two more questions, so I'll add in a second reviewer as well
/wait

alyssawilk · 2022-03-07T15:16:49Z

+        case Grpc::Status::WellKnownGrpcStatus::AlreadyExists:
+        case Grpc::Status::WellKnownGrpcStatus::PermissionDenied:
+        case Grpc::Status::WellKnownGrpcStatus::FailedPrecondition:
+        case Grpc::Status::WellKnownGrpcStatus::Aborted:


hm, canceled says "usually by the caller" but aborted says it's a concurrency issue. Would that be client side or server side error do you think?

aborted is used due to a concurrency issue which means the client should restart a read-modify-write sequence. I think this would be a client side error.

alyssawilk · 2022-03-07T15:18:39Z

@lizan I think this is ready for your pass (only 2 nits left on my end)

Below is the proposed change for tracing error tag behaviour. A brief description about whether each response code should be indicated as an error 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: Fixes envoyproxy#18877 Signed-off-by: bryanwux <jiayu1.wu@intel.com> Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>

update tracing error tag for grpc status codes

33732fc

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

KfreeZ reviewed Feb 25, 2022

View reviewed changes

bryanwux added 2 commits February 28, 2022 02:49

Merge branch 'envoyproxy:main' into main

124b3b7

fix formatting

5cd17ac

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

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

alyssawilk reviewed Feb 28, 2022

View reviewed changes

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

Merge branch 'envoyproxy:main' into main

e8600f6

repokitteh-read-only Bot removed the waiting label Mar 1, 2022

add runtime guard and change if to switch when checking grpc status code

da6c0fc

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

alyssawilk reviewed Mar 1, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Mar 1, 2022

Merge branch 'envoyproxy:main' into main

4f0e818

repokitteh-read-only Bot removed the waiting label Mar 2, 2022

bryanwux added 5 commits March 2, 2022 16:00

add unit tests

66db5b4

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

Merge branch 'main' of https://github.com/bryanwux/envoy into main

40ad373

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

change panic logs

d272800

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

fix typo

e5f58c0

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

Merge branch 'envoyproxy:main' into main

45838d3

repokitteh-read-only Bot added the waiting label Mar 2, 2022

remove default

75c711c

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

repokitteh-read-only Bot removed the waiting label Mar 3, 2022

alyssawilk added the waiting label Mar 3, 2022

repokitteh-read-only Bot removed the waiting label Mar 4, 2022

bryanwux requested review from lizan and snowp as code owners March 4, 2022 08:30

bryanwux closed this Mar 4, 2022

bryanwux reopened this Mar 4, 2022

bryanwux added 2 commits March 4, 2022 16:54

remove default and update tests

a77e2af

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

fix build

fea65c1

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

alyssawilk reviewed Mar 7, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Mar 7, 2022

alyssawilk assigned lizan Mar 7, 2022

alyssawilk approved these changes Mar 9, 2022

View reviewed changes

lizan approved these changes Mar 10, 2022

View reviewed changes

lizan merged commit cc6b501 into envoyproxy:main Mar 10, 2022

bryanwux mentioned this pull request Sep 14, 2022

tracing: envoy.reloadable_features.update_grpc_response_error_tag deprecation #23098

Merged

Conversation

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

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Feb 23, 2022

Uh oh!

repokitteh-read-only Bot commented Feb 23, 2022

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!

rojkov commented Feb 28, 2022

Uh oh!

alyssawilk 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!

bryanwux Mar 1, 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!

repokitteh-read-only Bot commented Mar 1, 2022

Uh oh!

bryanwux commented Mar 1, 2022

Uh oh!

repokitteh-read-only Bot commented Mar 1, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryanwux commented Mar 4, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Mar 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bryanwux commented Feb 23, 2022 •

edited

Loading

bryanwux Mar 1, 2022 •

edited

Loading