From 4542104825c5198ec0aaec28ce1e8b159459ccdf Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Sun, 4 Dec 2016 16:43:05 -0800 Subject: [PATCH 01/13] initial. --- include/envoy/http/access_log.h | 33 +++---- .../http/access_log/access_log_formatter.cc | 86 +++++++++++++------ .../http/access_log/access_log_formatter.h | 6 +- .../http/access_log/request_info_impl.h | 6 +- .../access_log/access_log_formatter_test.cc | 10 ++- .../http/access_log/access_log_impl_test.cc | 4 +- test/common/tracing/http_tracer_impl_test.cc | 6 ++ test/mocks/http/mocks.h | 2 +- 8 files changed, 105 insertions(+), 48 deletions(-) diff --git a/include/envoy/http/access_log.h b/include/envoy/http/access_log.h index 2f1477df257e8..54369caf2bc1c 100644 --- a/include/envoy/http/access_log.h +++ b/include/envoy/http/access_log.h @@ -10,29 +10,31 @@ namespace Http { namespace AccessLog { -enum class FailureReason { +enum FailureReason { // No failure. - None, + None = 0x0, // Local server healthcheck failed. - FailedLocalHealthCheck, + FailedLocalHealthCheck = 0x1, // No healthy upstream. - NoHealthyUpstream, + NoHealthyUpstream = 0x2, // Request timeout on upstream. - UpstreamRequestTimeout, + UpstreamRequestTimeout = 0x4, // Local codec level reset was sent on the stream. - LocalReset, + LocalReset = 0x8, // Remote codec level reset was received on the stream. - UpstreamRemoteReset, + UpstreamRemoteReset = 0x10, // Local reset by a connection pool due to an initial connection failure. - UpstreamConnectionFailure, + UpstreamConnectionFailure = 0x20, // If the stream was locally reset due to connection termination. - UpstreamConnectionTermination, + UpstreamConnectionTermination = 0x40, // The stream was reset because of a resource overflow. - UpstreamOverflow, + UpstreamOverflow = 0x80, // No route found for a given request. - NoRouteFound, - // Used when faults (abort with error codes) are injected. - FaultInjected, + NoRouteFound = 0x100, + // Abort with error code is injected. + FaultInjected = 0x200, + // Delay is injected. + DelayInjected = 0x400 }; /** @@ -89,9 +91,10 @@ class RequestInfo { virtual std::chrono::milliseconds duration() const PURE; /** - * @return the failure reason for richer log experience. + * @return the failure reason for richer log experience, this can be represented as binary OR of + * FailureReason enum values. */ - virtual FailureReason failureReason() const PURE; + virtual uint64_t failureReason() const PURE; /** * @return upstream host description. diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index 73fa92464b871..d41c936e86218 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -17,34 +17,72 @@ const std::string FilterReasonUtils::UPSTREAM_CONNECTION_TERMINATION = "UC"; const std::string FilterReasonUtils::UPSTREAM_OVERFLOW = "UO"; const std::string FilterReasonUtils::NO_ROUTE_FOUND = "NR"; const std::string FilterReasonUtils::FAULT_INJECTED = "FI"; +const std::string FilterReasonUtils::DELAY_INJECTED = "DI"; -const std::string& FilterReasonUtils::toShortString(FailureReason failure_reason) { - switch (failure_reason) { - case FailureReason::None: +void FilterReasonUtils::appendString(std::string& result, const std::string& append) { + if (result.empty()) { + result = append; + } else { + result += "," + append; + } +} + +const std::string FilterReasonUtils::toShortString(uint64_t failure_reason) { + std::string result = ""; + + if (failure_reason == FailureReason::None) { return NONE; - case FailureReason::FailedLocalHealthCheck: - return FAILED_LOCAL_HEALTH_CHECK; - case FailureReason::NoHealthyUpstream: - return NO_HEALTHY_UPSTREAM; - case FailureReason::UpstreamRequestTimeout: - return UPSTREAM_REQUEST_TIMEOUT; - case FailureReason::LocalReset: - return LOCAL_RESET; - case FailureReason::UpstreamRemoteReset: - return UPSTREAM_REMOTE_RESET; - case FailureReason::UpstreamConnectionFailure: - return UPSTREAM_CONNECTION_FAILURE; - case FailureReason::UpstreamConnectionTermination: - return UPSTREAM_CONNECTION_TERMINATION; - case FailureReason::UpstreamOverflow: - return UPSTREAM_OVERFLOW; - case FailureReason::NoRouteFound: - return NO_ROUTE_FOUND; - case FailureReason::FaultInjected: - return FAULT_INJECTED; } - throw std::invalid_argument("Unknown failure_reason"); + if (failure_reason & FailureReason::FailedLocalHealthCheck) { + appendString(result, FAILED_LOCAL_HEALTH_CHECK); + } + + if (failure_reason & FailureReason::NoHealthyUpstream) { + appendString(result, NO_HEALTHY_UPSTREAM); + } + + if (failure_reason & FailureReason::UpstreamRequestTimeout) { + appendString(result, UPSTREAM_REQUEST_TIMEOUT); + } + + if (failure_reason & FailureReason::LocalReset) { + appendString(result, LOCAL_RESET); + } + + if (failure_reason & FailureReason::UpstreamRemoteReset) { + appendString(result, UPSTREAM_REMOTE_RESET); + } + + if (failure_reason & FailureReason::UpstreamConnectionFailure) { + appendString(result, UPSTREAM_CONNECTION_FAILURE); + } + + if (failure_reason & FailureReason::UpstreamConnectionTermination) { + appendString(result, UPSTREAM_CONNECTION_TERMINATION); + } + + if (failure_reason & FailureReason::UpstreamOverflow) { + appendString(result, UPSTREAM_OVERFLOW); + } + + if (failure_reason & FailureReason::NoRouteFound) { + appendString(result, NO_ROUTE_FOUND); + } + + if (failure_reason & FailureReason::FaultInjected) { + appendString(result, FAULT_INJECTED); + } + + if (failure_reason & FailureReason::DelayInjected) { + appendString(result, DELAY_INJECTED); + } + + if (result.empty()) { + throw std::invalid_argument(fmt::format("Unknown failure_reason code {}", failure_reason)); + } + + return result; } const std::string AccessLogFormatUtils::DEFAULT_FORMAT = diff --git a/source/common/http/access_log/access_log_formatter.h b/source/common/http/access_log/access_log_formatter.h index b6a0ddb46f48b..a220fb47d112f 100644 --- a/source/common/http/access_log/access_log_formatter.h +++ b/source/common/http/access_log/access_log_formatter.h @@ -10,10 +10,11 @@ namespace AccessLog { */ class FilterReasonUtils { public: - static const std::string& toShortString(FailureReason failure_reason); + static const std::string toShortString(uint64_t failure_reason); private: - FilterReasonUtils(){}; + FilterReasonUtils(); + static void appendString(std::string& result, const std::string& append); const static std::string NONE; const static std::string FAILED_LOCAL_HEALTH_CHECK; @@ -26,6 +27,7 @@ class FilterReasonUtils { const static std::string UPSTREAM_OVERFLOW; const static std::string NO_ROUTE_FOUND; const static std::string FAULT_INJECTED; + const static std::string DELAY_INJECTED; }; /** diff --git a/source/common/http/access_log/request_info_impl.h b/source/common/http/access_log/request_info_impl.h index bed92f75416d3..5114f60bf7b16 100644 --- a/source/common/http/access_log/request_info_impl.h +++ b/source/common/http/access_log/request_info_impl.h @@ -27,10 +27,10 @@ struct RequestInfoImpl : public RequestInfo { } void onFailedResponse(Http::AccessLog::FailureReason failure_reason) override { - failure_reason_ = failure_reason; + failure_reason_ |= failure_reason; } - Http::AccessLog::FailureReason failureReason() const override { return failure_reason_; } + uint64_t failureReason() const override { return failure_reason_; } void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) override { upstream_host_ = host; } @@ -45,7 +45,7 @@ struct RequestInfoImpl : public RequestInfo { uint64_t bytes_received_{}; Optional response_code_; uint64_t bytes_sent_{}; - FailureReason failure_reason_{FailureReason::None}; + uint64_t failure_reason_{FailureReason::None}; Upstream::HostDescriptionPtr upstream_host_{}; bool hc_request_{}; }; diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index 65d1eab89e738..66889c9302b34 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -23,11 +23,19 @@ TEST(FailureReasonUtilsTest, toShortStringConversion) { std::make_pair(FailureReason::UpstreamConnectionTermination, "UC"), std::make_pair(FailureReason::UpstreamOverflow, "UO"), std::make_pair(FailureReason::NoRouteFound, "NR"), - std::make_pair(FailureReason::FaultInjected, "FI")}; + std::make_pair(FailureReason::FaultInjected, "FI"), + std::make_pair(FailureReason::DelayInjected, "DI")}; for (const auto& testCase : expected) { EXPECT_EQ(testCase.second, FilterReasonUtils::toShortString(testCase.first)); } + + // Test combinations. + EXPECT_EQ("UT,DI", FilterReasonUtils::toShortString(FailureReason::DelayInjected | + FailureReason::UpstreamRequestTimeout)); + EXPECT_EQ("UT,FI,DI", FilterReasonUtils::toShortString(FailureReason::FaultInjected | + FailureReason::DelayInjected | + FailureReason::UpstreamRequestTimeout)); } TEST(AccessLogFormatUtilsTest, protocolToString) { diff --git a/test/common/http/access_log/access_log_impl_test.cc b/test/common/http/access_log/access_log_impl_test.cc index 0d8eff2d692b8..81525e084d638 100644 --- a/test/common/http/access_log/access_log_impl_test.cc +++ b/test/common/http/access_log/access_log_impl_test.cc @@ -43,7 +43,7 @@ class TestRequestInfo : public RequestInfo { std::chrono::milliseconds duration() const override { return std::chrono::milliseconds(duration_); } - Http::AccessLog::FailureReason failureReason() const override { return failure_reason_; } + uint64_t failureReason() const override { return failure_reason_; } void onFailedResponse(FailureReason failure_reason) override { failure_reason_ = failure_reason; } void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) override { upstream_host_ = host; } Upstream::HostDescriptionPtr upstreamHost() const override { return upstream_host_; } @@ -53,7 +53,7 @@ class TestRequestInfo : public RequestInfo { SystemTime start_time_; Protocol protocol_{Protocol::Http11}; Optional response_code_; - FailureReason failure_reason_{FailureReason::None}; + uint64_t failure_reason_{FailureReason::None}; uint64_t duration_{3}; Upstream::HostDescriptionPtr upstream_host_{}; bool hc_request_{}; diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 615b1e3f3678b..b4c3119a6b49e 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -408,6 +408,8 @@ TEST_F(LightStepSinkTest, FlushSeveralSpans) { setupValidSink(); NiceMock request_info; + ON_CALL(request_info, failureReason()) + .WillByDefault(Return(Http::AccessLog::FailureReason::None)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); @@ -477,6 +479,8 @@ TEST_F(LightStepSinkTest, FlushSpansTimer) { setupValidSink(); NiceMock request_info; + ON_CALL(request_info, failureReason()) + .WillByDefault(Return(Http::AccessLog::FailureReason::None)); const Optional timeout(std::chrono::seconds(5)); EXPECT_CALL(cm_.async_client_, send_(_, _, timeout)); @@ -512,6 +516,8 @@ TEST_F(LightStepSinkTest, FlushOneSpanGrpcFailure) { setupValidSink(); NiceMock request_info; + ON_CALL(request_info, failureReason()) + .WillByDefault(Return(Http::AccessLog::FailureReason::None)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 711e755dfe6f6..a4c4eae2b3752 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -42,7 +42,7 @@ class MockRequestInfo : public RequestInfo { MOCK_CONST_METHOD0(responseCode, Optional&()); MOCK_CONST_METHOD0(bytesSent, uint64_t()); MOCK_CONST_METHOD0(duration, std::chrono::milliseconds()); - MOCK_CONST_METHOD0(failureReason, FailureReason()); + MOCK_CONST_METHOD0(failureReason, uint64_t()); MOCK_CONST_METHOD0(upstreamHost, Upstream::HostDescriptionPtr()); MOCK_CONST_METHOD0(healthCheck, bool()); MOCK_METHOD1(healthCheck, void(bool is_hc)); From 2c62c6da19bd763e0f920943d3240fc10651dea2 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Mon, 5 Dec 2016 23:05:09 -0800 Subject: [PATCH 02/13] first part of refactoring. --- include/envoy/http/access_log.h | 14 +++--- .../http/access_log/access_log_formatter.cc | 34 +++++++------- .../http/access_log/request_info_impl.h | 8 ++-- source/common/http/conn_manager_utility.cc | 2 +- source/common/http/filter/fault_filter.cc | 2 +- source/common/router/router.cc | 30 ++++++------- source/common/router/router.h | 4 +- source/common/tracing/http_tracer_impl.cc | 4 +- source/server/http/health_check.cc | 8 ++-- .../access_log/access_log_formatter_test.cc | 44 +++++++++---------- .../http/access_log/access_log_impl_test.cc | 8 ++-- test/common/http/conn_manager_utility_test.cc | 8 ++-- test/common/http/filter/fault_filter_test.cc | 12 ++--- test/common/router/router_test.cc | 18 ++++---- test/common/tracing/http_tracer_impl_test.cc | 12 ++--- test/mocks/http/mocks.h | 4 +- test/server/http/health_check_test.cc | 6 +-- 17 files changed, 109 insertions(+), 109 deletions(-) diff --git a/include/envoy/http/access_log.h b/include/envoy/http/access_log.h index 54369caf2bc1c..c7435ecc074d3 100644 --- a/include/envoy/http/access_log.h +++ b/include/envoy/http/access_log.h @@ -10,7 +10,7 @@ namespace Http { namespace AccessLog { -enum FailureReason { +enum ResponseFlag { // No failure. None = 0x0, // Local server healthcheck failed. @@ -45,10 +45,9 @@ class RequestInfo { virtual ~RequestInfo() {} /** - * filter can trigger this callback on failed response to provide more details about - * failure. + * filter can set response flags. */ - virtual void onFailedResponse(FailureReason failure_reason) PURE; + virtual void setResponseFlag(ResponseFlag response_flag) PURE; /** * filter can trigger this callback when an upstream host has been selected. @@ -91,10 +90,11 @@ class RequestInfo { virtual std::chrono::milliseconds duration() const PURE; /** - * @return the failure reason for richer log experience, this can be represented as binary OR of - * FailureReason enum values. + * @return response flags to enrich access log with details around response code. Response flag + * complements response + * code and add details to it. */ - virtual uint64_t failureReason() const PURE; + virtual uint64_t getResponseFlags() const PURE; /** * @return upstream host description. diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index d41c936e86218..faf451ee45a99 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -27,59 +27,59 @@ void FilterReasonUtils::appendString(std::string& result, const std::string& app } } -const std::string FilterReasonUtils::toShortString(uint64_t failure_reason) { +const std::string FilterReasonUtils::toShortString(uint64_t response_flags) { std::string result = ""; - if (failure_reason == FailureReason::None) { + if (response_flags == ResponseFlag::None) { return NONE; } - if (failure_reason & FailureReason::FailedLocalHealthCheck) { + if (response_flags & ResponseFlag::FailedLocalHealthCheck) { appendString(result, FAILED_LOCAL_HEALTH_CHECK); } - if (failure_reason & FailureReason::NoHealthyUpstream) { + if (response_flags & ResponseFlag::NoHealthyUpstream) { appendString(result, NO_HEALTHY_UPSTREAM); } - if (failure_reason & FailureReason::UpstreamRequestTimeout) { + if (response_flags & ResponseFlag::UpstreamRequestTimeout) { appendString(result, UPSTREAM_REQUEST_TIMEOUT); } - if (failure_reason & FailureReason::LocalReset) { + if (response_flags & ResponseFlag::LocalReset) { appendString(result, LOCAL_RESET); } - if (failure_reason & FailureReason::UpstreamRemoteReset) { + if (response_flags & ResponseFlag::UpstreamRemoteReset) { appendString(result, UPSTREAM_REMOTE_RESET); } - if (failure_reason & FailureReason::UpstreamConnectionFailure) { + if (response_flags & ResponseFlag::UpstreamConnectionFailure) { appendString(result, UPSTREAM_CONNECTION_FAILURE); } - if (failure_reason & FailureReason::UpstreamConnectionTermination) { + if (response_flags & ResponseFlag::UpstreamConnectionTermination) { appendString(result, UPSTREAM_CONNECTION_TERMINATION); } - if (failure_reason & FailureReason::UpstreamOverflow) { + if (response_flags & ResponseFlag::UpstreamOverflow) { appendString(result, UPSTREAM_OVERFLOW); } - if (failure_reason & FailureReason::NoRouteFound) { + if (response_flags & ResponseFlag::NoRouteFound) { appendString(result, NO_ROUTE_FOUND); } - if (failure_reason & FailureReason::FaultInjected) { + if (response_flags & ResponseFlag::FaultInjected) { appendString(result, FAULT_INJECTED); } - if (failure_reason & FailureReason::DelayInjected) { + if (response_flags & ResponseFlag::DelayInjected) { appendString(result, DELAY_INJECTED); } if (result.empty()) { - throw std::invalid_argument(fmt::format("Unknown failure_reason code {}", failure_reason)); + throw std::invalid_argument(fmt::format("unknown response flags: {}", response_flags)); } return result; @@ -87,7 +87,7 @@ const std::string FilterReasonUtils::toShortString(uint64_t failure_reason) { const std::string AccessLogFormatUtils::DEFAULT_FORMAT = "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%\" " - "%RESPONSE_CODE% %FAILURE_REASON% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% " + "%RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% " "%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% " "\"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" " "\"%REQ(:AUTHORITY)%\" \"%UPSTREAM_HOST%\"\n"; @@ -248,9 +248,9 @@ RequestInfoFormatter::RequestInfoFormatter(const std::string& field_name) { field_extractor_ = [](const RequestInfo& request_info) { return std::to_string(request_info.duration().count()); }; - } else if (field_name == "FAILURE_REASON") { + } else if (field_name == "RESPONSE_FLAG") { field_extractor_ = [](const RequestInfo& request_info) { - return FilterReasonUtils::toShortString(request_info.failureReason()); + return FilterReasonUtils::toShortString(request_info.getResponseFlags()); }; } else if (field_name == "UPSTREAM_HOST") { field_extractor_ = [](const RequestInfo& request_info) { diff --git a/source/common/http/access_log/request_info_impl.h b/source/common/http/access_log/request_info_impl.h index 5114f60bf7b16..5ba55966a8080 100644 --- a/source/common/http/access_log/request_info_impl.h +++ b/source/common/http/access_log/request_info_impl.h @@ -26,11 +26,11 @@ struct RequestInfoImpl : public RequestInfo { start_time_); } - void onFailedResponse(Http::AccessLog::FailureReason failure_reason) override { - failure_reason_ |= failure_reason; + void setResponseFlag(Http::AccessLog::ResponseFlag response_flag) override { + response_flags_ |= response_flag; } - uint64_t failureReason() const override { return failure_reason_; } + uint64_t getResponseFlags() const override { return response_flags_; } void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) override { upstream_host_ = host; } @@ -45,7 +45,7 @@ struct RequestInfoImpl : public RequestInfo { uint64_t bytes_received_{}; Optional response_code_; uint64_t bytes_sent_{}; - uint64_t failure_reason_{FailureReason::None}; + uint64_t response_flags_{ResponseFlag::None}; Upstream::HostDescriptionPtr upstream_host_{}; bool hc_request_{}; }; diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 6168497161720..c686b41b445d3 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -148,7 +148,7 @@ bool ConnectionManagerUtility::shouldTraceRequest( case Http::TracingType::All: return true; case Http::TracingType::UpstreamFailure: - return request_info.failureReason() != Http::AccessLog::FailureReason::None; + return request_info.getResponseFlags() != Http::AccessLog::ResponseFlag::None; } // Compiler enforces switch above to cover all the cases and it's impossible to be here, diff --git a/source/common/http/filter/fault_filter.cc b/source/common/http/filter/fault_filter.cc index 348abf505fcee..7f1ee24cad637 100644 --- a/source/common/http/filter/fault_filter.cc +++ b/source/common/http/filter/fault_filter.cc @@ -137,7 +137,7 @@ void FaultFilter::abortWithHTTPStatus() { "fault.http.abort.http_status", config_->abortCode()))}}}; callbacks_->encodeHeaders(std::move(response_headers), true); config_->stats().aborts_injected_.inc(); - callbacks_->requestInfo().onFailedResponse(Http::AccessLog::FailureReason::FaultInjected); + callbacks_->requestInfo().setResponseFlag(Http::AccessLog::ResponseFlag::FaultInjected); } void FaultFilter::resetTimerState() { diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 1a49b9d55c40b..d9de640291eff 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -153,7 +153,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e config_.stats_.no_route_.inc(); stream_log_debug("no cluster match for URL '{}'", *callbacks_, headers.Path()->value().c_str()); - callbacks_->requestInfo().onFailedResponse(Http::AccessLog::FailureReason::NoRouteFound); + callbacks_->requestInfo().setResponseFlag(Http::AccessLog::ResponseFlag::NoRouteFound); Http::HeaderMapPtr response_headers{new Http::HeaderMapImpl{ {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::NotFound))}}}; callbacks_->encodeHeaders(std::move(response_headers), true); @@ -180,7 +180,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e // See if we are supposed to immediately kill some percentage of this cluster's traffic. if (cluster_->maintenanceMode()) { - callbacks_->requestInfo().onFailedResponse(Http::AccessLog::FailureReason::UpstreamOverflow); + callbacks_->requestInfo().setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamOverflow); chargeUpstreamCode(Http::Code::ServiceUnavailable); Http::Utility::sendLocalReply(*callbacks_, Http::Code::ServiceUnavailable, "maintenance mode"); return Http::FilterHeadersStatus::StopIteration; @@ -226,7 +226,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e } void Filter::sendNoHealthyUpstreamResponse() { - callbacks_->requestInfo().onFailedResponse(Http::AccessLog::FailureReason::NoHealthyUpstream); + callbacks_->requestInfo().setResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream); chargeUpstreamCode(Http::Code::ServiceUnavailable); Http::Utility::sendLocalReply(*callbacks_, Http::Code::ServiceUnavailable, "no healthy upstream"); } @@ -360,15 +360,15 @@ void Filter::onUpstreamReset(UpstreamResetType type, Http::Code code; const char* body; if (type == UpstreamResetType::GlobalTimeout || type == UpstreamResetType::PerTryTimeout) { - callbacks_->requestInfo().onFailedResponse( - Http::AccessLog::FailureReason::UpstreamRequestTimeout); + callbacks_->requestInfo().setResponseFlag( + Http::AccessLog::ResponseFlag::UpstreamRequestTimeout); code = Http::Code::GatewayTimeout; body = "upstream request timeout"; } else { - Http::AccessLog::FailureReason failure_reason = - streamResetReasonToFailureReason(reset_reason.value()); - callbacks_->requestInfo().onFailedResponse(failure_reason); + Http::AccessLog::ResponseFlag failure_reason = + streamResetReasonToResponseFlag(reset_reason.value()); + callbacks_->requestInfo().setResponseFlag(failure_reason); code = Http::Code::ServiceUnavailable; body = "upstream connect error or disconnect/reset before headers"; } @@ -378,21 +378,21 @@ void Filter::onUpstreamReset(UpstreamResetType type, } } -Http::AccessLog::FailureReason -Filter::streamResetReasonToFailureReason(Http::StreamResetReason reset_reason) { +Http::AccessLog::ResponseFlag +Filter::streamResetReasonToResponseFlag(Http::StreamResetReason reset_reason) { switch (reset_reason) { case Http::StreamResetReason::ConnectionFailure: - return Http::AccessLog::FailureReason::UpstreamConnectionFailure; + return Http::AccessLog::ResponseFlag::UpstreamConnectionFailure; case Http::StreamResetReason::ConnectionTermination: - return Http::AccessLog::FailureReason::UpstreamConnectionTermination; + return Http::AccessLog::ResponseFlag::UpstreamConnectionTermination; case Http::StreamResetReason::LocalReset: case Http::StreamResetReason::LocalRefusedStreamReset: - return Http::AccessLog::FailureReason::LocalReset; + return Http::AccessLog::ResponseFlag::LocalReset; case Http::StreamResetReason::Overflow: - return Http::AccessLog::FailureReason::UpstreamOverflow; + return Http::AccessLog::ResponseFlag::UpstreamOverflow; case Http::StreamResetReason::RemoteReset: case Http::StreamResetReason::RemoteRefusedStreamReset: - return Http::AccessLog::FailureReason::UpstreamRemoteReset; + return Http::AccessLog::ResponseFlag::UpstreamRemoteReset; } throw std::invalid_argument("Unknown reset_reason"); diff --git a/source/common/router/router.h b/source/common/router/router.h index 7bd1255cae1f9..c003f36997172 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -167,8 +167,8 @@ class Filter : Logger::Loggable, public Http::StreamDecoderF enum class UpstreamResetType { Reset, GlobalTimeout, PerTryTimeout }; - Http::AccessLog::FailureReason - streamResetReasonToFailureReason(Http::StreamResetReason reset_reason); + Http::AccessLog::ResponseFlag + streamResetReasonToResponseFlag(Http::StreamResetReason reset_reason); const std::string& upstreamZone(); void chargeUpstreamCode(const Http::HeaderMap& response_headers); diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 8ff34a50159d3..ef72da986db65 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -264,9 +264,9 @@ void LightStepSink::flushTrace(const Http::HeaderMap& request_headers, const Htt span.SetTag("error", "true"); } - if (request_info.failureReason() != Http::AccessLog::FailureReason::None) { + if (request_info.getResponseFlags() != Http::AccessLog::ResponseFlag::None) { span.SetTag("failure reason", - Http::AccessLog::FilterReasonUtils::toShortString(request_info.failureReason())); + Http::AccessLog::FilterReasonUtils::toShortString(request_info.getResponseFlags())); } if (request_headers.ClientTraceId()) { diff --git a/source/server/http/health_check.cc b/source/server/http/health_check.cc index 17a9ede98e4ab..9a81504e6daab 100644 --- a/source/server/http/health_check.cc +++ b/source/server/http/health_check.cc @@ -125,8 +125,8 @@ void HealthCheckFilter::onComplete() { ASSERT(handling_); Http::HeaderMapPtr headers; if (server_.healthCheckFailed()) { - callbacks_->requestInfo().onFailedResponse( - Http::AccessLog::FailureReason::FailedLocalHealthCheck); + callbacks_->requestInfo().setResponseFlag( + Http::AccessLog::ResponseFlag::FailedLocalHealthCheck); headers.reset(new Http::HeaderMapImpl{ {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::ServiceUnavailable))}}); } else { @@ -136,8 +136,8 @@ void HealthCheckFilter::onComplete() { } if (!Http::CodeUtility::is2xx(enumToInt(final_status))) { - callbacks_->requestInfo().onFailedResponse( - Http::AccessLog::FailureReason::FailedLocalHealthCheck); + callbacks_->requestInfo().setResponseFlag( + Http::AccessLog::ResponseFlag::FailedLocalHealthCheck); } headers.reset(new Http::HeaderMapImpl{ diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index 66889c9302b34..105077621c838 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -12,30 +12,30 @@ namespace Http { namespace AccessLog { TEST(FailureReasonUtilsTest, toShortStringConversion) { - std::vector> expected = { - std::make_pair(FailureReason::None, "-"), - std::make_pair(FailureReason::FailedLocalHealthCheck, "LH"), - std::make_pair(FailureReason::NoHealthyUpstream, "UH"), - std::make_pair(FailureReason::UpstreamRequestTimeout, "UT"), - std::make_pair(FailureReason::LocalReset, "LR"), - std::make_pair(FailureReason::UpstreamRemoteReset, "UR"), - std::make_pair(FailureReason::UpstreamConnectionFailure, "UF"), - std::make_pair(FailureReason::UpstreamConnectionTermination, "UC"), - std::make_pair(FailureReason::UpstreamOverflow, "UO"), - std::make_pair(FailureReason::NoRouteFound, "NR"), - std::make_pair(FailureReason::FaultInjected, "FI"), - std::make_pair(FailureReason::DelayInjected, "DI")}; + std::vector> expected = { + std::make_pair(ResponseFlag::None, "-"), + std::make_pair(ResponseFlag::FailedLocalHealthCheck, "LH"), + std::make_pair(ResponseFlag::NoHealthyUpstream, "UH"), + std::make_pair(ResponseFlag::UpstreamRequestTimeout, "UT"), + std::make_pair(ResponseFlag::LocalReset, "LR"), + std::make_pair(ResponseFlag::UpstreamRemoteReset, "UR"), + std::make_pair(ResponseFlag::UpstreamConnectionFailure, "UF"), + std::make_pair(ResponseFlag::UpstreamConnectionTermination, "UC"), + std::make_pair(ResponseFlag::UpstreamOverflow, "UO"), + std::make_pair(ResponseFlag::NoRouteFound, "NR"), + std::make_pair(ResponseFlag::FaultInjected, "FI"), + std::make_pair(ResponseFlag::DelayInjected, "DI")}; for (const auto& testCase : expected) { EXPECT_EQ(testCase.second, FilterReasonUtils::toShortString(testCase.first)); } // Test combinations. - EXPECT_EQ("UT,DI", FilterReasonUtils::toShortString(FailureReason::DelayInjected | - FailureReason::UpstreamRequestTimeout)); - EXPECT_EQ("UT,FI,DI", FilterReasonUtils::toShortString(FailureReason::FaultInjected | - FailureReason::DelayInjected | - FailureReason::UpstreamRequestTimeout)); + EXPECT_EQ("UT,DI", FilterReasonUtils::toShortString(ResponseFlag::DelayInjected | + ResponseFlag::UpstreamRequestTimeout)); + EXPECT_EQ("UT,FI,DI", FilterReasonUtils::toShortString(ResponseFlag::FaultInjected | + ResponseFlag::DelayInjected | + ResponseFlag::UpstreamRequestTimeout)); } TEST(AccessLogFormatUtilsTest, protocolToString) { @@ -106,10 +106,10 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { } { - RequestInfoFormatter failure_reason_format("FAILURE_REASON"); - FailureReason reason = FailureReason::LocalReset; - EXPECT_CALL(requestInfo, failureReason()).WillOnce(Return(reason)); - EXPECT_EQ("LR", failure_reason_format.format(header, header, requestInfo)); + RequestInfoFormatter response_flags_format("RESPONSE_FLAGS"); + uint64_t response_flags = ResponseFlag::LocalReset; + EXPECT_CALL(requestInfo, getResponseFlags()).WillOnce(Return(response_flags)); + EXPECT_EQ("LR", response_flags_format.format(header, header, requestInfo)); } { diff --git a/test/common/http/access_log/access_log_impl_test.cc b/test/common/http/access_log/access_log_impl_test.cc index 81525e084d638..fa6a1ae7a5966 100644 --- a/test/common/http/access_log/access_log_impl_test.cc +++ b/test/common/http/access_log/access_log_impl_test.cc @@ -43,8 +43,8 @@ class TestRequestInfo : public RequestInfo { std::chrono::milliseconds duration() const override { return std::chrono::milliseconds(duration_); } - uint64_t failureReason() const override { return failure_reason_; } - void onFailedResponse(FailureReason failure_reason) override { failure_reason_ = failure_reason; } + uint64_t getResponseFlags() const override { return response_flags_; } + void setResponseFlag(ResponseFlag response_flag) override { response_flags_ = response_flag; } void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) override { upstream_host_ = host; } Upstream::HostDescriptionPtr upstreamHost() const override { return upstream_host_; } bool healthCheck() const override { return hc_request_; } @@ -53,7 +53,7 @@ class TestRequestInfo : public RequestInfo { SystemTime start_time_; Protocol protocol_{Protocol::Http11}; Optional response_code_; - uint64_t failure_reason_{FailureReason::None}; + uint64_t response_flags_{ResponseFlag::None}; uint64_t duration_{3}; Upstream::HostDescriptionPtr upstream_host_{}; bool hc_request_{}; @@ -86,7 +86,7 @@ TEST_F(AccessLogImplTest, LogMoreData) { InstancePtr log = InstanceImpl::fromJson(*loader, runtime_, log_manager_); EXPECT_CALL(*file_, write(_)); - request_info_.failure_reason_ = FailureReason::UpstreamConnectionFailure; + request_info_.response_flags_ = ResponseFlag::UpstreamConnectionFailure; request_headers_.addViaCopy(Http::Headers::get().UserAgent, "user-agent-set"); request_headers_.addViaCopy(Http::Headers::get().RequestId, "id"); request_headers_.addViaCopy(Http::Headers::get().Host, "host"); diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index c7387bf5c3498..363dbc2bfd450 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -61,12 +61,12 @@ TEST_F(ConnectionManagerUtilityTest, ShouldTraceRequest) { NiceMock request_info; Optional tracing_failure( {"operation", Http::TracingType::UpstreamFailure}); - EXPECT_CALL(request_info, failureReason()) - .WillOnce(Return(Http::AccessLog::FailureReason::UpstreamConnectionFailure)); + EXPECT_CALL(request_info, getResponseFlags()) + .WillOnce(Return(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)); EXPECT_TRUE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); - EXPECT_CALL(request_info, failureReason()) - .WillOnce(Return(Http::AccessLog::FailureReason::None)); + EXPECT_CALL(request_info, getResponseFlags()) + .WillOnce(Return(Http::AccessLog::ResponseFlag::None)); EXPECT_FALSE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); } } diff --git a/test/common/http/filter/fault_filter_test.cc b/test/common/http/filter/fault_filter_test.cc index 669c9d7a54f99..421bd48cf49b2 100644 --- a/test/common/http/filter/fault_filter_test.cc +++ b/test/common/http/filter/fault_filter_test.cc @@ -214,7 +214,7 @@ TEST_F(FaultFilterTest, AbortWithHttpStatus) { EXPECT_CALL(filter_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); EXPECT_CALL(filter_callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::FaultInjected)); + setResponseFlag(Http::AccessLog::ResponseFlag::FaultInjected)); EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); EXPECT_EQ(FilterDataStatus::Continue, filter_->decodeData(data_, false)); @@ -244,7 +244,7 @@ TEST_F(FaultFilterTest, FixedDelayZeroDuration) { EXPECT_CALL(runtime_.snapshot_, getInteger("fault.http.abort.http_status", _)).Times(0); EXPECT_CALL(filter_callbacks_, encodeHeaders_(_, _)).Times(0); - EXPECT_CALL(filter_callbacks_.request_info_, onFailedResponse(_)).Times(0); + EXPECT_CALL(filter_callbacks_.request_info_, setResponseFlag(_)).Times(0); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); // Expect filter to continue execution when delay is 0ms @@ -281,7 +281,7 @@ TEST_F(FaultFilterTest, FixedDelayNonZeroDuration) { // Delay only case EXPECT_CALL(runtime_.snapshot_, getInteger("fault.http.abort.http_status", _)).Times(0); EXPECT_CALL(filter_callbacks_, encodeHeaders_(_, _)).Times(0); - EXPECT_CALL(filter_callbacks_.request_info_, onFailedResponse(_)).Times(0); + EXPECT_CALL(filter_callbacks_.request_info_, setResponseFlag(_)).Times(0); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(1); timer_->callback_(); @@ -322,7 +322,7 @@ TEST_F(FaultFilterTest, FixedDelayAndAbort) { EXPECT_CALL(filter_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); EXPECT_CALL(filter_callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::FaultInjected)); + setResponseFlag(Http::AccessLog::ResponseFlag::FaultInjected)); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -366,7 +366,7 @@ TEST_F(FaultFilterTest, FixedDelayAndAbortHeaderMatchSuccess) { Http::TestHeaderMapImpl response_headers{{":status", "503"}}; EXPECT_CALL(filter_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); EXPECT_CALL(filter_callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::FaultInjected)); + setResponseFlag(Http::AccessLog::ResponseFlag::FaultInjected)); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); @@ -390,7 +390,7 @@ TEST_F(FaultFilterTest, FixedDelayAndAbortHeaderMatchFail) { EXPECT_CALL(runtime_.snapshot_, featureEnabled("fault.http.abort.abort_percent", _)).Times(0); EXPECT_CALL(runtime_.snapshot_, getInteger("fault.http.abort.http_status", _)).Times(0); EXPECT_CALL(filter_callbacks_, encodeHeaders_(_, _)).Times(0); - EXPECT_CALL(filter_callbacks_.request_info_, onFailedResponse(_)).Times(0); + EXPECT_CALL(filter_callbacks_.request_info_, setResponseFlag(_)).Times(0); EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); // Expect filter to continue execution when headers don't match diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 5e632babdd87b..750d8d6e44030 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -76,7 +76,7 @@ class RouterTest : public testing::Test { TEST_F(RouterTest, RouteNotFound) { EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::NoRouteFound)); + setResponseFlag(Http::AccessLog::ResponseFlag::NoRouteFound)); Http::TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); @@ -105,7 +105,7 @@ TEST_F(RouterTest, PoolFailureWithPriority) { EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); EXPECT_CALL(callbacks_, encodeData(_, true)); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::UpstreamConnectionFailure)); + setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)); EXPECT_CALL(callbacks_.request_info_, onUpstreamHostSelected(_)) .WillOnce(Invoke([&](const Upstream::HostDescriptionPtr host) -> void { EXPECT_EQ(host_url_, host->url()); })); @@ -136,7 +136,7 @@ TEST_F(RouterTest, NoHost) { EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); EXPECT_CALL(callbacks_, encodeData(_, true)); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::NoHealthyUpstream)); + setResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream)); Http::TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); @@ -151,7 +151,7 @@ TEST_F(RouterTest, MaintenanceMode) { EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); EXPECT_CALL(callbacks_, encodeData(_, true)); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::UpstreamOverflow)); + setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamOverflow)); Http::TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); @@ -202,7 +202,7 @@ TEST_F(RouterTest, UpstreamTimeout) { router_.decodeData(data, true); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::UpstreamRequestTimeout)); + setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout)); EXPECT_CALL(encoder.stream_, resetStream(Http::StreamResetReason::LocalReset)); Http::TestHeaderMapImpl response_headers{ {":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}}; @@ -240,7 +240,7 @@ TEST_F(RouterTest, UpstreamPerTryTimeout) { router_.decodeData(data, true); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::UpstreamRequestTimeout)); + setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout)); EXPECT_CALL(encoder.stream_, resetStream(Http::StreamResetReason::LocalReset)); Http::TestHeaderMapImpl response_headers{ {":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}}; @@ -264,7 +264,7 @@ TEST_F(RouterTest, RetryRequestNotComplete) { return nullptr; })); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::UpstreamRemoteReset)); + setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRemoteReset)); EXPECT_CALL(callbacks_.request_info_, onUpstreamHostSelected(_)) .WillOnce(Invoke([&](const Upstream::HostDescriptionPtr host) -> void { EXPECT_EQ(host_url_, host->url()); })); @@ -306,7 +306,7 @@ TEST_F(RouterTest, RetryNoneHealthy) { EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); EXPECT_CALL(callbacks_, encodeData(_, true)); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::NoHealthyUpstream)); + setResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream)); router_.retry_state_->callback_(); } @@ -472,7 +472,7 @@ TEST_F(RouterTest, RetryTimeoutDuringRetryDelay) { // Fire timeout. EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::UpstreamRequestTimeout)); + setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout)); EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(504)); EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putResponseTime(_)).Times(0); Http::TestHeaderMapImpl response_headers{ diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index b4c3119a6b49e..86497aa29050d 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -408,8 +408,8 @@ TEST_F(LightStepSinkTest, FlushSeveralSpans) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, failureReason()) - .WillByDefault(Return(Http::AccessLog::FailureReason::None)); + ON_CALL(request_info, getResponseFlags()) + .WillByDefault(Return(Http::AccessLog::ResponseFlag::None)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); @@ -479,8 +479,8 @@ TEST_F(LightStepSinkTest, FlushSpansTimer) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, failureReason()) - .WillByDefault(Return(Http::AccessLog::FailureReason::None)); + ON_CALL(request_info, getResponseFlags()) + .WillByDefault(Return(Http::AccessLog::ResponseFlag::None)); const Optional timeout(std::chrono::seconds(5)); EXPECT_CALL(cm_.async_client_, send_(_, _, timeout)); @@ -516,8 +516,8 @@ TEST_F(LightStepSinkTest, FlushOneSpanGrpcFailure) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, failureReason()) - .WillByDefault(Return(Http::AccessLog::FailureReason::None)); + ON_CALL(request_info, getResponseFlags()) + .WillByDefault(Return(Http::AccessLog::ResponseFlag::None)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index a4c4eae2b3752..3760b0afcefbb 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -33,7 +33,7 @@ class MockRequestInfo : public RequestInfo { ~MockRequestInfo(); // Http::AccessLog::RequestInfo - MOCK_METHOD1(onFailedResponse, void(FailureReason failure_reason)); + MOCK_METHOD1(setResponseFlag, void(ResponseFlag response_flag)); MOCK_METHOD1(onUpstreamHostSelected, void(Upstream::HostDescriptionPtr host)); MOCK_CONST_METHOD0(startTime, SystemTime()); MOCK_CONST_METHOD0(bytesReceived, uint64_t()); @@ -42,7 +42,7 @@ class MockRequestInfo : public RequestInfo { MOCK_CONST_METHOD0(responseCode, Optional&()); MOCK_CONST_METHOD0(bytesSent, uint64_t()); MOCK_CONST_METHOD0(duration, std::chrono::milliseconds()); - MOCK_CONST_METHOD0(failureReason, uint64_t()); + MOCK_CONST_METHOD0(getResponseFlags, uint64_t()); MOCK_CONST_METHOD0(upstreamHost, Upstream::HostDescriptionPtr()); MOCK_CONST_METHOD0(healthCheck, bool()); MOCK_METHOD1(healthCheck, void(bool is_hc)); diff --git a/test/server/http/health_check_test.cc b/test/server/http/health_check_test.cc index daca189202970..b70a7b59e2c07 100644 --- a/test/server/http/health_check_test.cc +++ b/test/server/http/health_check_test.cc @@ -83,7 +83,7 @@ TEST_F(HealthCheckFilterNoPassThroughTest, HealthCheckFailedCallbackCalled) { })); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::FailedLocalHealthCheck)); + setResponseFlag(Http::AccessLog::ResponseFlag::FailedLocalHealthCheck)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); @@ -133,7 +133,7 @@ TEST_F(HealthCheckFilterCachingTest, CachedServiceUnavailableCallbackCalled) { })); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::FailedLocalHealthCheck)); + setResponseFlag(Http::AccessLog::ResponseFlag::FailedLocalHealthCheck)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); @@ -170,7 +170,7 @@ TEST_F(HealthCheckFilterCachingTest, All) { // Verify that the next request uses the cached value. prepareFilter(true); EXPECT_CALL(callbacks_.request_info_, - onFailedResponse(Http::AccessLog::FailureReason::FailedLocalHealthCheck)); + setResponseFlag(Http::AccessLog::ResponseFlag::FailedLocalHealthCheck)); EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&health_check_response), true)) .Times(1) .WillRepeatedly(Invoke([&](Http::HeaderMap& headers, bool end_stream) { From 3bfb52222873f02b85438bacac518cdda1e66823 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Tue, 6 Dec 2016 13:25:15 -0800 Subject: [PATCH 03/13] Fix. --- configs/access_log_format_helper.template.json | 8 ++++---- docs/configuration/http_conn_man/access_log.rst | 8 ++++---- examples/grpc-bridge/config/s2s-python-envoy.json | 2 +- source/common/http/access_log/access_log_formatter.cc | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/configs/access_log_format_helper.template.json b/configs/access_log_format_helper.template.json index 7c11c9a2eba36..710f8aa90912c 100644 --- a/configs/access_log_format_helper.template.json +++ b/configs/access_log_format_helper.template.json @@ -1,15 +1,15 @@ {% macro ingress_sampled_log() %} - "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %FAILURE_REASON% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\"\n" + "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\"\n" {% endmacro %} {% macro ingress_full() %} - "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%\" %RESPONSE_CODE% %FAILURE_REASON% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\"\n" + "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%\" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\"\n" {% endmacro %} {% macro egress_error_log() %} - "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %FAILURE_REASON% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\" \"%UPSTREAM_HOST%\"\n" + "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\" \"%UPSTREAM_HOST%\"\n" {% endmacro %} {% macro egress_error_amazon_service() %} - "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %FAILURE_REASON% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\" \"%UPSTREAM_HOST%\" \"%RESP(X-AMZN-RequestId)%\"\n" + "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\" \"%UPSTREAM_HOST%\" \"%RESP(X-AMZN-RequestId)%\"\n" {% endmacro %} diff --git a/docs/configuration/http_conn_man/access_log.rst b/docs/configuration/http_conn_man/access_log.rst index 185393411423c..2db355749d630 100644 --- a/docs/configuration/http_conn_man/access_log.rst +++ b/docs/configuration/http_conn_man/access_log.rst @@ -71,8 +71,8 @@ The following command operators are supported: %DURATION% Total duration in milliseconds of the request from the start time to the last byte out. -%FAILURE_REASON% - Additional failure reason if any in addition to response code. Possible values are: +%RESPONSE_FLAGS% + Additional details to response code if any. Possible values are: * **LH**: Local service failed :ref:`health check request ` in addition to 503 response code. * **UH**: No healthy upstream hosts in upstream cluster in addition to 503 response code. @@ -83,7 +83,7 @@ The following command operators are supported: * **UC**: Upstream connection termination in addition to 503 response code. * **UO**: Upstream overflow (:ref:`circuit breaking `) in addition to 503 response code. * **NR**: No :ref:`route configured ` for a given request in addition to 404 response code. - * **FI**: The request was aborted as a result of :ref:`fault injection `. + * **FI**: The request was aborted with injected response code by :ref:`fault injection `. %UPSTREAM_HOST% Upstream host URL (e.g., tcp://ip:port for TCP connections). @@ -107,7 +107,7 @@ If custom format is not specified, Envoy uses the following default format: .. code-block:: none [%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%" - %RESPONSE_CODE% %FAILURE_REASON% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% + %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%"\n diff --git a/examples/grpc-bridge/config/s2s-python-envoy.json b/examples/grpc-bridge/config/s2s-python-envoy.json index 02430e45866d7..6bfeb15de5d02 100644 --- a/examples/grpc-bridge/config/s2s-python-envoy.json +++ b/examples/grpc-bridge/config/s2s-python-envoy.json @@ -13,7 +13,7 @@ "access_log": [ { "path": "/var/log/envoy/egress_http.log", - "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %FAILURE_REASON% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\" \"%UPSTREAM_HOST%\"\n" + "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\" \"%UPSTREAM_HOST%\"\n" } ], "stat_prefix": "egress_http", diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index faf451ee45a99..ce803f5a3a710 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -248,7 +248,7 @@ RequestInfoFormatter::RequestInfoFormatter(const std::string& field_name) { field_extractor_ = [](const RequestInfo& request_info) { return std::to_string(request_info.duration().count()); }; - } else if (field_name == "RESPONSE_FLAG") { + } else if (field_name == "RESPONSE_FLAGS") { field_extractor_ = [](const RequestInfo& request_info) { return FilterReasonUtils::toShortString(request_info.getResponseFlags()); }; From ba7efd5595b0f7f76d14ed2c905786a9e285afa5 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Tue, 6 Dec 2016 14:26:39 -0800 Subject: [PATCH 04/13] fix tests. --- include/envoy/http/access_log.h | 2 +- .../http/access_log/access_log_formatter.cc | 32 ++++++++--------- .../http/access_log/access_log_formatter.h | 2 +- .../http/access_log/request_info_impl.h | 8 ++++- source/common/http/conn_manager_utility.cc | 2 +- source/common/tracing/http_tracer_impl.cc | 5 ++- .../access_log/access_log_formatter_test.cc | 34 ++++++++++++++----- .../http/access_log/access_log_impl_test.cc | 4 ++- test/common/http/conn_manager_utility_test.cc | 9 ++--- test/common/tracing/http_tracer_impl_test.cc | 12 +++---- test/mocks/http/mocks.h | 2 +- 11 files changed, 66 insertions(+), 46 deletions(-) diff --git a/include/envoy/http/access_log.h b/include/envoy/http/access_log.h index c7435ecc074d3..8607c16c808b7 100644 --- a/include/envoy/http/access_log.h +++ b/include/envoy/http/access_log.h @@ -94,7 +94,7 @@ class RequestInfo { * complements response * code and add details to it. */ - virtual uint64_t getResponseFlags() const PURE; + virtual bool isSetResponseFlag(ResponseFlag response_flag) const PURE; /** * @return upstream host description. diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index ce803f5a3a710..2b8f42fc627ce 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -27,61 +27,57 @@ void FilterReasonUtils::appendString(std::string& result, const std::string& app } } -const std::string FilterReasonUtils::toShortString(uint64_t response_flags) { +const std::string FilterReasonUtils::toShortString(const RequestInfo& request_info) { std::string result = ""; - if (response_flags == ResponseFlag::None) { + if (request_info.isSetResponseFlag(ResponseFlag::None)) { return NONE; } - if (response_flags & ResponseFlag::FailedLocalHealthCheck) { + if (request_info.isSetResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { appendString(result, FAILED_LOCAL_HEALTH_CHECK); } - if (response_flags & ResponseFlag::NoHealthyUpstream) { + if (request_info.isSetResponseFlag(ResponseFlag::NoHealthyUpstream)) { appendString(result, NO_HEALTHY_UPSTREAM); } - if (response_flags & ResponseFlag::UpstreamRequestTimeout) { + if (request_info.isSetResponseFlag(ResponseFlag::UpstreamRequestTimeout)) { appendString(result, UPSTREAM_REQUEST_TIMEOUT); } - if (response_flags & ResponseFlag::LocalReset) { + if (request_info.isSetResponseFlag(ResponseFlag::LocalReset)) { appendString(result, LOCAL_RESET); } - if (response_flags & ResponseFlag::UpstreamRemoteReset) { + if (request_info.isSetResponseFlag(ResponseFlag::UpstreamRemoteReset)) { appendString(result, UPSTREAM_REMOTE_RESET); } - if (response_flags & ResponseFlag::UpstreamConnectionFailure) { + if (request_info.isSetResponseFlag(ResponseFlag::UpstreamConnectionFailure)) { appendString(result, UPSTREAM_CONNECTION_FAILURE); } - if (response_flags & ResponseFlag::UpstreamConnectionTermination) { + if (request_info.isSetResponseFlag(ResponseFlag::UpstreamConnectionTermination)) { appendString(result, UPSTREAM_CONNECTION_TERMINATION); } - if (response_flags & ResponseFlag::UpstreamOverflow) { + if (request_info.isSetResponseFlag(ResponseFlag::UpstreamOverflow)) { appendString(result, UPSTREAM_OVERFLOW); } - if (response_flags & ResponseFlag::NoRouteFound) { + if (request_info.isSetResponseFlag(ResponseFlag::NoRouteFound)) { appendString(result, NO_ROUTE_FOUND); } - if (response_flags & ResponseFlag::FaultInjected) { + if (request_info.isSetResponseFlag(ResponseFlag::FaultInjected)) { appendString(result, FAULT_INJECTED); } - if (response_flags & ResponseFlag::DelayInjected) { + if (request_info.isSetResponseFlag(ResponseFlag::DelayInjected)) { appendString(result, DELAY_INJECTED); } - if (result.empty()) { - throw std::invalid_argument(fmt::format("unknown response flags: {}", response_flags)); - } - return result; } @@ -250,7 +246,7 @@ RequestInfoFormatter::RequestInfoFormatter(const std::string& field_name) { }; } else if (field_name == "RESPONSE_FLAGS") { field_extractor_ = [](const RequestInfo& request_info) { - return FilterReasonUtils::toShortString(request_info.getResponseFlags()); + return FilterReasonUtils::toShortString(request_info); }; } else if (field_name == "UPSTREAM_HOST") { field_extractor_ = [](const RequestInfo& request_info) { diff --git a/source/common/http/access_log/access_log_formatter.h b/source/common/http/access_log/access_log_formatter.h index a220fb47d112f..ef7be1427d579 100644 --- a/source/common/http/access_log/access_log_formatter.h +++ b/source/common/http/access_log/access_log_formatter.h @@ -10,7 +10,7 @@ namespace AccessLog { */ class FilterReasonUtils { public: - static const std::string toShortString(uint64_t failure_reason); + static const std::string toShortString(const RequestInfo& request_info); private: FilterReasonUtils(); diff --git a/source/common/http/access_log/request_info_impl.h b/source/common/http/access_log/request_info_impl.h index 5ba55966a8080..e1cc9e4542fb1 100644 --- a/source/common/http/access_log/request_info_impl.h +++ b/source/common/http/access_log/request_info_impl.h @@ -30,7 +30,13 @@ struct RequestInfoImpl : public RequestInfo { response_flags_ |= response_flag; } - uint64_t getResponseFlags() const override { return response_flags_; } + bool isSetResponseFlag(ResponseFlag flag) const override { + if (flag == ResponseFlag::None) { + return response_flags_ == ResponseFlag::None; + } + + return response_flags_ & flag; + } void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) override { upstream_host_ = host; } diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index c686b41b445d3..0899909e596f5 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -148,7 +148,7 @@ bool ConnectionManagerUtility::shouldTraceRequest( case Http::TracingType::All: return true; case Http::TracingType::UpstreamFailure: - return request_info.getResponseFlags() != Http::AccessLog::ResponseFlag::None; + return !request_info.isSetResponseFlag(Http::AccessLog::ResponseFlag::None); } // Compiler enforces switch above to cover all the cases and it's impossible to be here, diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index ef72da986db65..f42efff58628a 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -264,9 +264,8 @@ void LightStepSink::flushTrace(const Http::HeaderMap& request_headers, const Htt span.SetTag("error", "true"); } - if (request_info.getResponseFlags() != Http::AccessLog::ResponseFlag::None) { - span.SetTag("failure reason", - Http::AccessLog::FilterReasonUtils::toShortString(request_info.getResponseFlags())); + if (!request_info.isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) { + span.SetTag("failure reason", Http::AccessLog::FilterReasonUtils::toShortString(request_info)); } if (request_headers.ClientTraceId()) { diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index 105077621c838..8bf6b92d1344e 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/http/mocks.h" #include "test/test_common/utility.h" +using testing::NiceMock; using testing::Return; using testing::ReturnRef; @@ -27,15 +28,31 @@ TEST(FailureReasonUtilsTest, toShortStringConversion) { std::make_pair(ResponseFlag::DelayInjected, "DI")}; for (const auto& testCase : expected) { - EXPECT_EQ(testCase.second, FilterReasonUtils::toShortString(testCase.first)); + NiceMock request_info; + ON_CALL(request_info, isSetResponseFlag(testCase.first)).WillByDefault(Return(true)); + EXPECT_EQ(testCase.second, FilterReasonUtils::toShortString(request_info)); } // Test combinations. - EXPECT_EQ("UT,DI", FilterReasonUtils::toShortString(ResponseFlag::DelayInjected | - ResponseFlag::UpstreamRequestTimeout)); - EXPECT_EQ("UT,FI,DI", FilterReasonUtils::toShortString(ResponseFlag::FaultInjected | - ResponseFlag::DelayInjected | - ResponseFlag::UpstreamRequestTimeout)); + { + NiceMock request_info; + ON_CALL(request_info, isSetResponseFlag(ResponseFlag::DelayInjected)) + .WillByDefault(Return(true)); + ON_CALL(request_info, isSetResponseFlag(ResponseFlag::UpstreamRequestTimeout)) + .WillByDefault(Return(true)); + EXPECT_EQ("UT,DI", FilterReasonUtils::toShortString(request_info)); + } + + { + NiceMock request_info; + ON_CALL(request_info, isSetResponseFlag(ResponseFlag::DelayInjected)) + .WillByDefault(Return(true)); + ON_CALL(request_info, isSetResponseFlag(ResponseFlag::UpstreamRequestTimeout)) + .WillByDefault(Return(true)); + ON_CALL(request_info, isSetResponseFlag(ResponseFlag::FaultInjected)) + .WillByDefault(Return(true)); + EXPECT_EQ("UT,FI,DI", FilterReasonUtils::toShortString(request_info)); + } } TEST(AccessLogFormatUtilsTest, protocolToString) { @@ -55,7 +72,7 @@ TEST(AccessLogFormatterTest, plainStringFormatter) { TEST(AccessLogFormatterTest, requestInfoFormatter) { EXPECT_THROW(RequestInfoFormatter formatter("unknown_field"), EnvoyException); - MockRequestInfo requestInfo; + NiceMock requestInfo; TestHeaderMapImpl header{{":method", "GET"}, {":path", "/"}}; { @@ -107,8 +124,7 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { { RequestInfoFormatter response_flags_format("RESPONSE_FLAGS"); - uint64_t response_flags = ResponseFlag::LocalReset; - EXPECT_CALL(requestInfo, getResponseFlags()).WillOnce(Return(response_flags)); + ON_CALL(requestInfo, isSetResponseFlag(ResponseFlag::LocalReset)).WillByDefault(Return(true)); EXPECT_EQ("LR", response_flags_format.format(header, header, requestInfo)); } diff --git a/test/common/http/access_log/access_log_impl_test.cc b/test/common/http/access_log/access_log_impl_test.cc index fa6a1ae7a5966..79b5853f7b2de 100644 --- a/test/common/http/access_log/access_log_impl_test.cc +++ b/test/common/http/access_log/access_log_impl_test.cc @@ -43,7 +43,9 @@ class TestRequestInfo : public RequestInfo { std::chrono::milliseconds duration() const override { return std::chrono::milliseconds(duration_); } - uint64_t getResponseFlags() const override { return response_flags_; } + bool isSetResponseFlag(ResponseFlag response_flag) const override { + return response_flags_ == response_flag; + } void setResponseFlag(ResponseFlag response_flag) override { response_flags_ = response_flag; } void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) override { upstream_host_ = host; } Upstream::HostDescriptionPtr upstreamHost() const override { return upstream_host_; } diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 363dbc2bfd450..279503408a15f 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -61,12 +61,13 @@ TEST_F(ConnectionManagerUtilityTest, ShouldTraceRequest) { NiceMock request_info; Optional tracing_failure( {"operation", Http::TracingType::UpstreamFailure}); - EXPECT_CALL(request_info, getResponseFlags()) - .WillOnce(Return(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)); + ON_CALL(request_info, + isSetResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)) + .WillByDefault(Return(true)); EXPECT_TRUE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); - EXPECT_CALL(request_info, getResponseFlags()) - .WillOnce(Return(Http::AccessLog::ResponseFlag::None)); + ON_CALL(request_info, isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) + .WillByDefault(Return(true)); EXPECT_FALSE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); } } diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 86497aa29050d..f1a490db311a6 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -408,8 +408,8 @@ TEST_F(LightStepSinkTest, FlushSeveralSpans) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, getResponseFlags()) - .WillByDefault(Return(Http::AccessLog::ResponseFlag::None)); + ON_CALL(request_info, isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) + .WillByDefault(Return(true)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); @@ -479,8 +479,8 @@ TEST_F(LightStepSinkTest, FlushSpansTimer) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, getResponseFlags()) - .WillByDefault(Return(Http::AccessLog::ResponseFlag::None)); + ON_CALL(request_info, isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) + .WillByDefault(Return(true)); const Optional timeout(std::chrono::seconds(5)); EXPECT_CALL(cm_.async_client_, send_(_, _, timeout)); @@ -516,8 +516,8 @@ TEST_F(LightStepSinkTest, FlushOneSpanGrpcFailure) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, getResponseFlags()) - .WillByDefault(Return(Http::AccessLog::ResponseFlag::None)); + ON_CALL(request_info, isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) + .WillByDefault(Return(true)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 3760b0afcefbb..661fc1999b513 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -42,7 +42,7 @@ class MockRequestInfo : public RequestInfo { MOCK_CONST_METHOD0(responseCode, Optional&()); MOCK_CONST_METHOD0(bytesSent, uint64_t()); MOCK_CONST_METHOD0(duration, std::chrono::milliseconds()); - MOCK_CONST_METHOD0(getResponseFlags, uint64_t()); + MOCK_CONST_METHOD1(isSetResponseFlag, bool(Http::AccessLog::ResponseFlag)); MOCK_CONST_METHOD0(upstreamHost, Upstream::HostDescriptionPtr()); MOCK_CONST_METHOD0(healthCheck, bool()); MOCK_METHOD1(healthCheck, void(bool is_hc)); From 947a456c9dd6a5285e1f26c5a6992b2490430e77 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Wed, 7 Dec 2016 10:19:48 -0800 Subject: [PATCH 05/13] Update access_log.rst --- docs/configuration/http_conn_man/access_log.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/configuration/http_conn_man/access_log.rst b/docs/configuration/http_conn_man/access_log.rst index 2db355749d630..6e6f2f3ad7d3b 100644 --- a/docs/configuration/http_conn_man/access_log.rst +++ b/docs/configuration/http_conn_man/access_log.rst @@ -72,7 +72,7 @@ The following command operators are supported: Total duration in milliseconds of the request from the start time to the last byte out. %RESPONSE_FLAGS% - Additional details to response code if any. Possible values are: + Additional details about the response, if any. Possible values are: * **LH**: Local service failed :ref:`health check request ` in addition to 503 response code. * **UH**: No healthy upstream hosts in upstream cluster in addition to 503 response code. @@ -83,7 +83,7 @@ The following command operators are supported: * **UC**: Upstream connection termination in addition to 503 response code. * **UO**: Upstream overflow (:ref:`circuit breaking `) in addition to 503 response code. * **NR**: No :ref:`route configured ` for a given request in addition to 404 response code. - * **FI**: The request was aborted with injected response code by :ref:`fault injection `. + * **FI**: The request was aborted with an injected response code via :ref:`fault injection `. %UPSTREAM_HOST% Upstream host URL (e.g., tcp://ip:port for TCP connections). From 65741de5843017aca88053b08e3411cfe5abd4e2 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 14 Dec 2016 13:23:03 -0800 Subject: [PATCH 06/13] getResponseFlag. --- include/envoy/http/access_log.h | 8 ++--- .../http/access_log/access_log_formatter.cc | 30 +++++++------------ .../http/access_log/request_info_impl.h | 2 +- source/common/http/conn_manager_utility.cc | 2 +- source/common/tracing/http_tracer_impl.cc | 2 +- .../access_log/access_log_formatter_test.cc | 14 ++++----- .../http/access_log/access_log_impl_test.cc | 2 +- test/common/http/conn_manager_utility_test.cc | 4 +-- test/common/tracing/http_tracer_impl_test.cc | 6 ++-- test/mocks/http/mocks.h | 2 +- 10 files changed, 31 insertions(+), 41 deletions(-) diff --git a/include/envoy/http/access_log.h b/include/envoy/http/access_log.h index 8607c16c808b7..6dd014cf10875 100644 --- a/include/envoy/http/access_log.h +++ b/include/envoy/http/access_log.h @@ -45,7 +45,7 @@ class RequestInfo { virtual ~RequestInfo() {} /** - * filter can set response flags. + * each filter can set independent response flag, flags are accumulated. */ virtual void setResponseFlag(ResponseFlag response_flag) PURE; @@ -90,11 +90,9 @@ class RequestInfo { virtual std::chrono::milliseconds duration() const PURE; /** - * @return response flags to enrich access log with details around response code. Response flag - * complements response - * code and add details to it. + * @return whether response flag is set or not. */ - virtual bool isSetResponseFlag(ResponseFlag response_flag) const PURE; + virtual bool getResponseFlag(ResponseFlag response_flag) const PURE; /** * @return upstream host description. diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index 2b8f42fc627ce..0d9abb530108d 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -28,56 +28,48 @@ void FilterReasonUtils::appendString(std::string& result, const std::string& app } const std::string FilterReasonUtils::toShortString(const RequestInfo& request_info) { - std::string result = ""; + std::string result; - if (request_info.isSetResponseFlag(ResponseFlag::None)) { - return NONE; - } - - if (request_info.isSetResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { + if (request_info.getResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { appendString(result, FAILED_LOCAL_HEALTH_CHECK); } - if (request_info.isSetResponseFlag(ResponseFlag::NoHealthyUpstream)) { + if (request_info.getResponseFlag(ResponseFlag::NoHealthyUpstream)) { appendString(result, NO_HEALTHY_UPSTREAM); } - if (request_info.isSetResponseFlag(ResponseFlag::UpstreamRequestTimeout)) { + if (request_info.getResponseFlag(ResponseFlag::UpstreamRequestTimeout)) { appendString(result, UPSTREAM_REQUEST_TIMEOUT); } - if (request_info.isSetResponseFlag(ResponseFlag::LocalReset)) { + if (request_info.getResponseFlag(ResponseFlag::LocalReset)) { appendString(result, LOCAL_RESET); } - if (request_info.isSetResponseFlag(ResponseFlag::UpstreamRemoteReset)) { + if (request_info.getResponseFlag(ResponseFlag::UpstreamRemoteReset)) { appendString(result, UPSTREAM_REMOTE_RESET); } - if (request_info.isSetResponseFlag(ResponseFlag::UpstreamConnectionFailure)) { + if (request_info.getResponseFlag(ResponseFlag::UpstreamConnectionFailure)) { appendString(result, UPSTREAM_CONNECTION_FAILURE); } - if (request_info.isSetResponseFlag(ResponseFlag::UpstreamConnectionTermination)) { + if (request_info.getResponseFlag(ResponseFlag::UpstreamConnectionTermination)) { appendString(result, UPSTREAM_CONNECTION_TERMINATION); } - if (request_info.isSetResponseFlag(ResponseFlag::UpstreamOverflow)) { + if (request_info.getResponseFlag(ResponseFlag::UpstreamOverflow)) { appendString(result, UPSTREAM_OVERFLOW); } - if (request_info.isSetResponseFlag(ResponseFlag::NoRouteFound)) { + if (request_info.getResponseFlag(ResponseFlag::NoRouteFound)) { appendString(result, NO_ROUTE_FOUND); } - if (request_info.isSetResponseFlag(ResponseFlag::FaultInjected)) { + if (request_info.getResponseFlag(ResponseFlag::FaultInjected)) { appendString(result, FAULT_INJECTED); } - if (request_info.isSetResponseFlag(ResponseFlag::DelayInjected)) { - appendString(result, DELAY_INJECTED); - } - return result; } diff --git a/source/common/http/access_log/request_info_impl.h b/source/common/http/access_log/request_info_impl.h index e1cc9e4542fb1..4639fa9b668e1 100644 --- a/source/common/http/access_log/request_info_impl.h +++ b/source/common/http/access_log/request_info_impl.h @@ -30,7 +30,7 @@ struct RequestInfoImpl : public RequestInfo { response_flags_ |= response_flag; } - bool isSetResponseFlag(ResponseFlag flag) const override { + bool getResponseFlag(ResponseFlag flag) const override { if (flag == ResponseFlag::None) { return response_flags_ == ResponseFlag::None; } diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 0899909e596f5..99951298830ba 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -148,7 +148,7 @@ bool ConnectionManagerUtility::shouldTraceRequest( case Http::TracingType::All: return true; case Http::TracingType::UpstreamFailure: - return !request_info.isSetResponseFlag(Http::AccessLog::ResponseFlag::None); + return !request_info.getResponseFlag(Http::AccessLog::ResponseFlag::None); } // Compiler enforces switch above to cover all the cases and it's impossible to be here, diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index f42efff58628a..0178161c94584 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -264,7 +264,7 @@ void LightStepSink::flushTrace(const Http::HeaderMap& request_headers, const Htt span.SetTag("error", "true"); } - if (!request_info.isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) { + if (!request_info.getResponseFlag(Http::AccessLog::ResponseFlag::None)) { span.SetTag("failure reason", Http::AccessLog::FilterReasonUtils::toShortString(request_info)); } diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index 8bf6b92d1344e..8ed0b251bc9e4 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -29,27 +29,27 @@ TEST(FailureReasonUtilsTest, toShortStringConversion) { for (const auto& testCase : expected) { NiceMock request_info; - ON_CALL(request_info, isSetResponseFlag(testCase.first)).WillByDefault(Return(true)); + ON_CALL(request_info, getResponseFlag(testCase.first)).WillByDefault(Return(true)); EXPECT_EQ(testCase.second, FilterReasonUtils::toShortString(request_info)); } // Test combinations. { NiceMock request_info; - ON_CALL(request_info, isSetResponseFlag(ResponseFlag::DelayInjected)) + ON_CALL(request_info, getResponseFlag(ResponseFlag::DelayInjected)) .WillByDefault(Return(true)); - ON_CALL(request_info, isSetResponseFlag(ResponseFlag::UpstreamRequestTimeout)) + ON_CALL(request_info, getResponseFlag(ResponseFlag::UpstreamRequestTimeout)) .WillByDefault(Return(true)); EXPECT_EQ("UT,DI", FilterReasonUtils::toShortString(request_info)); } { NiceMock request_info; - ON_CALL(request_info, isSetResponseFlag(ResponseFlag::DelayInjected)) + ON_CALL(request_info, getResponseFlag(ResponseFlag::DelayInjected)) .WillByDefault(Return(true)); - ON_CALL(request_info, isSetResponseFlag(ResponseFlag::UpstreamRequestTimeout)) + ON_CALL(request_info, getResponseFlag(ResponseFlag::UpstreamRequestTimeout)) .WillByDefault(Return(true)); - ON_CALL(request_info, isSetResponseFlag(ResponseFlag::FaultInjected)) + ON_CALL(request_info, getResponseFlag(ResponseFlag::FaultInjected)) .WillByDefault(Return(true)); EXPECT_EQ("UT,FI,DI", FilterReasonUtils::toShortString(request_info)); } @@ -124,7 +124,7 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { { RequestInfoFormatter response_flags_format("RESPONSE_FLAGS"); - ON_CALL(requestInfo, isSetResponseFlag(ResponseFlag::LocalReset)).WillByDefault(Return(true)); + ON_CALL(requestInfo, getResponseFlag(ResponseFlag::LocalReset)).WillByDefault(Return(true)); EXPECT_EQ("LR", response_flags_format.format(header, header, requestInfo)); } diff --git a/test/common/http/access_log/access_log_impl_test.cc b/test/common/http/access_log/access_log_impl_test.cc index 79b5853f7b2de..a38a4c67bcd43 100644 --- a/test/common/http/access_log/access_log_impl_test.cc +++ b/test/common/http/access_log/access_log_impl_test.cc @@ -43,7 +43,7 @@ class TestRequestInfo : public RequestInfo { std::chrono::milliseconds duration() const override { return std::chrono::milliseconds(duration_); } - bool isSetResponseFlag(ResponseFlag response_flag) const override { + bool getResponseFlag(ResponseFlag response_flag) const override { return response_flags_ == response_flag; } void setResponseFlag(ResponseFlag response_flag) override { response_flags_ = response_flag; } diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 279503408a15f..16248c1a95745 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -62,11 +62,11 @@ TEST_F(ConnectionManagerUtilityTest, ShouldTraceRequest) { Optional tracing_failure( {"operation", Http::TracingType::UpstreamFailure}); ON_CALL(request_info, - isSetResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)) + getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)) .WillByDefault(Return(true)); EXPECT_TRUE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); - ON_CALL(request_info, isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) + ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::None)) .WillByDefault(Return(true)); EXPECT_FALSE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); } diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index f1a490db311a6..ebf98bf9dc2d0 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -408,7 +408,7 @@ TEST_F(LightStepSinkTest, FlushSeveralSpans) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) + ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::None)) .WillByDefault(Return(true)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; @@ -479,7 +479,7 @@ TEST_F(LightStepSinkTest, FlushSpansTimer) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) + ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::None)) .WillByDefault(Return(true)); const Optional timeout(std::chrono::seconds(5)); @@ -516,7 +516,7 @@ TEST_F(LightStepSinkTest, FlushOneSpanGrpcFailure) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, isSetResponseFlag(Http::AccessLog::ResponseFlag::None)) + ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::None)) .WillByDefault(Return(true)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 661fc1999b513..0786d26dcd6dd 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -42,7 +42,7 @@ class MockRequestInfo : public RequestInfo { MOCK_CONST_METHOD0(responseCode, Optional&()); MOCK_CONST_METHOD0(bytesSent, uint64_t()); MOCK_CONST_METHOD0(duration, std::chrono::milliseconds()); - MOCK_CONST_METHOD1(isSetResponseFlag, bool(Http::AccessLog::ResponseFlag)); + MOCK_CONST_METHOD1(getResponseFlag, bool(Http::AccessLog::ResponseFlag)); MOCK_CONST_METHOD0(upstreamHost, Upstream::HostDescriptionPtr()); MOCK_CONST_METHOD0(healthCheck, bool()); MOCK_METHOD1(healthCheck, void(bool is_hc)); From 653aa15ffc6892023a032585e53904745adb29ca Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 14 Dec 2016 14:43:05 -0800 Subject: [PATCH 07/13] first part. --- include/envoy/http/access_log.h | 6 +---- .../http/access_log/access_log_formatter.cc | 2 +- .../http/access_log/request_info_impl.h | 10 ++------ source/common/http/conn_manager_utility.cc | 10 +++++++- source/common/http/conn_manager_utility.h | 2 ++ source/common/tracing/http_tracer_impl.cc | 4 +--- .../access_log/access_log_formatter_test.cc | 23 ++++++++----------- .../http/access_log/access_log_impl_test.cc | 2 +- test/common/http/conn_manager_utility_test.cc | 7 +++--- test/common/router/router_test.cc | 5 +--- test/common/tracing/http_tracer_impl_test.cc | 9 +++----- 11 files changed, 33 insertions(+), 47 deletions(-) diff --git a/include/envoy/http/access_log.h b/include/envoy/http/access_log.h index 6dd014cf10875..8e73c8ab2772f 100644 --- a/include/envoy/http/access_log.h +++ b/include/envoy/http/access_log.h @@ -11,8 +11,6 @@ namespace Http { namespace AccessLog { enum ResponseFlag { - // No failure. - None = 0x0, // Local server healthcheck failed. FailedLocalHealthCheck = 0x1, // No healthy upstream. @@ -32,9 +30,7 @@ enum ResponseFlag { // No route found for a given request. NoRouteFound = 0x100, // Abort with error code is injected. - FaultInjected = 0x200, - // Delay is injected. - DelayInjected = 0x400 + FaultInjected = 0x200 }; /** diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index 0d9abb530108d..c2e7e6c1c8cf7 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -70,7 +70,7 @@ const std::string FilterReasonUtils::toShortString(const RequestInfo& request_in appendString(result, FAULT_INJECTED); } - return result; + return result.empty() ? NONE : result; } const std::string AccessLogFormatUtils::DEFAULT_FORMAT = diff --git a/source/common/http/access_log/request_info_impl.h b/source/common/http/access_log/request_info_impl.h index 4639fa9b668e1..967f1365960df 100644 --- a/source/common/http/access_log/request_info_impl.h +++ b/source/common/http/access_log/request_info_impl.h @@ -30,13 +30,7 @@ struct RequestInfoImpl : public RequestInfo { response_flags_ |= response_flag; } - bool getResponseFlag(ResponseFlag flag) const override { - if (flag == ResponseFlag::None) { - return response_flags_ == ResponseFlag::None; - } - - return response_flags_ & flag; - } + bool getResponseFlag(ResponseFlag flag) const override { return response_flags_ & flag; } void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) override { upstream_host_ = host; } @@ -51,7 +45,7 @@ struct RequestInfoImpl : public RequestInfo { uint64_t bytes_received_{}; Optional response_code_; uint64_t bytes_sent_{}; - uint64_t response_flags_{ResponseFlag::None}; + uint64_t response_flags_{}; Upstream::HostDescriptionPtr upstream_host_{}; bool hc_request_{}; }; diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 99951298830ba..62886641f32db 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -148,7 +148,7 @@ bool ConnectionManagerUtility::shouldTraceRequest( case Http::TracingType::All: return true; case Http::TracingType::UpstreamFailure: - return !request_info.getResponseFlag(Http::AccessLog::ResponseFlag::None); + return isUpstreamFailure(request_info); } // Compiler enforces switch above to cover all the cases and it's impossible to be here, @@ -156,4 +156,12 @@ bool ConnectionManagerUtility::shouldTraceRequest( NOT_IMPLEMENTED; } +bool ConnectionManagerUtility::isUpstreamFailure(const Http::AccessLog::RequestInfo& request_info) { + return request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionTermination); +} + } // Http diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index 6f79750bd3b9b..51a2da0e6ee4a 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -27,6 +27,8 @@ class ConnectionManagerUtility { const Optional& config); private: + static bool isUpstreamFailure(const Http::AccessLog::RequestInfo& request_info); + // NOTE: This is used for stable randomness in the case where the route table does not use any // runtime rules. If runtime rules are used, we use true randomness which is slower but // provides behavior that most consumers would expect. diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 0178161c94584..3f26d2a8274f2 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -264,9 +264,7 @@ void LightStepSink::flushTrace(const Http::HeaderMap& request_headers, const Htt span.SetTag("error", "true"); } - if (!request_info.getResponseFlag(Http::AccessLog::ResponseFlag::None)) { - span.SetTag("failure reason", Http::AccessLog::FilterReasonUtils::toShortString(request_info)); - } + span.SetTag("response flags", Http::AccessLog::FilterReasonUtils::toShortString(request_info)); if (request_headers.ClientTraceId()) { span.SetTag("guid:x-client-trace-id", diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index 8ed0b251bc9e4..3b26f4ca27c94 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/http/mocks.h" #include "test/test_common/utility.h" +using testing::_; using testing::NiceMock; using testing::Return; using testing::ReturnRef; @@ -14,7 +15,6 @@ namespace AccessLog { TEST(FailureReasonUtilsTest, toShortStringConversion) { std::vector> expected = { - std::make_pair(ResponseFlag::None, "-"), std::make_pair(ResponseFlag::FailedLocalHealthCheck, "LH"), std::make_pair(ResponseFlag::NoHealthyUpstream, "UH"), std::make_pair(ResponseFlag::UpstreamRequestTimeout, "UT"), @@ -24,8 +24,7 @@ TEST(FailureReasonUtilsTest, toShortStringConversion) { std::make_pair(ResponseFlag::UpstreamConnectionTermination, "UC"), std::make_pair(ResponseFlag::UpstreamOverflow, "UO"), std::make_pair(ResponseFlag::NoRouteFound, "NR"), - std::make_pair(ResponseFlag::FaultInjected, "FI"), - std::make_pair(ResponseFlag::DelayInjected, "DI")}; + std::make_pair(ResponseFlag::FaultInjected, "FI")}; for (const auto& testCase : expected) { NiceMock request_info; @@ -33,25 +32,21 @@ TEST(FailureReasonUtilsTest, toShortStringConversion) { EXPECT_EQ(testCase.second, FilterReasonUtils::toShortString(request_info)); } - // Test combinations. + // No flag is set. { NiceMock request_info; - ON_CALL(request_info, getResponseFlag(ResponseFlag::DelayInjected)) - .WillByDefault(Return(true)); - ON_CALL(request_info, getResponseFlag(ResponseFlag::UpstreamRequestTimeout)) - .WillByDefault(Return(true)); - EXPECT_EQ("UT,DI", FilterReasonUtils::toShortString(request_info)); + ON_CALL(request_info, getResponseFlag(_)).WillByDefault(Return(false)); + EXPECT_EQ("-", FilterReasonUtils::toShortString(request_info)); } + // Test combinations. + // These are not real use cases, but are used to cover multiple response flags case. { NiceMock request_info; - ON_CALL(request_info, getResponseFlag(ResponseFlag::DelayInjected)) - .WillByDefault(Return(true)); + ON_CALL(request_info, getResponseFlag(ResponseFlag::FaultInjected)).WillByDefault(Return(true)); ON_CALL(request_info, getResponseFlag(ResponseFlag::UpstreamRequestTimeout)) .WillByDefault(Return(true)); - ON_CALL(request_info, getResponseFlag(ResponseFlag::FaultInjected)) - .WillByDefault(Return(true)); - EXPECT_EQ("UT,FI,DI", FilterReasonUtils::toShortString(request_info)); + EXPECT_EQ("UT,FI", FilterReasonUtils::toShortString(request_info)); } } diff --git a/test/common/http/access_log/access_log_impl_test.cc b/test/common/http/access_log/access_log_impl_test.cc index a38a4c67bcd43..1ab54ca8eca43 100644 --- a/test/common/http/access_log/access_log_impl_test.cc +++ b/test/common/http/access_log/access_log_impl_test.cc @@ -55,7 +55,7 @@ class TestRequestInfo : public RequestInfo { SystemTime start_time_; Protocol protocol_{Protocol::Http11}; Optional response_code_; - uint64_t response_flags_{ResponseFlag::None}; + uint64_t response_flags_{}; uint64_t duration_{3}; Upstream::HostDescriptionPtr upstream_host_{}; bool hc_request_{}; diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 16248c1a95745..2fc1981eea9a5 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -61,13 +61,12 @@ TEST_F(ConnectionManagerUtilityTest, ShouldTraceRequest) { NiceMock request_info; Optional tracing_failure( {"operation", Http::TracingType::UpstreamFailure}); - ON_CALL(request_info, - getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)) + ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)) .WillByDefault(Return(true)); EXPECT_TRUE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); - ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::None)) - .WillByDefault(Return(true)); + ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)) + .WillByDefault(Return(false)); EXPECT_FALSE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); } } diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 45a21cc239903..d58bac58c8894 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -488,12 +488,9 @@ TEST_F(RouterTest, RetryTimeoutDuringRetryDelay) { // Fire timeout. EXPECT_CALL(callbacks_.request_info_, -<<<<<<< HEAD setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout)); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(504)); -======= - onFailedResponse(Http::AccessLog::FailureReason::UpstreamRequestTimeout)); ->>>>>>> master EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putResponseTime(_)).Times(0); Http::TestHeaderMapImpl response_headers{ {":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}}; diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index ebf98bf9dc2d0..7e11509e48b0d 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -408,8 +408,7 @@ TEST_F(LightStepSinkTest, FlushSeveralSpans) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::None)) - .WillByDefault(Return(true)); + ON_CALL(request_info, getResponseFlag(_)).WillByDefault(Return(true)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); @@ -479,8 +478,7 @@ TEST_F(LightStepSinkTest, FlushSpansTimer) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::None)) - .WillByDefault(Return(true)); + ON_CALL(request_info, getResponseFlag(_)).WillByDefault(Return(true)); const Optional timeout(std::chrono::seconds(5)); EXPECT_CALL(cm_.async_client_, send_(_, _, timeout)); @@ -516,8 +514,7 @@ TEST_F(LightStepSinkTest, FlushOneSpanGrpcFailure) { setupValidSink(); NiceMock request_info; - ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::None)) - .WillByDefault(Return(true)); + ON_CALL(request_info, getResponseFlag(_)).WillByDefault(Return(true)); Http::MockAsyncClientRequest request(&cm_.async_client_); Http::AsyncClient::Callbacks* callback; const Optional timeout(std::chrono::seconds(5)); From e71ce4319c41a6585444bfafb91ba61a2ef9e3a3 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 14 Dec 2016 14:56:27 -0800 Subject: [PATCH 08/13] Refactor. --- .../http/access_log/access_log_formatter.cc | 31 +++++++++---------- .../http/access_log/access_log_formatter.h | 7 ++--- source/common/tracing/http_tracer_impl.cc | 2 +- .../access_log/access_log_formatter_test.cc | 8 ++--- test/common/router/router_test.cc | 1 - 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index c2e7e6c1c8cf7..dc8f29a4aece4 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -6,20 +6,19 @@ namespace Http { namespace AccessLog { -const std::string FilterReasonUtils::NONE = "-"; -const std::string FilterReasonUtils::FAILED_LOCAL_HEALTH_CHECK = "LH"; -const std::string FilterReasonUtils::NO_HEALTHY_UPSTREAM = "UH"; -const std::string FilterReasonUtils::UPSTREAM_REQUEST_TIMEOUT = "UT"; -const std::string FilterReasonUtils::LOCAL_RESET = "LR"; -const std::string FilterReasonUtils::UPSTREAM_REMOTE_RESET = "UR"; -const std::string FilterReasonUtils::UPSTREAM_CONNECTION_FAILURE = "UF"; -const std::string FilterReasonUtils::UPSTREAM_CONNECTION_TERMINATION = "UC"; -const std::string FilterReasonUtils::UPSTREAM_OVERFLOW = "UO"; -const std::string FilterReasonUtils::NO_ROUTE_FOUND = "NR"; -const std::string FilterReasonUtils::FAULT_INJECTED = "FI"; -const std::string FilterReasonUtils::DELAY_INJECTED = "DI"; - -void FilterReasonUtils::appendString(std::string& result, const std::string& append) { +const std::string ResponseFlagUtils::NONE = "-"; +const std::string ResponseFlagUtils::FAILED_LOCAL_HEALTH_CHECK = "LH"; +const std::string ResponseFlagUtils::NO_HEALTHY_UPSTREAM = "UH"; +const std::string ResponseFlagUtils::UPSTREAM_REQUEST_TIMEOUT = "UT"; +const std::string ResponseFlagUtils::LOCAL_RESET = "LR"; +const std::string ResponseFlagUtils::UPSTREAM_REMOTE_RESET = "UR"; +const std::string ResponseFlagUtils::UPSTREAM_CONNECTION_FAILURE = "UF"; +const std::string ResponseFlagUtils::UPSTREAM_CONNECTION_TERMINATION = "UC"; +const std::string ResponseFlagUtils::UPSTREAM_OVERFLOW = "UO"; +const std::string ResponseFlagUtils::NO_ROUTE_FOUND = "NR"; +const std::string ResponseFlagUtils::FAULT_INJECTED = "FI"; + +void ResponseFlagUtils::appendString(std::string& result, const std::string& append) { if (result.empty()) { result = append; } else { @@ -27,7 +26,7 @@ void FilterReasonUtils::appendString(std::string& result, const std::string& app } } -const std::string FilterReasonUtils::toShortString(const RequestInfo& request_info) { +const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_info) { std::string result; if (request_info.getResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { @@ -238,7 +237,7 @@ RequestInfoFormatter::RequestInfoFormatter(const std::string& field_name) { }; } else if (field_name == "RESPONSE_FLAGS") { field_extractor_ = [](const RequestInfo& request_info) { - return FilterReasonUtils::toShortString(request_info); + return ResponseFlagUtils::toShortString(request_info); }; } else if (field_name == "UPSTREAM_HOST") { field_extractor_ = [](const RequestInfo& request_info) { diff --git a/source/common/http/access_log/access_log_formatter.h b/source/common/http/access_log/access_log_formatter.h index ef7be1427d579..bb6d796e8c2d4 100644 --- a/source/common/http/access_log/access_log_formatter.h +++ b/source/common/http/access_log/access_log_formatter.h @@ -6,14 +6,14 @@ namespace Http { namespace AccessLog { /** - * Util class for FilterReason. + * Util class for ResponseFlags. */ -class FilterReasonUtils { +class ResponseFlagUtils { public: static const std::string toShortString(const RequestInfo& request_info); private: - FilterReasonUtils(); + ResponseFlagUtils(); static void appendString(std::string& result, const std::string& append); const static std::string NONE; @@ -27,7 +27,6 @@ class FilterReasonUtils { const static std::string UPSTREAM_OVERFLOW; const static std::string NO_ROUTE_FOUND; const static std::string FAULT_INJECTED; - const static std::string DELAY_INJECTED; }; /** diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 3f26d2a8274f2..abea122f2a598 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -264,7 +264,7 @@ void LightStepSink::flushTrace(const Http::HeaderMap& request_headers, const Htt span.SetTag("error", "true"); } - span.SetTag("response flags", Http::AccessLog::FilterReasonUtils::toShortString(request_info)); + span.SetTag("response flags", Http::AccessLog::ResponseFlagUtils::toShortString(request_info)); if (request_headers.ClientTraceId()) { span.SetTag("guid:x-client-trace-id", diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index 3b26f4ca27c94..6898e98e0ec13 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -13,7 +13,7 @@ using testing::ReturnRef; namespace Http { namespace AccessLog { -TEST(FailureReasonUtilsTest, toShortStringConversion) { +TEST(ResponseFlagUtilsTest, toShortStringConversion) { std::vector> expected = { std::make_pair(ResponseFlag::FailedLocalHealthCheck, "LH"), std::make_pair(ResponseFlag::NoHealthyUpstream, "UH"), @@ -29,14 +29,14 @@ TEST(FailureReasonUtilsTest, toShortStringConversion) { for (const auto& testCase : expected) { NiceMock request_info; ON_CALL(request_info, getResponseFlag(testCase.first)).WillByDefault(Return(true)); - EXPECT_EQ(testCase.second, FilterReasonUtils::toShortString(request_info)); + EXPECT_EQ(testCase.second, ResponseFlagUtils::toShortString(request_info)); } // No flag is set. { NiceMock request_info; ON_CALL(request_info, getResponseFlag(_)).WillByDefault(Return(false)); - EXPECT_EQ("-", FilterReasonUtils::toShortString(request_info)); + EXPECT_EQ("-", ResponseFlagUtils::toShortString(request_info)); } // Test combinations. @@ -46,7 +46,7 @@ TEST(FailureReasonUtilsTest, toShortStringConversion) { ON_CALL(request_info, getResponseFlag(ResponseFlag::FaultInjected)).WillByDefault(Return(true)); ON_CALL(request_info, getResponseFlag(ResponseFlag::UpstreamRequestTimeout)) .WillByDefault(Return(true)); - EXPECT_EQ("UT,FI", FilterReasonUtils::toShortString(request_info)); + EXPECT_EQ("UT,FI", ResponseFlagUtils::toShortString(request_info)); } } diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index d58bac58c8894..90c411d53b933 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -490,7 +490,6 @@ TEST_F(RouterTest, RetryTimeoutDuringRetryDelay) { EXPECT_CALL(callbacks_.request_info_, setResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout)); - EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(504)); EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putResponseTime(_)).Times(0); Http::TestHeaderMapImpl response_headers{ {":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}}; From 98833d77a3d81706841109ae738d821a7bc09c27 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 14 Dec 2016 15:28:03 -0800 Subject: [PATCH 09/13] test. --- source/common/http/conn_manager_utility.cc | 7 +++- test/common/http/conn_manager_utility_test.cc | 37 ++++++++++++++++--- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 62886641f32db..a8825a0033f93 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -159,9 +159,12 @@ bool ConnectionManagerUtility::shouldTraceRequest( bool ConnectionManagerUtility::isUpstreamFailure(const Http::AccessLog::RequestInfo& request_info) { return request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream) | request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure) | - request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamOverflow) | request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout) | - request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionTermination); + request_info.getResponseFlag( + Http::AccessLog::ResponseFlag::UpstreamConnectionTermination) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoRouteFound) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::FaultInjected); } } // Http diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 2fc1981eea9a5..4dfde80bf2943 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -61,13 +61,38 @@ TEST_F(ConnectionManagerUtilityTest, ShouldTraceRequest) { NiceMock request_info; Optional tracing_failure( {"operation", Http::TracingType::UpstreamFailure}); - ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)) - .WillByDefault(Return(true)); - EXPECT_TRUE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); - ON_CALL(request_info, getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure)) - .WillByDefault(Return(false)); - EXPECT_FALSE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); + std::vector upstream_failed_response_flag{ + Http::AccessLog::ResponseFlag::UpstreamConnectionFailure, + Http::AccessLog::ResponseFlag::UpstreamConnectionTermination, + Http::AccessLog::ResponseFlag::NoHealthyUpstream, + Http::AccessLog::ResponseFlag::UpstreamRequestTimeout, + Http::AccessLog::ResponseFlag::UpstreamOverflow, + Http::AccessLog::ResponseFlag::FaultInjected, Http::AccessLog::ResponseFlag::NoRouteFound}; + + for (Http::AccessLog::ResponseFlag flag : upstream_failed_response_flag) { + ON_CALL(request_info, getResponseFlag(flag)).WillByDefault(Return(true)); + EXPECT_TRUE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); + + ON_CALL(request_info, getResponseFlag(flag)).WillByDefault(Return(false)); + EXPECT_FALSE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); + } + } + + { + NiceMock request_info; + Optional tracing_failure( + {"operation", Http::TracingType::UpstreamFailure}); + + std::vector upstream_not_failed_response_flag{ + Http::AccessLog::ResponseFlag::FailedLocalHealthCheck, + Http::AccessLog::ResponseFlag::LocalReset, + Http::AccessLog::ResponseFlag::UpstreamRemoteReset}; + + for (Http::AccessLog::ResponseFlag flag : upstream_not_failed_response_flag) { + ON_CALL(request_info, getResponseFlag(flag)).WillByDefault(Return(true)); + EXPECT_FALSE(ConnectionManagerUtility::shouldTraceRequest(request_info, tracing_failure)); + } } } From 54800f2869c6d909de6c220ccbeada98b81b24ee Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 14 Dec 2016 15:34:17 -0800 Subject: [PATCH 10/13] nits. --- source/common/http/conn_manager_utility.cc | 2 +- source/common/http/conn_manager_utility.h | 2 +- source/common/router/router.cc | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index a8825a0033f93..9c9b1a140bfb4 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -156,7 +156,7 @@ bool ConnectionManagerUtility::shouldTraceRequest( NOT_IMPLEMENTED; } -bool ConnectionManagerUtility::isUpstreamFailure(const Http::AccessLog::RequestInfo& request_info) { +bool ConnectionManagerUtility::isTraceableFailure(const Http::AccessLog::RequestInfo& request_info) { return request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream) | request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure) | request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamOverflow) | diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index 51a2da0e6ee4a..e4cf54785abd2 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -27,7 +27,7 @@ class ConnectionManagerUtility { const Optional& config); private: - static bool isUpstreamFailure(const Http::AccessLog::RequestInfo& request_info); + static bool isTraceableFailure(const Http::AccessLog::RequestInfo& request_info); // NOTE: This is used for stable randomness in the case where the route table does not use any // runtime rules. If runtime rules are used, we use true randomness which is slower but diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 119245c1ef169..401c1af98850c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -373,9 +373,9 @@ void Filter::onUpstreamReset(UpstreamResetType type, code = Http::Code::GatewayTimeout; body = "upstream request timeout"; } else { - Http::AccessLog::ResponseFlag failure_reason = + Http::AccessLog::ResponseFlag response_flags = streamResetReasonToResponseFlag(reset_reason.value()); - callbacks_->requestInfo().setResponseFlag(failure_reason); + callbacks_->requestInfo().setResponseFlag(response_flags); code = Http::Code::ServiceUnavailable; body = "upstream connect error or disconnect/reset before headers"; } From 7d63edf8741bfaddd4a13389914eeca470ecc22d Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 14 Dec 2016 15:37:48 -0800 Subject: [PATCH 11/13] fix. --- source/common/http/conn_manager_utility.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 9c9b1a140bfb4..08bf4c72ac3be 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -156,7 +156,8 @@ bool ConnectionManagerUtility::shouldTraceRequest( NOT_IMPLEMENTED; } -bool ConnectionManagerUtility::isTraceableFailure(const Http::AccessLog::RequestInfo& request_info) { +bool ConnectionManagerUtility::isTraceableFailure( + const Http::AccessLog::RequestInfo& request_info) { return request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream) | request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure) | request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamOverflow) | From 12c6a6c2c99344a0a8b2922f3fd833ca2a068439 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 14 Dec 2016 15:38:53 -0800 Subject: [PATCH 12/13] fix method. --- source/common/http/conn_manager_utility.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 08bf4c72ac3be..b9c34ea066b4c 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -148,7 +148,7 @@ bool ConnectionManagerUtility::shouldTraceRequest( case Http::TracingType::All: return true; case Http::TracingType::UpstreamFailure: - return isUpstreamFailure(request_info); + return isTraceableFailure(request_info); } // Compiler enforces switch above to cover all the cases and it's impossible to be here, From 9f5516daf8aa8354ee1d427f3bcca82952c19995 Mon Sep 17 00:00:00 2001 From: Roman Dzhabarov Date: Wed, 14 Dec 2016 16:53:11 -0800 Subject: [PATCH 13/13] moved. --- include/envoy/http/access_log.h | 4 ++-- .../http/access_log/access_log_formatter.cc | 11 +++++++++++ .../common/http/access_log/access_log_formatter.h | 1 + source/common/http/conn_manager_utility.cc | 15 ++------------- .../http/access_log/access_log_impl_test.cc | 4 ++-- 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/include/envoy/http/access_log.h b/include/envoy/http/access_log.h index 8e73c8ab2772f..68b5f29c4e8f2 100644 --- a/include/envoy/http/access_log.h +++ b/include/envoy/http/access_log.h @@ -41,12 +41,12 @@ class RequestInfo { virtual ~RequestInfo() {} /** - * each filter can set independent response flag, flags are accumulated. + * Each filter can set independent response flag, flags are accumulated. */ virtual void setResponseFlag(ResponseFlag response_flag) PURE; /** - * filter can trigger this callback when an upstream host has been selected. + * Filter can trigger this callback when an upstream host has been selected. */ virtual void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) PURE; diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index dc8f29a4aece4..f6cbadacfe0ba 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -72,6 +72,17 @@ const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_in return result.empty() ? NONE : result; } +bool ResponseFlagUtils::isTraceableFailure(const Http::AccessLog::RequestInfo& request_info) { + return request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamOverflow) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout) | + request_info.getResponseFlag( + Http::AccessLog::ResponseFlag::UpstreamConnectionTermination) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoRouteFound) | + request_info.getResponseFlag(Http::AccessLog::ResponseFlag::FaultInjected); +} + const std::string AccessLogFormatUtils::DEFAULT_FORMAT = "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%\" " "%RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% " diff --git a/source/common/http/access_log/access_log_formatter.h b/source/common/http/access_log/access_log_formatter.h index bb6d796e8c2d4..abdef95ca5363 100644 --- a/source/common/http/access_log/access_log_formatter.h +++ b/source/common/http/access_log/access_log_formatter.h @@ -11,6 +11,7 @@ namespace AccessLog { class ResponseFlagUtils { public: static const std::string toShortString(const RequestInfo& request_info); + static bool isTraceableFailure(const Http::AccessLog::RequestInfo& request_info); private: ResponseFlagUtils(); diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index b9c34ea066b4c..6038b7c00aad4 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -1,5 +1,6 @@ #include "conn_manager_utility.h" +#include "common/http/access_log/access_log_formatter.h" #include "common/http/headers.h" #include "common/http/utility.h" #include "common/network/utility.h" @@ -148,7 +149,7 @@ bool ConnectionManagerUtility::shouldTraceRequest( case Http::TracingType::All: return true; case Http::TracingType::UpstreamFailure: - return isTraceableFailure(request_info); + return Http::AccessLog::ResponseFlagUtils::isTraceableFailure(request_info); } // Compiler enforces switch above to cover all the cases and it's impossible to be here, @@ -156,16 +157,4 @@ bool ConnectionManagerUtility::shouldTraceRequest( NOT_IMPLEMENTED; } -bool ConnectionManagerUtility::isTraceableFailure( - const Http::AccessLog::RequestInfo& request_info) { - return request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoHealthyUpstream) | - request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamConnectionFailure) | - request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamOverflow) | - request_info.getResponseFlag(Http::AccessLog::ResponseFlag::UpstreamRequestTimeout) | - request_info.getResponseFlag( - Http::AccessLog::ResponseFlag::UpstreamConnectionTermination) | - request_info.getResponseFlag(Http::AccessLog::ResponseFlag::NoRouteFound) | - request_info.getResponseFlag(Http::AccessLog::ResponseFlag::FaultInjected); -} - } // Http diff --git a/test/common/http/access_log/access_log_impl_test.cc b/test/common/http/access_log/access_log_impl_test.cc index 1ab54ca8eca43..fa63697cd29ed 100644 --- a/test/common/http/access_log/access_log_impl_test.cc +++ b/test/common/http/access_log/access_log_impl_test.cc @@ -44,9 +44,9 @@ class TestRequestInfo : public RequestInfo { return std::chrono::milliseconds(duration_); } bool getResponseFlag(ResponseFlag response_flag) const override { - return response_flags_ == response_flag; + return response_flags_ & response_flag; } - void setResponseFlag(ResponseFlag response_flag) override { response_flags_ = response_flag; } + void setResponseFlag(ResponseFlag response_flag) override { response_flags_ |= response_flag; } void onUpstreamHostSelected(Upstream::HostDescriptionPtr host) override { upstream_host_ = host; } Upstream::HostDescriptionPtr upstreamHost() const override { return upstream_host_; } bool healthCheck() const override { return hc_request_; }