grpc: Update tracing error tags for grpc status code

docs/root/intro/arch_overview/observability/tracing.rst

@@ -134,7 +134,8 @@ associated with it. Each span generated by Envoy contains the following data:
 * Upstream cluster name, observability name, and address.
 * HTTP response status code.
 * GRPC response status and message (if available).
-* An error tag when HTTP status is 5xx or GRPC status is not "OK".
+* An error tag when HTTP status is 5xx or GRPC status is not "OK" and represents a server side error.
+  See `GRPC's documentation <https://grpc.github.io/grpc/core/md_doc_statuscodes.html>`_ for more information about GRPC status code.
 * Tracing system-specific metadata.
 
 The span also includes a name (or operation) which by default is defined as the host of the invoked

docs/root/version_history/current.rst

@@ -19,6 +19,7 @@ Minor Behavior Changes
 * http: now the max concurrent streams of http2 connection can not only be adjusted down according to the SETTINGS frame but also can be adjusted up, of course, it can not exceed the configured upper bounds. This fix is guarded by ``envoy.reloadable_features.http2_allow_capacity_increase_by_settings``.
 * http: when writing custom filters, `injectEncodedDataToFilterChain` and `injectDecodedDataToFilterChain` now trigger sending of headers if they were not yet sent due to `StopIteration`. Previously, calling one of the inject functions in that state would trigger an assertion. See issue #19891 for more details.
 * perf: ssl contexts are now tracked without scan based garbage collection and greatly improved the performance on secret update.
+* tracing: set tracing error tag for grpc non-ok response code only when it is a server side error.
 
 Bug Fixes
 ---------

source/common/tracing/http_tracer_impl.cc

@@ -84,10 +84,21 @@ static void addGrpcRequestTags(Span& span, const Http::RequestHeaderMap& headers
 template <class T> static void addGrpcResponseTags(Span& span, const T& headers) {
   addTagIfNotNull(span, Tracing::Tags::get().GrpcStatusCode, headers.GrpcStatus());
   addTagIfNotNull(span, Tracing::Tags::get().GrpcMessage, headers.GrpcMessage());
-  // Set error tag when status is not OK.
+  // Set error tag when Grpc status code represents an error. See
+  // https://github.com/envoyproxy/envoy/issues/18877
   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()) {
+    const auto& status = grpc_status_code.value();
+    if (status != Grpc::Status::WellKnownGrpcStatus::InvalidCode &&
+        (status == Grpc::Status::WellKnownGrpcStatus::Unknown ||
+         status == Grpc::Status::WellKnownGrpcStatus::DeadlineExceeded ||
+         status == Grpc::Status::WellKnownGrpcStatus::Unimplemented ||
+         status == Grpc::Status::WellKnownGrpcStatus::Internal ||
+         status == Grpc::Status::WellKnownGrpcStatus::Unavailable ||
+         status == Grpc::Status::WellKnownGrpcStatus::DataLoss ||
+         status == Grpc::Status::WellKnownGrpcStatus::Unauthenticated)) {
+      span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True);
+    }
   }
 }
 

test/common/tracing/http_tracer_impl_test.cc

@@ -634,7 +634,6 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcErrorTag) {
   stream_info.downstream_connection_info_provider_->setDirectRemoteAddressForTest(remote_address);
 
   EXPECT_CALL(span, setTag(_, _)).Times(testing::AnyNumber());
-  EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("POST")));
   EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2")));
   EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("200")));
@@ -679,7 +678,6 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcTrailersOnly) {
   stream_info.downstream_connection_info_provider_->setDirectRemoteAddressForTest(remote_address);
 
   EXPECT_CALL(span, setTag(_, _)).Times(testing::AnyNumber());
-  EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("POST")));
   EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2")));
   EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("200")));
@@ -825,7 +823,6 @@ TEST_F(HttpTracerImplTest, ChildUpstreamSpanTest) {
   EXPECT_CALL(*second_span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("200")));
   EXPECT_CALL(*second_span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("7")));
   EXPECT_CALL(*second_span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied")));
-  EXPECT_CALL(*second_span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
 
   HttpTracerUtility::finalizeUpstreamSpan(*child_span, &response_headers_, &response_trailers_,
                                           stream_info_, config_);