Skip to content

Commit

Permalink
Fix status code and handle trailers-only case.
Browse files Browse the repository at this point in the history 
Signed-off-by: Caleb Gilmour <caleb.gilmour@datadoghq.com>
  • Loading branch information
@cgilmour
cgilmour committed Aug 24, 2019
1 parent 958947f commit 93b43d0
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 20 deletions.
36 changes: 20 additions & 16 deletions source/common/tracing/http_tracer_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::GrpcStatus> 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()) {
Expand Down Expand Up @@ -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::GrpcStatus> 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()) {
Expand Down
46 changes: 42 additions & 4 deletions test/common/tracing/http_tracer_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ TEST(HttpConnManFinalizerImpl, GrpcOkStatus) {
stream_info, config);
}

TEST(HttpConnManFinalizerImpl, GrpcError) {
TEST(HttpConnManFinalizerImpl, GrpcErrorTag) {
const std::string path_prefix = "http://";
NiceMock<MockSpan> span;

Expand All @@ -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<StreamInfo::MockStreamInfo> stream_info;

absl::optional<Http::Protocol> protocol = Http::Protocol::Http2;
absl::optional<uint32_t> response_code(403);
absl::optional<uint32_t> 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));
Expand All @@ -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<MockConfig> config;
HttpTracerUtility::finalizeSpan(span, &request_headers, &response_headers, &response_trailers,
stream_info, config);
}

TEST(HttpConnManFinalizerImpl, GrpcTrailersOnly) {
const std::string path_prefix = "http://";
NiceMock<MockSpan> 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<StreamInfo::MockStreamInfo> stream_info;

absl::optional<Http::Protocol> protocol = Http::Protocol::Http2;
absl::optional<uint32_t> 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")));

Expand Down

0 comments on commit 93b43d0

Please sign in to comment.