diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 1f79df1b2f3bd..7444255e34aba 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -78,6 +78,9 @@ removed_config_or_runtime: - area: router change: | removed ``envoy.reloadable_features.update_expected_rq_timeout_on_retry`` and legacy code paths. +- area: tracing + change: | + removed ``envoy.reloadable_features.update_grpc_response_error_tag`` and legacy code paths. new_features: - area: http diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index f4f76aa0617a4..877c5fff43f63 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -68,7 +68,6 @@ RUNTIME_GUARD(envoy_reloadable_features_support_locality_update_on_eds_cluster_e RUNTIME_GUARD(envoy_reloadable_features_test_feature_true); RUNTIME_GUARD(envoy_reloadable_features_tls_async_cert_validation); RUNTIME_GUARD(envoy_reloadable_features_top_level_ecds_stats); -RUNTIME_GUARD(envoy_reloadable_features_update_grpc_response_error_tag); RUNTIME_GUARD(envoy_reloadable_features_use_rfc_connect); RUNTIME_GUARD(envoy_reloadable_features_validate_connect); RUNTIME_GUARD(envoy_restart_features_explicit_wildcard_resource); diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 9dbc0c92164b8..a61d65b38c5af 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -87,42 +87,36 @@ template static void addGrpcResponseTags(Span& span, const T& headers) // Set error tag when Grpc status code represents an upstream error. See // https://github.com/envoyproxy/envoy/issues/18877. absl::optional grpc_status_code = Grpc::Common::getGrpcStatus(headers); - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.update_grpc_response_error_tag")) { - if (grpc_status_code.has_value()) { - const auto& status = grpc_status_code.value(); - if (status != Grpc::Status::WellKnownGrpcStatus::InvalidCode) { - switch (status) { - // Each case below is considered to be a client side error, therefore should not be - // tagged as an upstream error. See https://grpc.github.io/grpc/core/md_doc_statuscodes.html - // for more details about how each Grpc status code is defined and whether it is an - // upstream error or a client error. - case Grpc::Status::WellKnownGrpcStatus::Ok: - case Grpc::Status::WellKnownGrpcStatus::Canceled: - case Grpc::Status::WellKnownGrpcStatus::InvalidArgument: - case Grpc::Status::WellKnownGrpcStatus::NotFound: - case Grpc::Status::WellKnownGrpcStatus::AlreadyExists: - case Grpc::Status::WellKnownGrpcStatus::PermissionDenied: - case Grpc::Status::WellKnownGrpcStatus::FailedPrecondition: - case Grpc::Status::WellKnownGrpcStatus::Aborted: - case Grpc::Status::WellKnownGrpcStatus::OutOfRange: - case Grpc::Status::WellKnownGrpcStatus::Unauthenticated: - break; - case Grpc::Status::WellKnownGrpcStatus::Unknown: - case Grpc::Status::WellKnownGrpcStatus::DeadlineExceeded: - case Grpc::Status::WellKnownGrpcStatus::Unimplemented: - case Grpc::Status::WellKnownGrpcStatus::ResourceExhausted: - case Grpc::Status::WellKnownGrpcStatus::Internal: - case Grpc::Status::WellKnownGrpcStatus::Unavailable: - case Grpc::Status::WellKnownGrpcStatus::DataLoss: - span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True); - break; - } + if (grpc_status_code.has_value()) { + const auto& status = grpc_status_code.value(); + if (status != Grpc::Status::WellKnownGrpcStatus::InvalidCode) { + switch (status) { + // Each case below is considered to be a client side error, therefore should not be + // tagged as an upstream error. See https://grpc.github.io/grpc/core/md_doc_statuscodes.html + // for more details about how each Grpc status code is defined and whether it is an + // upstream error or a client error. + case Grpc::Status::WellKnownGrpcStatus::Ok: + case Grpc::Status::WellKnownGrpcStatus::Canceled: + case Grpc::Status::WellKnownGrpcStatus::InvalidArgument: + case Grpc::Status::WellKnownGrpcStatus::NotFound: + case Grpc::Status::WellKnownGrpcStatus::AlreadyExists: + case Grpc::Status::WellKnownGrpcStatus::PermissionDenied: + case Grpc::Status::WellKnownGrpcStatus::FailedPrecondition: + case Grpc::Status::WellKnownGrpcStatus::Aborted: + case Grpc::Status::WellKnownGrpcStatus::OutOfRange: + case Grpc::Status::WellKnownGrpcStatus::Unauthenticated: + break; + case Grpc::Status::WellKnownGrpcStatus::Unknown: + case Grpc::Status::WellKnownGrpcStatus::DeadlineExceeded: + case Grpc::Status::WellKnownGrpcStatus::Unimplemented: + case Grpc::Status::WellKnownGrpcStatus::ResourceExhausted: + case Grpc::Status::WellKnownGrpcStatus::Internal: + case Grpc::Status::WellKnownGrpcStatus::Unavailable: + case Grpc::Status::WellKnownGrpcStatus::DataLoss: + span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True); + break; } } - } else { - if (grpc_status_code && grpc_status_code.value() != Grpc::Status::WellKnownGrpcStatus::Ok) { - span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True); - } } } diff --git a/test/common/tracing/BUILD b/test/common/tracing/BUILD index 9a49a1d9b1287..9160a233ce0ef 100644 --- a/test/common/tracing/BUILD +++ b/test/common/tracing/BUILD @@ -8,47 +8,11 @@ licenses(["notice"]) # Apache 2 envoy_package() -envoy_cc_test( - name = "http_tracer_impl_legacy_test", - srcs = [ - "http_tracer_impl_test.cc", - ], - args = [ - "--runtime-feature-disable-for-tests=envoy.reloadable_features.update_grpc_response_error_tag", - ], - coverage = True, - deps = [ - "//source/common/common:base64_lib", - "//source/common/http:header_map_lib", - "//source/common/http:headers_lib", - "//source/common/http:message_lib", - "//source/common/network:address_lib", - "//source/common/runtime:runtime_lib", - "//source/common/tracing:custom_tag_lib", - "//source/common/tracing:http_tracer_lib", - "//test/mocks/http:http_mocks", - "//test/mocks/local_info:local_info_mocks", - "//test/mocks/router:router_mocks", - "//test/mocks/runtime:runtime_mocks", - "//test/mocks/stats:stats_mocks", - "//test/mocks/thread_local:thread_local_mocks", - "//test/mocks/tracing:tracing_mocks", - "//test/test_common:environment_lib", - "//test/test_common:utility_lib", - "@envoy_api//envoy/config/core/v3:pkg_cc_proto", - "@envoy_api//envoy/type/tracing/v3:pkg_cc_proto", - ], -) - envoy_cc_test( name = "http_tracer_impl_test", srcs = [ "http_tracer_impl_test.cc", ], - args = [ - "--runtime-feature-override-for-tests=envoy.reloadable_features.update_grpc_response_error_tag", - ], - coverage = True, deps = [ "//source/common/common:base64_lib", "//source/common/http:header_map_lib", diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 85d3be9d3f13c..ade826516948a 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -625,13 +625,8 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcErrorTag) { {"content-type", "application/grpc"}}; Http::TestResponseTrailerMapImpl response_trailers; - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.update_grpc_response_error_tag")) { - response_trailers.setGrpcStatus("14"); - response_trailers.setGrpcMessage("unavailable"); - } else { - response_trailers.setGrpcStatus("7"); - response_trailers.setGrpcMessage("permission denied"); - } + response_trailers.setGrpcStatus("14"); + response_trailers.setGrpcMessage("unavailable"); absl::optional protocol = Http::Protocol::Http2; absl::optional response_code(200); @@ -649,14 +644,9 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcErrorTag) { EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcAuthority), Eq("example.com:80"))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcContentType), Eq("application/grpc"))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcTimeout), Eq("10s"))); - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.update_grpc_response_error_tag")) { - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14"))); - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("unavailable"))); - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); - } else { - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("7"))); - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied"))); - } + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("unavailable"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip))); HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers, @@ -680,13 +670,8 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcTrailersOnly) { Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, {"content-type", "application/grpc"}}; - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.update_grpc_response_error_tag")) { - response_headers.setGrpcStatus("14"); - response_headers.setGrpcMessage("unavailable"); - } else { - response_headers.setGrpcStatus("7"); - response_headers.setGrpcMessage("permission denied"); - } + response_headers.setGrpcStatus("14"); + response_headers.setGrpcMessage("unavailable"); Http::TestResponseTrailerMapImpl response_trailers; @@ -705,14 +690,9 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcTrailersOnly) { EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcPath), Eq("/pb.Foo/Bar"))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcAuthority), Eq("example.com:80"))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcContentType), Eq("application/grpc"))); - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.update_grpc_response_error_tag")) { - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14"))); - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("unavailable"))); - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); - } else { - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("7"))); - EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied"))); - } + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("unavailable"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().PeerAddress), Eq(expected_ip))); HttpTracerUtility::finalizeDownstreamSpan(span, &request_headers, &response_headers,