From 93b43d0745deaae666fbb2e239620d2303f05bd8 Mon Sep 17 00:00:00 2001 From: Caleb Gilmour Date: Sat, 24 Aug 2019 00:26:12 +0000 Subject: [PATCH] Fix status code and handle trailers-only case. Signed-off-by: Caleb Gilmour --- source/common/tracing/http_tracer_impl.cc | 36 ++++++++------- test/common/tracing/http_tracer_impl_test.cc | 46 ++++++++++++++++++-- 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index b0313e9fffbf..004e41c84a86 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -82,6 +82,22 @@ Decision HttpTracerUtility::isTracing(const StreamInfo::StreamInfo& stream_info, NOT_REACHED_GCOVR_EXCL_LINE; } +static void addGrpcTags(Span& span, const Http::HeaderMap& headers) { + const Http::HeaderEntry* grpc_status_header = headers.GrpcStatus(); + if (grpc_status_header) { + span.setTag(Tracing::Tags::get().GrpcStatusCode, grpc_status_header->value().getStringView()); + } + const Http::HeaderEntry* grpc_message_header = headers.GrpcMessage(); + if (grpc_message_header) { + span.setTag(Tracing::Tags::get().GrpcMessage, grpc_message_header->value().getStringView()); + } + absl::optional grpc_status_code = Grpc::Common::getGrpcStatus(headers); + // Set error tag when status is not OK. + if (grpc_status_code && grpc_status_code.value() != Grpc::Status::GrpcStatus::Ok) { + span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True); + } +} + static void annotateVerbose(Span& span, const StreamInfo::StreamInfo& stream_info) { const auto start_time = stream_info.startTime(); if (stream_info.lastDownstreamRxByteReceived()) { @@ -167,22 +183,10 @@ void HttpTracerUtility::finalizeSpan(Span& span, const Http::HeaderMap* request_ StreamInfo::ResponseFlagUtils::toShortString(stream_info)); // GRPC data. - if (response_headers && response_trailers && stream_info.protocol() == Http::Protocol::Http2 && - Grpc::Common::hasGrpcContentType(*response_headers)) { - const Http::HeaderEntry* grpc_status_header = response_trailers->GrpcStatus(); - if (grpc_status_header) { - span.setTag(Tracing::Tags::get().GrpcStatusCode, grpc_status_header->value().getStringView()); - } - const Http::HeaderEntry* grpc_message_header = response_trailers->GrpcMessage(); - if (grpc_message_header) { - span.setTag(Tracing::Tags::get().GrpcMessage, grpc_message_header->value().getStringView()); - } - absl::optional grpc_status_code = - Grpc::Common::getGrpcStatus(*response_trailers); - // Set error tag when status is not OK. - if (grpc_status_code && grpc_status_code.value() != Grpc::Status::GrpcStatus::Ok) { - span.setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True); - } + if (response_trailers && response_trailers->GrpcStatus() != nullptr) { + addGrpcTags(span, *response_trailers); + } else if (response_headers && response_headers->GrpcStatus() != nullptr) { + addGrpcTags(span, *response_headers); } if (tracing_config.verbose()) { diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 75f8baede814..4e1b991507f3 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -389,7 +389,7 @@ TEST(HttpConnManFinalizerImpl, GrpcOkStatus) { stream_info, config); } -TEST(HttpConnManFinalizerImpl, GrpcError) { +TEST(HttpConnManFinalizerImpl, GrpcErrorTag) { const std::string path_prefix = "http://"; NiceMock span; @@ -400,14 +400,14 @@ TEST(HttpConnManFinalizerImpl, GrpcError) { {"content-type", "application/grpc"}, {"te", "trailers"}}; - Http::TestHeaderMapImpl response_headers{{":status", "403"}, + Http::TestHeaderMapImpl response_headers{{":status", "200"}, {"content-type", "application/grpc"}}; Http::TestHeaderMapImpl response_trailers{{"grpc-status", "7"}, {"grpc-message", "permission denied"}}; NiceMock stream_info; absl::optional protocol = Http::Protocol::Http2; - absl::optional response_code(403); + absl::optional response_code(200); EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); @@ -417,7 +417,45 @@ TEST(HttpConnManFinalizerImpl, GrpcError) { 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("403"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("200"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("7"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied"))); + + NiceMock config; + HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers, + stream_info, config); +} + +TEST(HttpConnManFinalizerImpl, GrpcTrailersOnly) { + const std::string path_prefix = "http://"; + NiceMock span; + + Http::TestHeaderMapImpl request_headers{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/pb.Foo/Bar"}, + {":authority", "example.com:80"}, + {"content-type", "application/grpc"}, + {"te", "trailers"}}; + + Http::TestHeaderMapImpl response_headers{{":status", "200"}, + {"content-type", "application/grpc"}, + {"grpc-status", "7"}, + {"grpc-message", "permission denied"}}; + Http::TestHeaderMapImpl response_trailers; + NiceMock stream_info; + + absl::optional protocol = Http::Protocol::Http2; + absl::optional response_code(200); + EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); + EXPECT_CALL(stream_info, bytesReceived()).WillOnce(Return(10)); + EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(11)); + EXPECT_CALL(stream_info, protocol()).WillRepeatedly(ReturnPointee(&protocol)); + + 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"))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("7"))); EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GrpcMessage), Eq("permission denied")));