Skip to content

tracing: envoy.reloadable_features.update_grpc_response_error_tag deprecation #23098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adisuissa merged 2 commits into from
Sep 19, 2022
Merged

tracing: envoy.reloadable_features.update_grpc_response_error_tag deprecation #23098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ea0fff
remove deprecation
bryanwux Sep 14, 2022
27d1384
remove redundant code
bryanwux Sep 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
62 changes: 28 additions & 34 deletions source/common/tracing/http_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,42 +87,36 @@ template <class T> 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::GrpcStatus> grpc_status_code = Grpc::Common::getGrpcStatus(headers);
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.update_grpc_response_error_tag")) {
Copy link
Copy Markdown
Member

@daixiang0 daixiang0 Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old logic is that

if feature
  if
else

Now we have enabled the feature, still need the else on the top level?

Copy link
Copy Markdown

@giantcroc giantcroc Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need else any more, because the feature is always enabled.

Copy link
Copy Markdown
Contributor Author

@bryanwux bryanwux Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. The logic in else is actually the old code path which we don't need anymore.

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);
}
}
}

Expand Down
36 changes: 0 additions & 36 deletions test/common/tracing/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,47 +8,11 @@ licenses(["notice"]) # Apache 2

envoy_package()

envoy_cc_test(
name = "http_tracer_impl_legacy_test",
Copy link
Copy Markdown
Member

@daixiang0 daixiang0 Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between http_tracer_impl_test and http_tracer_impl_legacy_test?

Copy link
Copy Markdown
Contributor Author

@bryanwux bryanwux Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

runtime feature is enabled in http_tracer_impl_test while is not enabled in http_tracer_impl_legacy_test. Since runtime guard is now removed, I think we should also remove the 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",
Expand Down
40 changes: 10 additions & 30 deletions test/common/tracing/http_tracer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Http::Protocol> protocol = Http::Protocol::Http2;
absl::optional<uint32_t> response_code(200);
Expand All @@ -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,
Expand All @@ -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;

Expand All @@ -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,
Expand Down