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
Show file tree
Hide file tree
Changes from 2 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
Expand Up @@ -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",
Expand Down
6 changes: 6 additions & 0 deletions include/envoy/http/filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
};

/**
Expand Down
12 changes: 11 additions & 1 deletion source/common/http/async_client_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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_;
Expand Down
1 change: 1 addition & 0 deletions source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
1 change: 1 addition & 0 deletions source/common/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
11 changes: 11 additions & 0 deletions source/common/router/router.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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_) {
Expand Down
15 changes: 15 additions & 0 deletions test/common/http/async_client_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions test/common/router/router_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "gtest/gtest.h"

using testing::_;
using testing::AnyNumber;
using testing::AssertionFailure;
using testing::AssertionResult;
using testing::AssertionSuccess;
Expand Down Expand Up @@ -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());
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() {
Expand Down Expand Up @@ -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));
Expand Down
1 change: 1 addition & 0 deletions test/common/router/router_upstream_log_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down
5 changes: 5 additions & 0 deletions test/mocks/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions test/mocks/http/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -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&));
Expand Down Expand Up @@ -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};
Expand All @@ -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));
Expand All @@ -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 {
Expand Down