From c876ba085c43511192038273010e6f457082f077 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Tue, 31 Jan 2017 11:57:23 -0800 Subject: [PATCH 1/4] Add access log handler for HTTP stream filter. --- include/envoy/http/filter.h | 22 ++++++++++++++++++++++ source/common/http/conn_manager_impl.cc | 7 +++++++ source/common/http/conn_manager_impl.h | 2 ++ 3 files changed, 31 insertions(+) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 1959d6d047f44..b7b5172f885b0 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -267,6 +267,22 @@ class StreamFilter : public StreamDecoderFilter, public StreamEncoderFilter {}; typedef std::shared_ptr StreamFilterPtr; +/** + * Stream access log handler interface. + */ +class AccessLogHandler { +public: + virtual ~AccessLogHandler() {} + + /** + * Called with request_info data. + * @param request_info: the request_info data. + */ + virtual void logRequestInfo(const Http::AccessLog::RequestInfo& request_info) PURE; +}; + +typedef std::shared_ptr AccessLogHandlerPtr; + /** * These callbacks are provided by the connection manager to the factory so that the factory can * build the filter chain in an application specific way. @@ -292,6 +308,12 @@ class FilterChainFactoryCallbacks { * @param filter supplies the filter to add. */ virtual void addStreamFilter(Http::StreamFilterPtr filter) PURE; + + /** + * Add an access log handler that is called after response is send. + * @param handler supplies the handler to add. + */ + virtual void addAccessLogHandler(AccessLogHandlerPtr handler) PURE; }; /** diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index cd0ca2e3782af..8a67015a1071f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -284,6 +284,9 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { for (AccessLog::InstancePtr access_log : connection_manager_.config_.accessLogs()) { access_log->log(request_headers_.get(), response_headers_.get(), request_info_); } + for (const auto& log_handler : access_log_handlers_) { + log_handler->logRequestInfo(request_info_); + } if (active_span_ && !request_info_.healthCheck()) { Tracing::HttpTracerUtility::finalizeSpan(*active_span_, request_info_); @@ -307,6 +310,10 @@ void ConnectionManagerImpl::ActiveStream::addStreamFilter(StreamFilterPtr filter addStreamEncoderFilter(filter); } +void ConnectionManagerImpl::ActiveStream::addAccessLogHandler(AccessLogHandlerPtr handler) { + access_log_handlers_.push_back(handler); +} + void ConnectionManagerImpl::ActiveStream::chargeStats(HeaderMap& headers) { uint64_t response_code = Utility::getResponseStatus(headers); request_info_.response_code_.value(response_code); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 2cc8ff4a932a3..b0d04acab75a2 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -380,6 +380,7 @@ class ConnectionManagerImpl : Logger::Loggable, void addStreamDecoderFilter(StreamDecoderFilterPtr filter) override; void addStreamEncoderFilter(StreamEncoderFilterPtr filter) override; void addStreamFilter(StreamFilterPtr filter) override; + void addAccessLogHandler(AccessLogHandlerPtr handler) override; // Tracing::TracingConfig virtual const std::string& operationName() const override; @@ -406,6 +407,7 @@ class ConnectionManagerImpl : Logger::Loggable, HeaderMapPtr request_trailers_; std::list decoder_filters_; std::list encoder_filters_; + std::list access_log_handlers_; Stats::TimespanPtr request_timer_; std::list> reset_callbacks_; State state_; From 4b0daab3de691776cd54d9fc2572d328b440577c Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Tue, 31 Jan 2017 16:04:14 -0800 Subject: [PATCH 2/4] Use AccessLog interface directly. --- include/envoy/http/filter.h | 18 +----------------- source/common/http/conn_manager_impl.cc | 5 +++-- source/common/http/conn_manager_impl.h | 4 ++-- 3 files changed, 6 insertions(+), 21 deletions(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index b7b5172f885b0..8ee8137003dcc 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -267,22 +267,6 @@ class StreamFilter : public StreamDecoderFilter, public StreamEncoderFilter {}; typedef std::shared_ptr StreamFilterPtr; -/** - * Stream access log handler interface. - */ -class AccessLogHandler { -public: - virtual ~AccessLogHandler() {} - - /** - * Called with request_info data. - * @param request_info: the request_info data. - */ - virtual void logRequestInfo(const Http::AccessLog::RequestInfo& request_info) PURE; -}; - -typedef std::shared_ptr AccessLogHandlerPtr; - /** * These callbacks are provided by the connection manager to the factory so that the factory can * build the filter chain in an application specific way. @@ -313,7 +297,7 @@ class FilterChainFactoryCallbacks { * Add an access log handler that is called after response is send. * @param handler supplies the handler to add. */ - virtual void addAccessLogHandler(AccessLogHandlerPtr handler) PURE; + virtual void addAccessLogHandler(Http::AccessLog::InstancePtr handler) PURE; }; /** diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 8a67015a1071f..085dd1f426391 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -285,7 +285,7 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { access_log->log(request_headers_.get(), response_headers_.get(), request_info_); } for (const auto& log_handler : access_log_handlers_) { - log_handler->logRequestInfo(request_info_); + log_handler->log(request_headers_.get(), response_headers_.get(), request_info_); } if (active_span_ && !request_info_.healthCheck()) { @@ -310,7 +310,8 @@ void ConnectionManagerImpl::ActiveStream::addStreamFilter(StreamFilterPtr filter addStreamEncoderFilter(filter); } -void ConnectionManagerImpl::ActiveStream::addAccessLogHandler(AccessLogHandlerPtr handler) { +void ConnectionManagerImpl::ActiveStream::addAccessLogHandler( + Http::AccessLog::InstancePtr handler) { access_log_handlers_.push_back(handler); } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index b0d04acab75a2..dbed9dc089b3d 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -380,7 +380,7 @@ class ConnectionManagerImpl : Logger::Loggable, void addStreamDecoderFilter(StreamDecoderFilterPtr filter) override; void addStreamEncoderFilter(StreamEncoderFilterPtr filter) override; void addStreamFilter(StreamFilterPtr filter) override; - void addAccessLogHandler(AccessLogHandlerPtr handler) override; + void addAccessLogHandler(Http::AccessLog::InstancePtr handler) override; // Tracing::TracingConfig virtual const std::string& operationName() const override; @@ -407,7 +407,7 @@ class ConnectionManagerImpl : Logger::Loggable, HeaderMapPtr request_trailers_; std::list decoder_filters_; std::list encoder_filters_; - std::list access_log_handlers_; + std::list access_log_handlers_; Stats::TimespanPtr request_timer_; std::list> reset_callbacks_; State state_; From 6e015174f045f8dee6ae6cd6039f879f1292a432 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Tue, 31 Jan 2017 19:11:16 -0800 Subject: [PATCH 3/4] Add access log handler test. --- test/common/http/conn_manager_impl_test.cc | 47 +++++++++++++++++++++- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index bea59e344426a..606150dfe9593 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -3,13 +3,13 @@ #include "envoy/http/access_log.h" #include "common/buffer/buffer_impl.h" -#include "common/http/access_log/access_log_impl.h" #include "common/http/access_log/access_log_formatter.h" +#include "common/http/access_log/access_log_impl.h" #include "common/http/conn_manager_impl.h" #include "common/http/date_provider_impl.h" #include "common/http/exception.h" -#include "common/http/headers.h" #include "common/http/header_map_impl.h" +#include "common/http/headers.h" #include "common/stats/stats_impl.h" #include "test/mocks/access_log/mocks.h" @@ -239,6 +239,49 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { conn_manager_->onData(fake_input); } +TEST_F(HttpConnectionManagerImplTest, TestAccessLog) { + setup(false, ""); + + std::shared_ptr filter( + new NiceMock()); + std::shared_ptr handler( + new NiceMock()); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillOnce(Invoke([&](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(filter); + callbacks.addAccessLogHandler(handler); + })); + + EXPECT_CALL(*handler, log(_, _, _)) + .WillOnce(Invoke([](const Http::HeaderMap*, const Http::HeaderMap*, + const Http::AccessLog::RequestInfo& request_info) { + EXPECT_TRUE(request_info.responseCode().valid()); + EXPECT_EQ(request_info.responseCode().value(), uint32_t(200)); + })); + + Http::StreamDecoder* decoder = nullptr; + NiceMock encoder; + EXPECT_CALL(*codec_, dispatch(_)) + .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(encoder); + + Http::HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, + {":path", "/"}, + {"x-request-id", "125a4afb-6f55-a4ba-ad80-413f09f48a28"}}}; + decoder->decodeHeaders(std::move(headers), true); + + Http::HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; + filter->callbacks_->encodeHeaders(std::move(response_headers), true); + + data.drain(4); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input); +} + TEST_F(HttpConnectionManagerImplTest, DoNotStartSpanIfTracingIsNotEnabled) { setup(false, ""); From 9b8ebe5526aba77c9faf2e386fe21903a2552988 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Wed, 1 Feb 2017 09:44:18 -0800 Subject: [PATCH 4/4] Update the comment. --- include/envoy/http/filter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 8ee8137003dcc..d8d47e225bc3c 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -294,7 +294,7 @@ class FilterChainFactoryCallbacks { virtual void addStreamFilter(Http::StreamFilterPtr filter) PURE; /** - * Add an access log handler that is called after response is send. + * Add an access log handler that is called when the stream is destroyed. * @param handler supplies the handler to add. */ virtual void addAccessLogHandler(Http::AccessLog::InstancePtr handler) PURE;