From d8ae623cd0cfd4b081f3552c2bbf79c1d472800a Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 9 Jul 2019 10:04:31 -0400 Subject: [PATCH 1/2] object tracking on http encode Signed-off-by: Alyssa Wilk --- include/envoy/http/BUILD | 1 + include/envoy/http/filter.h | 6 ++++++ source/common/http/async_client_impl.h | 12 +++++++++++- source/common/http/conn_manager_impl.h | 1 + source/common/router/BUILD | 1 + source/common/router/router.cc | 11 +++++++++++ test/common/http/async_client_impl_test.cc | 15 +++++++++++++++ test/common/router/router_test.cc | 6 ++++++ test/common/router/router_upstream_log_test.cc | 1 + test/mocks/common.h | 5 +++++ test/mocks/http/mocks.cc | 2 ++ test/mocks/http/mocks.h | 4 ++++ 12 files changed, 64 insertions(+), 1 deletion(-) diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index 4f0355015ce3..ce03a17f9624 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -62,6 +62,7 @@ envoy_cc_library( ":codec_interface", ":header_map_interface", "//include/envoy/access_log:access_log_interface", + "//include/envoy/common:scope_tracker_interface", "//include/envoy/event:dispatcher_interface", "//include/envoy/grpc:status", "//include/envoy/router:router_interface", diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 1e1bacaab3fc..9dc0d6a6f8d6 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -6,6 +6,7 @@ #include #include "envoy/access_log/access_log.h" +#include "envoy/common/scope_tracker.h" #include "envoy/event/dispatcher.h" #include "envoy/grpc/status.h" #include "envoy/http/codec.h" @@ -185,6 +186,11 @@ class StreamFilterCallbacks { * @return tracing configuration. */ virtual const Tracing::Config& tracingConfig() PURE; + + /** + * @return the ScopeTrackedObject for this stream. + */ + virtual const ScopeTrackedObject& scope() PURE; }; /** diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 9830b98627cc..1276587c7796 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -9,6 +9,7 @@ #include #include +#include "envoy/common/scope_tracker.h" #include "envoy/config/typed_metadata.h" #include "envoy/event/dispatcher.h" #include "envoy/http/async_client.h" @@ -73,7 +74,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, public StreamDecoderFilterCallbacks, public Event::DeferredDeletable, Logger::Loggable, - LinkedObject { + LinkedObject, + public ScopeTrackedObject { public: AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCallbacks& callbacks, const AsyncClient::StreamOptions& options); @@ -339,9 +341,17 @@ class AsyncStreamImpl : public AsyncClient::Stream, void setDecoderBufferLimit(uint32_t) override {} uint32_t decoderBufferLimit() override { return 0; } bool recreateStream() override { return false; } + const ScopeTrackedObject& scope() override { return *this; } void addUpstreamSocketOptions(const Network::Socket::OptionsSharedPtr&) override {} Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const override { return {}; } + // ScopeTrackedObject + void dumpState(std::ostream& os, int indent_level) const override { + const char* spaces = spacesForLevel(indent_level); + os << spaces << "AsyncClient " << this << DUMP_MEMBER(stream_id_) << "\n"; + DUMP_DETAILS(&stream_info_); + } + AsyncClient::StreamCallbacks& stream_callbacks_; const uint64_t stream_id_; Router::ProdFilter router_; diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index a7ef3ef9eae8..ad0f005c52ad 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -140,6 +140,7 @@ class ConnectionManagerImpl : Logger::Loggable, StreamInfo::StreamInfo& streamInfo() override; Tracing::Span& activeSpan() override; Tracing::Config& tracingConfig() override; + const ScopeTrackedObject& scope() override { return parent_; } // Functions to set or get iteration state. bool canIterate() { return iteration_state_ == IterationState::Continue; } diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 445cc2232673..6e07d093018a 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -240,6 +240,7 @@ envoy_cc_library( "//source/common/common:hex_lib", "//source/common/common:linked_object", "//source/common/common:minimal_logger_lib", + "//source/common/common:scope_tracker", "//source/common/common:utility_lib", "//source/common/grpc:common_lib", "//source/common/http:codes_lib", diff --git a/source/common/router/router.cc b/source/common/router/router.cc index ca379197b838..b229fedfde6d 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -16,6 +16,7 @@ #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/common/enum_to_int.h" +#include "common/common/scope_tracker.h" #include "common/common/utility.h" #include "common/grpc/common.h" #include "common/http/codes.h" @@ -1329,11 +1330,15 @@ Filter::UpstreamRequest::~UpstreamRequest() { } void Filter::UpstreamRequest::decode100ContinueHeaders(Http::HeaderMapPtr&& headers) { + ScopeTrackerScopeState scope(&parent_.callbacks_->scope(), parent_.callbacks_->dispatcher()); + ASSERT(100 == Http::Utility::getResponseStatus(*headers)); parent_.onUpstream100ContinueHeaders(std::move(headers), *this); } void Filter::UpstreamRequest::decodeHeaders(Http::HeaderMapPtr&& headers, bool end_stream) { + ScopeTrackerScopeState scope(&parent_.callbacks_->scope(), parent_.callbacks_->dispatcher()); + // TODO(rodaine): This is actually measuring after the headers are parsed and not the first byte. upstream_timing_.onFirstUpstreamRxByteReceived(parent_.callbacks_->dispatcher().timeSource()); maybeEndDecode(end_stream); @@ -1348,12 +1353,16 @@ void Filter::UpstreamRequest::decodeHeaders(Http::HeaderMapPtr&& headers, bool e } void Filter::UpstreamRequest::decodeData(Buffer::Instance& data, bool end_stream) { + ScopeTrackerScopeState scope(&parent_.callbacks_->scope(), parent_.callbacks_->dispatcher()); + maybeEndDecode(end_stream); stream_info_.addBytesReceived(data.length()); parent_.onUpstreamData(data, *this, end_stream); } void Filter::UpstreamRequest::decodeTrailers(Http::HeaderMapPtr&& trailers) { + ScopeTrackerScopeState scope(&parent_.callbacks_->scope(), parent_.callbacks_->dispatcher()); + maybeEndDecode(true); if (!parent_.config_.upstream_logs_.empty()) { upstream_trailers_ = std::make_unique(*trailers); @@ -1425,6 +1434,8 @@ void Filter::UpstreamRequest::encodeTrailers(const Http::HeaderMap& trailers) { void Filter::UpstreamRequest::onResetStream(Http::StreamResetReason reason, absl::string_view transport_failure_reason) { + ScopeTrackerScopeState scope(&parent_.callbacks_->scope(), parent_.callbacks_->dispatcher()); + clearRequestEncoder(); awaiting_headers_ = false; if (!calling_encode_headers_) { diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index 60fdcf67b4d3..9a58c3f4f83d 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -908,6 +908,21 @@ TEST_F(AsyncClientImplTest, RdsGettersTest) { EXPECT_CALL(stream_callbacks_, onReset()); } +TEST_F(AsyncClientImplTest, DumpState) { + TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + AsyncClient::Stream* stream = client_.start(stream_callbacks_, AsyncClient::StreamOptions()); + Http::StreamDecoderFilterCallbacks* filter_callbacks = + static_cast(stream); + + std::stringstream out; + filter_callbacks->scope().dumpState(out); + std::string state = out.str(); + EXPECT_THAT(state, testing::HasSubstr("protocol_: 1")); + + EXPECT_CALL(stream_callbacks_, onReset()); +} + } // namespace // Must not be in anonymous namespace for friend to work. diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index b9e2f9dc8172..00444c8efa0c 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -33,6 +33,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::AnyNumber; using testing::AssertionFailure; using testing::AssertionResult; using testing::AssertionSuccess; @@ -98,6 +99,8 @@ class RouterTestBase : public testing::Test { // Make the "system time" non-zero, because 0 is considered invalid by DateUtil. test_time_.setMonotonicTime(std::chrono::milliseconds(50)); + + EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(AnyNumber()); } void expectResponseTimerCreate() { @@ -1292,10 +1295,13 @@ TEST_F(RouterTest, GrpcOk) { HttpTestUtility::addDefaultHeaders(headers); router_.decodeHeaders(headers, true); + EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(2); Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}}); EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); response_decoder->decodeHeaders(std::move(response_headers), false); EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + + EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(2); Http::HeaderMapPtr response_trailers(new Http::TestHeaderMapImpl{{"grpc-status", "0"}}); response_decoder->decodeTrailers(std::move(response_trailers)); EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); diff --git a/test/common/router/router_upstream_log_test.cc b/test/common/router/router_upstream_log_test.cc index 17e9a4c978d1..ce4574fe9358 100644 --- a/test/common/router/router_upstream_log_test.cc +++ b/test/common/router/router_upstream_log_test.cc @@ -91,6 +91,7 @@ class RouterUpstreamLogTest : public testing::Test { router_proto)); router_.reset(new TestFilter(*config_)); router_->setDecoderFilterCallbacks(callbacks_); + EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(testing::AnyNumber()); upstream_locality_.set_zone("to_az"); diff --git a/test/mocks/common.h b/test/mocks/common.h index 77637b83ade2..4e8f33a067c0 100644 --- a/test/mocks/common.h +++ b/test/mocks/common.h @@ -2,6 +2,7 @@ #include +#include "envoy/common/scope_tracker.h" #include "envoy/common/time.h" #include "envoy/common/token_bucket.h" #include "envoy/event/timer.h" @@ -86,4 +87,8 @@ inline bool operator==(const StringViewSaver& saver, const char* str) { return saver.value() == str; } +class MockScopedTrackedObject : public ScopeTrackedObject { + MOCK_CONST_METHOD2(dumpState, void(std::ostream&, int)); +}; + } // namespace Envoy diff --git a/test/mocks/http/mocks.cc b/test/mocks/http/mocks.cc index c30c9bc400ea..0f1161c5725e 100644 --- a/test/mocks/http/mocks.cc +++ b/test/mocks/http/mocks.cc @@ -65,6 +65,7 @@ MockStreamDecoderFilterCallbacks::MockStreamDecoderFilterCallbacks() { ON_CALL(*this, activeSpan()).WillByDefault(ReturnRef(active_span_)); ON_CALL(*this, tracingConfig()).WillByDefault(ReturnRef(tracing_config_)); + ON_CALL(*this, scope()).WillByDefault(ReturnRef(scope_)); ON_CALL(*this, sendLocalReply(_, _, _, _, _)) .WillByDefault(Invoke([this](Code code, absl::string_view body, std::function modify_headers, @@ -97,6 +98,7 @@ MockStreamEncoderFilterCallbacks::MockStreamEncoderFilterCallbacks() { ON_CALL(*this, encodingBuffer()).WillByDefault(Invoke(&buffer_, &Buffer::InstancePtr::get)); ON_CALL(*this, activeSpan()).WillByDefault(ReturnRef(active_span_)); ON_CALL(*this, tracingConfig()).WillByDefault(ReturnRef(tracing_config_)); + ON_CALL(*this, scope()).WillByDefault(ReturnRef(scope_)); } MockStreamEncoderFilterCallbacks::~MockStreamEncoderFilterCallbacks() = default; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 64ebef385c5f..cd1f54ca22e4 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -141,6 +141,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(streamInfo, StreamInfo::StreamInfo&()); MOCK_METHOD0(activeSpan, Tracing::Span&()); MOCK_METHOD0(tracingConfig, Tracing::Config&()); + MOCK_METHOD0(scope, const ScopeTrackedObject&()); MOCK_METHOD0(onDecoderFilterAboveWriteBufferHighWatermark, void()); MOCK_METHOD0(onDecoderFilterBelowWriteBufferLowWatermark, void()); MOCK_METHOD1(addDownstreamWatermarkCallbacks, void(DownstreamWatermarkCallbacks&)); @@ -188,6 +189,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, std::list callbacks_{}; testing::NiceMock active_span_; testing::NiceMock tracing_config_; + testing::NiceMock scope_; std::string details_; bool is_grpc_request_{}; bool is_head_request_{false}; @@ -211,6 +213,7 @@ class MockStreamEncoderFilterCallbacks : public StreamEncoderFilterCallbacks, MOCK_METHOD0(streamInfo, StreamInfo::StreamInfo&()); MOCK_METHOD0(activeSpan, Tracing::Span&()); MOCK_METHOD0(tracingConfig, Tracing::Config&()); + MOCK_METHOD0(scope, const ScopeTrackedObject&()); MOCK_METHOD0(onEncoderFilterAboveWriteBufferHighWatermark, void()); MOCK_METHOD0(onEncoderFilterBelowWriteBufferLowWatermark, void()); MOCK_METHOD1(setEncoderBufferLimit, void(uint32_t)); @@ -227,6 +230,7 @@ class MockStreamEncoderFilterCallbacks : public StreamEncoderFilterCallbacks, Buffer::InstancePtr buffer_; testing::NiceMock active_span_; testing::NiceMock tracing_config_; + testing::NiceMock scope_; }; class MockStreamDecoderFilter : public StreamDecoderFilter { From bc54e49525f5637cf6812802941eda3a02e897ea Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 22 Jul 2019 14:51:13 -0400 Subject: [PATCH 2/2] comment Signed-off-by: Alyssa Wilk --- test/common/router/router_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index f8662df00abc..09e769996a03 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -100,6 +100,7 @@ class RouterTestBase : public testing::Test { // Make the "system time" non-zero, because 0 is considered invalid by DateUtil. test_time_.setMonotonicTime(std::chrono::milliseconds(50)); + // Allow any number of setTrackedObject calls for the dispatcher strict mock. EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(AnyNumber()); }