Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: tracking object scope on the encode path #7603

Merged
merged 3 commits into from
Jul 23, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/envoy/http/BUILD
Original file line number Diff line number Diff line change
@@ -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",
6 changes: 6 additions & 0 deletions include/envoy/http/filter.h
Original file line number Diff line number Diff line change
@@ -6,6 +6,7 @@
#include <string>

#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;
};

/**
12 changes: 11 additions & 1 deletion source/common/http/async_client_impl.h
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@
#include <string>
#include <vector>

#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<Logger::Id::http>,
LinkedObject<AsyncStreamImpl> {
LinkedObject<AsyncStreamImpl>,
public ScopeTrackedObject {
public:
AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCallbacks& callbacks,
const AsyncClient::StreamOptions& options);
@@ -340,9 +342,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_;
1 change: 1 addition & 0 deletions source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
@@ -144,6 +144,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
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; }
1 change: 1 addition & 0 deletions source/common/router/BUILD
Original file line number Diff line number Diff line change
@@ -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",
11 changes: 11 additions & 0 deletions source/common/router/router.cc
Original file line number Diff line number Diff line change
@@ -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"
@@ -1339,11 +1340,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);
@@ -1358,12 +1363,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<Http::HeaderMapImpl>(*trailers);
@@ -1452,6 +1461,8 @@ void Filter::UpstreamRequest::encodeMetadata(Http::MetadataMapPtr&& metadata_map

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_) {
15 changes: 15 additions & 0 deletions test/common/http/async_client_impl_test.cc
Original file line number Diff line number Diff line change
@@ -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<Http::AsyncStreamImpl*>(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.
7 changes: 7 additions & 0 deletions test/common/router/router_test.cc
Original file line number Diff line number Diff line change
@@ -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,9 @@ 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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed due to a strict mock? If so can you comment? If not can we remove? Same below?

}

void expectResponseTimerCreate() {
@@ -1292,10 +1296,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));
1 change: 1 addition & 0 deletions test/common/router/router_upstream_log_test.cc
Original file line number Diff line number Diff line change
@@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry can you add a similar comment here about the strict mock? Fine to do in your next change if you want.


upstream_locality_.set_zone("to_az");

5 changes: 5 additions & 0 deletions test/mocks/common.h
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@

#include <cstdint>

#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
2 changes: 2 additions & 0 deletions test/mocks/http/mocks.cc
Original file line number Diff line number Diff line change
@@ -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<void(HeaderMap & headers)> 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;
4 changes: 4 additions & 0 deletions test/mocks/http/mocks.h
Original file line number Diff line number Diff line change
@@ -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&));
@@ -189,6 +190,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks,
std::list<DownstreamWatermarkCallbacks*> callbacks_{};
testing::NiceMock<Tracing::MockSpan> active_span_;
testing::NiceMock<Tracing::MockConfig> tracing_config_;
testing::NiceMock<MockScopedTrackedObject> scope_;
std::string details_;
bool is_grpc_request_{};
bool is_head_request_{false};
@@ -212,6 +214,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));
@@ -228,6 +231,7 @@ class MockStreamEncoderFilterCallbacks : public StreamEncoderFilterCallbacks,
Buffer::InstancePtr buffer_;
testing::NiceMock<Tracing::MockSpan> active_span_;
testing::NiceMock<Tracing::MockConfig> tracing_config_;
testing::NiceMock<MockScopedTrackedObject> scope_;
};

class MockStreamDecoderFilter : public StreamDecoderFilter {