From 860d066d5070c6b0a5a5c0ab2975c1bd38909ab9 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Thu, 1 Aug 2019 14:02:16 -0700 Subject: [PATCH 01/17] Adaptive concurrency boilerplate Signed-off-by: Tony Allen --- CODEOWNERS | 2 + .../http/adaptive_concurrency/v2alpha/BUILD | 11 ++ .../v2alpha/adaptive_concurrency.proto | 11 ++ .../filters/http/adaptive_concurrency/BUILD | 43 ++++++++ .../adaptive_concurrency_filter.cc | 46 ++++++++ .../adaptive_concurrency_filter.h | 102 ++++++++++++++++++ .../concurrency_controller/BUILD | 24 +++++ .../concurrency_controller.h | 38 +++++++ .../concurrency_controller/noop_controller.h | 27 +++++ .../http/adaptive_concurrency/config.cc | 41 +++++++ .../http/adaptive_concurrency/config.h | 36 +++++++ .../filters/http/well_known_names.h | 2 + 12 files changed, 383 insertions(+) create mode 100644 api/envoy/config/filter/http/adaptive_concurrency/v2alpha/BUILD create mode 100644 api/envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.proto create mode 100644 source/extensions/filters/http/adaptive_concurrency/BUILD create mode 100644 source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc create mode 100644 source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h create mode 100644 source/extensions/filters/http/adaptive_concurrency/concurrency_controller/BUILD create mode 100644 source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h create mode 100644 source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h create mode 100644 source/extensions/filters/http/adaptive_concurrency/config.cc create mode 100644 source/extensions/filters/http/adaptive_concurrency/config.h diff --git a/CODEOWNERS b/CODEOWNERS index a252439569d6..9576cbb5a308 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -44,5 +44,7 @@ extensions/filters/common/original_src @snowp @klarose /*/extensions/filters/http/dynamic_forward_proxy @mattklein123 @alyssawilk # omit_canary_hosts retry predicate /*/extensions/retry/host/omit_canary_hosts @sriduth @snowp +# adaptive concurrency limit extension. +/*/extensions/filters/http/adaptive_concurrency @tonya11en @mattklein123 # http inspector /*/extensions/filters/listener/http_inspector @crazyxy @PiotrSikora @lizan diff --git a/api/envoy/config/filter/http/adaptive_concurrency/v2alpha/BUILD b/api/envoy/config/filter/http/adaptive_concurrency/v2alpha/BUILD new file mode 100644 index 000000000000..948ceec2223d --- /dev/null +++ b/api/envoy/config/filter/http/adaptive_concurrency/v2alpha/BUILD @@ -0,0 +1,11 @@ +load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library_internal") + +licenses(["notice"]) # Apache 2 + +api_proto_library_internal( + name = "adaptive_concurrency", + srcs = ["adaptive_concurrency.proto"], + deps = [ + "//envoy/api/v2/core:base", + ], +) diff --git a/api/envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.proto b/api/envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.proto new file mode 100644 index 000000000000..ff19657260e8 --- /dev/null +++ b/api/envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.proto @@ -0,0 +1,11 @@ +syntax = "proto3"; + +package envoy.config.filter.http.adaptive_concurrency.v2alpha; + +option java_package = "io.envoyproxy.envoy.config.filter.http.adaptive_concurrency.v2alpha"; +option java_outer_classname = "AdaptiveConcurrencyProto"; +option java_multiple_files = true; +option go_package = "v2alpha"; + +message AdaptiveConcurrency { +} diff --git a/source/extensions/filters/http/adaptive_concurrency/BUILD b/source/extensions/filters/http/adaptive_concurrency/BUILD new file mode 100644 index 000000000000..3f08db25b387 --- /dev/null +++ b/source/extensions/filters/http/adaptive_concurrency/BUILD @@ -0,0 +1,43 @@ +licenses(["notice"]) # Apache 2 + +# HTTP L7 filter that dynamically adjusts the number of allowed concurrent +# requests based on sampled latencies. +# Public docs: TODO (tonya11en) + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "adaptive_concurrency_filter_lib", + srcs = ["adaptive_concurrency_filter.cc"], + hdrs = ["adaptive_concurrency_filter.h"], + deps = [ + "//include/envoy/http:filter_interface", + "//include/envoy/runtime:runtime_interface", + "//include/envoy/stats:stats_interface", + "//include/envoy/stats:stats_macros", + "//source/common/common:assert_lib", + "//source/common/protobuf:utility_lib", + "//source/extensions/filters/http:well_known_names", + "//source/extensions/filters/http/adaptive_concurrency/concurrency_controller:concurrency_controller_lib", + "@envoy_api//envoy/config/filter/http/adaptive_concurrency/v2alpha:adaptive_concurrency_cc", + ], +) + +envoy_cc_library( + name = "config", + srcs = ["config.cc"], + hdrs = ["config.h"], + deps = [ + "//include/envoy/registry", + "//source/common/config:filter_json_lib", + "//source/extensions/filters/http:well_known_names", + "//source/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_filter_lib", + "//source/extensions/filters/http/common:factory_base_lib", + ], +) diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc new file mode 100644 index 000000000000..9dc0ffba7136 --- /dev/null +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -0,0 +1,46 @@ +#include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" + +#include +#include +#include +#include + +#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" +#include "extensions/filters/http/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace AdaptiveConcurrency { + +AdaptiveConcurrencyFilterConfig::AdaptiveConcurrencyFilterConfig( + const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency&, + Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope, + TimeSource& time_source) + : runtime_(runtime), stats_prefix_(stats_prefix), scope_(scope), time_source_(time_source) {} + +AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( + AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller) + : config_(config), controller_(controller) {} + +AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() {} + +Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) { + // TODO (tonya11en). + return Http::FilterHeadersStatus::Continue; +} + +void AdaptiveConcurrencyFilter::onDestroy() { + // TODO (tonya11en). +} + +Http::FilterHeadersStatus AdaptiveConcurrencyFilter::encodeHeaders(Http::HeaderMap&, + bool /*end_stream*/) { + // TODO (tonya11en). + return Http::FilterHeadersStatus::Continue; +} + +} // namespace AdaptiveConcurrency +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h new file mode 100644 index 000000000000..9b9c2a145da6 --- /dev/null +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -0,0 +1,102 @@ +#pragma once + +#include +#include +#include +#include +#include + +#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" +#include "envoy/http/filter.h" +#include "envoy/runtime/runtime.h" +#include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" + +#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace AdaptiveConcurrency { + +/** + * Configuration for the adaptive concurrency limit filter. + */ +class AdaptiveConcurrencyFilterConfig { +public: + AdaptiveConcurrencyFilterConfig( + const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency& + adaptive_concurrency, + Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope, + TimeSource& time_source); + + Runtime::Loader& runtime() { return runtime_; } + const std::string& statsPrefix() { return stats_prefix_; } + Stats::Scope& scope() { return scope_; } + TimeSource& timeSource() { return time_source_; } + +private: + Runtime::Loader& runtime_; + const std::string stats_prefix_; + Stats::Scope& scope_; + TimeSource& time_source_; +}; + +using AdaptiveConcurrencyFilterConfigSharedPtr = std::shared_ptr; +using ConcurrencyControllerSharedPtr = + std::shared_ptr; + +/** + * A filter that samples request latencies and dynamically adjusts the request + * concurrency window. + */ +class AdaptiveConcurrencyFilter : public Http::StreamFilter, Logger::Loggable { +public: + AdaptiveConcurrencyFilter(AdaptiveConcurrencyFilterConfigSharedPtr config, + ConcurrencyControllerSharedPtr controller); + ~AdaptiveConcurrencyFilter() override; + + // Http::StreamFilterBase + void onDestroy() override; + + // Http::StreamDecoderFilter + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool end_stream) override; + Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override { + return Http::FilterDataStatus::Continue; + } + Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) override { + return Http::FilterTrailersStatus::Continue; + } + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override { + decoder_callbacks_ = &callbacks; + } + + // Http::StreamEncoderFilter + Http::FilterHeadersStatus encode100ContinueHeaders(Http::HeaderMap&) override { + return Http::FilterHeadersStatus::Continue; + } + Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap&, bool) override; + Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override { + return Http::FilterDataStatus::Continue; + } + Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap&) override { + return Http::FilterTrailersStatus::Continue; + } + Http::FilterMetadataStatus encodeMetadata(Http::MetadataMap&) override { + return Http::FilterMetadataStatus::Continue; + } + void setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callbacks) override { + encoder_callbacks_ = &callbacks; + } + +private: + AdaptiveConcurrencyFilterConfigSharedPtr config_; + Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; + Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; + ConcurrencyControllerSharedPtr controller_; +}; + +} // namespace AdaptiveConcurrency +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/BUILD b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/BUILD new file mode 100644 index 000000000000..75763e2bd8f4 --- /dev/null +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/BUILD @@ -0,0 +1,24 @@ +licenses(["notice"]) # Apache 2 + +# HTTP L7 filter that dynamically adjusts the number of allowed concurrent +# requests based on sampled latencies. +# Public docs: TODO (tonya11en) + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "concurrency_controller_lib", + srcs = [], + hdrs = [ + "concurrency_controller.h", + "noop_controller.h", + ], + deps = [ + ], +) diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h new file mode 100644 index 000000000000..38840ae5f58c --- /dev/null +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h @@ -0,0 +1,38 @@ +#pragma once + +#include + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace AdaptiveConcurrency { +namespace ConcurrencyController { + +/** + * Adaptive concurrency controller interface. + */ +class ConcurrencyController { +public: + virtual ~ConcurrencyController() = default; + + /** + * Called during decoding when the adaptive concurrency filter is attempting + * to forward a request. Returns true if a request is successfully forwarded. + */ + virtual bool tryForwardRequest() PURE; + + /** + * Called during encoding when the request latency is known. Records the + * request latency to update the internal state of the controller for + * concurrency limit calculations. + * + * @param rq_latency is the clocked round-trip time for the request. + */ + virtual void recordLatencySample(const std::chrono::nanoseconds& rq_latency) PURE; +}; + +} // namespace ConcurrencyController +} // namespace AdaptiveConcurrency +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h new file mode 100644 index 000000000000..c304e3313458 --- /dev/null +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h @@ -0,0 +1,27 @@ +#pragma once + +#include + +#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace AdaptiveConcurrency { +namespace ConcurrencyController { + +/** + * Adaptive concurrency controller that does nothing. + */ +class NoopController : public ConcurrencyController { +public: + // ConcurrencyController. + bool tryForwardRequest() override { return true; } + void recordLatencySample(const std::chrono::nanoseconds&) override {} +}; + +} // namespace ConcurrencyController +} // namespace AdaptiveConcurrency +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/adaptive_concurrency/config.cc b/source/extensions/filters/http/adaptive_concurrency/config.cc new file mode 100644 index 000000000000..70a7c22575a7 --- /dev/null +++ b/source/extensions/filters/http/adaptive_concurrency/config.cc @@ -0,0 +1,41 @@ +#include "extensions/filters/http/adaptive_concurrency/config.h" + +#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.validate.h" +#include "envoy/registry/registry.h" + +#include "common/config/filter_json.h" + +#include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" +#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace AdaptiveConcurrency { + +Http::FilterFactoryCb AdaptiveConcurrencyFilterFactory::createFilterFactoryFromProtoTyped( + const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency& config, + const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { + + // TODO (tonya11en): Noop controller needs to be replaced with an actual + // implementation in a future patch. + auto noop_ctl = std::make_shared(); + + AdaptiveConcurrencyFilterConfigSharedPtr filter_config(new AdaptiveConcurrencyFilterConfig( + config, context.runtime(), stats_prefix, context.scope(), context.timeSource())); + + return [filter_config, noop_ctl](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(std::make_shared(filter_config, noop_ctl)); + }; +} + +/** + * Static registration for the adaptive_concurrency filter. @see RegisterFactory. + */ +REGISTER_FACTORY(AdaptiveConcurrencyFilterFactory, + Server::Configuration::NamedHttpFilterConfigFactory); + +} // namespace AdaptiveConcurrency +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/adaptive_concurrency/config.h b/source/extensions/filters/http/adaptive_concurrency/config.h new file mode 100644 index 000000000000..1091f3a803b6 --- /dev/null +++ b/source/extensions/filters/http/adaptive_concurrency/config.h @@ -0,0 +1,36 @@ +#pragma once + +#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" +#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.validate.h" + +#include "extensions/filters/http/common/factory_base.h" +#include "extensions/filters/http/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace AdaptiveConcurrency { + +/** + * Config registration for the adaptive concurrency limit filter. @see NamedHttpFilterConfigFactory. + */ +class AdaptiveConcurrencyFilterFactory + : public Common::FactoryBase< + envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency> { +public: + AdaptiveConcurrencyFilterFactory() : FactoryBase(HttpFilterNames::get().AdaptiveConcurrency) {} + + Http::FilterFactoryCb + createFilterFactory(const Json::Object& json_config, const std::string& stats_prefix, + Server::Configuration::FactoryContext& context) override; + + Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency& + proto_config, + const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; +}; + +} // namespace AdaptiveConcurrency +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/well_known_names.h b/source/extensions/filters/http/well_known_names.h index 3ce6643fa8fc..4f5c45a83026 100644 --- a/source/extensions/filters/http/well_known_names.h +++ b/source/extensions/filters/http/well_known_names.h @@ -54,6 +54,8 @@ class HttpFilterNameValues { const std::string HeaderToMetadata = "envoy.filters.http.header_to_metadata"; // Tap filter const std::string Tap = "envoy.filters.http.tap"; + // Adaptive concurrency limit filter. + const std::string AdaptiveConcurrency = "envoy.filters.http.adaptive_concurrency"; // Original Src Filter const std::string OriginalSrc = "envoy.filters.http.original_src"; // Dynamic forward proxy filter From 8884a4a0ebeb51e97e4893ffebf8598e722ffda7 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Fri, 2 Aug 2019 16:18:31 -0700 Subject: [PATCH 02/17] Remove excess deps Signed-off-by: Tony Allen --- source/extensions/filters/http/adaptive_concurrency/BUILD | 5 ----- 1 file changed, 5 deletions(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/BUILD b/source/extensions/filters/http/adaptive_concurrency/BUILD index 3f08db25b387..3ab5db00c1ca 100644 --- a/source/extensions/filters/http/adaptive_concurrency/BUILD +++ b/source/extensions/filters/http/adaptive_concurrency/BUILD @@ -18,11 +18,6 @@ envoy_cc_library( hdrs = ["adaptive_concurrency_filter.h"], deps = [ "//include/envoy/http:filter_interface", - "//include/envoy/runtime:runtime_interface", - "//include/envoy/stats:stats_interface", - "//include/envoy/stats:stats_macros", - "//source/common/common:assert_lib", - "//source/common/protobuf:utility_lib", "//source/extensions/filters/http:well_known_names", "//source/extensions/filters/http/adaptive_concurrency/concurrency_controller:concurrency_controller_lib", "@envoy_api//envoy/config/filter/http/adaptive_concurrency/v2alpha:adaptive_concurrency_cc", From 25d3c961c7ecbb14807f7df35d0117a7338e3c9b Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Fri, 2 Aug 2019 16:42:30 -0700 Subject: [PATCH 03/17] Hook up and use noop controller. Signed-off-by: Tony Allen --- .../adaptive_concurrency_filter.cc | 26 +++++++++++++++---- .../adaptive_concurrency_filter.h | 4 ++- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index 9dc0ffba7136..25cb40463848 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -25,9 +25,21 @@ AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() {} -Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) { - // TODO (tonya11en). - return Http::FilterHeadersStatus::Continue; +Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool end_stream) { + if (!end_stream) { + return Http::FilterHeadersStatus::Continue; + } + + if (controller_->tryForwardRequest()) { + rq_start_time_ = config_->timeSource().monotonicTime(); + return Http::FilterHeadersStatus::Continue; + } + + // TODO (tonya11en): Remove filler words. + decoder_callbacks_->sendLocalReply( + Http::Code::ServiceUnavailable, "filler words", nullptr, + absl::nullopt, "more filler words"); + return Http::FilterHeadersStatus::StopIteration; } void AdaptiveConcurrencyFilter::onDestroy() { @@ -35,8 +47,12 @@ void AdaptiveConcurrencyFilter::onDestroy() { } Http::FilterHeadersStatus AdaptiveConcurrencyFilter::encodeHeaders(Http::HeaderMap&, - bool /*end_stream*/) { - // TODO (tonya11en). + bool end_stream) { + if (end_stream) { + const std::chrono::nanoseconds rq_latency = config_->timeSource().monotonicTime() - rq_start_time_; + controller_->recordLatencySample(rq_latency); + } + return Http::FilterHeadersStatus::Continue; } diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h index 9b9c2a145da6..3f588184989f 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -6,12 +6,13 @@ #include #include -#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" #include "envoy/http/filter.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/scope.h" +#include "envoy/common/time.h" #include "envoy/stats/stats_macros.h" +#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" #include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" namespace Envoy { @@ -94,6 +95,7 @@ class AdaptiveConcurrencyFilter : public Http::StreamFilter, Logger::Loggable Date: Fri, 2 Aug 2019 16:55:31 -0700 Subject: [PATCH 04/17] cleanup Signed-off-by: Tony Allen --- .../concurrency_controller/concurrency_controller.h | 6 ++++-- .../extensions/filters/http/adaptive_concurrency/config.h | 4 ---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h index 38840ae5f58c..3204f2250681 100644 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h @@ -9,7 +9,8 @@ namespace AdaptiveConcurrency { namespace ConcurrencyController { /** - * Adaptive concurrency controller interface. + * Adaptive concurrency controller interface. All implementations of this + * interface must be thread-safe. */ class ConcurrencyController { public: @@ -17,7 +18,8 @@ class ConcurrencyController { /** * Called during decoding when the adaptive concurrency filter is attempting - * to forward a request. Returns true if a request is successfully forwarded. + * to forward a request. Returns true once the controller's internal state is + * updated and the request can be forwarded. */ virtual bool tryForwardRequest() PURE; diff --git a/source/extensions/filters/http/adaptive_concurrency/config.h b/source/extensions/filters/http/adaptive_concurrency/config.h index 1091f3a803b6..bce0e0fc6c71 100644 --- a/source/extensions/filters/http/adaptive_concurrency/config.h +++ b/source/extensions/filters/http/adaptive_concurrency/config.h @@ -20,10 +20,6 @@ class AdaptiveConcurrencyFilterFactory public: AdaptiveConcurrencyFilterFactory() : FactoryBase(HttpFilterNames::get().AdaptiveConcurrency) {} - Http::FilterFactoryCb - createFilterFactory(const Json::Object& json_config, const std::string& stats_prefix, - Server::Configuration::FactoryContext& context) override; - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency& proto_config, From a694091c397a6ec5cbda1d2f047b33e4b2336e77 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Fri, 2 Aug 2019 17:23:01 -0700 Subject: [PATCH 05/17] fix format Signed-off-by: Tony Allen --- .../adaptive_concurrency_filter.cc | 11 ++++++----- .../adaptive_concurrency_filter.h | 4 ++-- .../filters/http/adaptive_concurrency/config.cc | 3 ++- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index 25cb40463848..dd6a979cbaac 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -25,7 +25,8 @@ AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() {} -Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool end_stream) { +Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, + bool end_stream) { if (!end_stream) { return Http::FilterHeadersStatus::Continue; } @@ -36,9 +37,8 @@ Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderM } // TODO (tonya11en): Remove filler words. - decoder_callbacks_->sendLocalReply( - Http::Code::ServiceUnavailable, "filler words", nullptr, - absl::nullopt, "more filler words"); + decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "filler words", nullptr, + absl::nullopt, "more filler words"); return Http::FilterHeadersStatus::StopIteration; } @@ -49,7 +49,8 @@ void AdaptiveConcurrencyFilter::onDestroy() { Http::FilterHeadersStatus AdaptiveConcurrencyFilter::encodeHeaders(Http::HeaderMap&, bool end_stream) { if (end_stream) { - const std::chrono::nanoseconds rq_latency = config_->timeSource().monotonicTime() - rq_start_time_; + const std::chrono::nanoseconds rq_latency = + config_->timeSource().monotonicTime() - rq_start_time_; controller_->recordLatencySample(rq_latency); } diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h index 3f588184989f..8fb259a1070d 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -6,13 +6,13 @@ #include #include +#include "envoy/common/time.h" +#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" #include "envoy/http/filter.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/scope.h" -#include "envoy/common/time.h" #include "envoy/stats/stats_macros.h" -#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" #include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" namespace Envoy { diff --git a/source/extensions/filters/http/adaptive_concurrency/config.cc b/source/extensions/filters/http/adaptive_concurrency/config.cc index 70a7c22575a7..95929df82ad2 100644 --- a/source/extensions/filters/http/adaptive_concurrency/config.cc +++ b/source/extensions/filters/http/adaptive_concurrency/config.cc @@ -19,7 +19,8 @@ Http::FilterFactoryCb AdaptiveConcurrencyFilterFactory::createFilterFactoryFromP // TODO (tonya11en): Noop controller needs to be replaced with an actual // implementation in a future patch. - auto noop_ctl = std::make_shared(); + std::shared_ptr noop_ctl = + std::make_shared(); AdaptiveConcurrencyFilterConfigSharedPtr filter_config(new AdaptiveConcurrencyFilterConfig( config, context.runtime(), stats_prefix, context.scope(), context.timeSource())); From b99ac6247eee3522847027916a9582c74e8efc60 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Fri, 2 Aug 2019 21:32:44 -0700 Subject: [PATCH 06/17] tidy Signed-off-by: Tony Allen --- .../adaptive_concurrency/adaptive_concurrency_filter.cc | 8 ++++---- .../adaptive_concurrency/adaptive_concurrency_filter.h | 4 ++-- .../concurrency_controller/concurrency_controller.h | 2 ++ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index dd6a979cbaac..5619e5a3f58d 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -15,15 +15,15 @@ namespace AdaptiveConcurrency { AdaptiveConcurrencyFilterConfig::AdaptiveConcurrencyFilterConfig( const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency&, - Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope, + Runtime::Loader& runtime, std::string stats_prefix, Stats::Scope& scope, TimeSource& time_source) - : runtime_(runtime), stats_prefix_(stats_prefix), scope_(scope), time_source_(time_source) {} + : runtime_(runtime), stats_prefix_(std::move(stats_prefix)), scope_(scope), time_source_(time_source) {} AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller) - : config_(config), controller_(controller) {} + : config_(std::move(config)), controller_(std::move(controller)) {} -AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() {} +AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() = default; Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool end_stream) { diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h index 8fb259a1070d..a3f6932e4fda 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -28,11 +28,11 @@ class AdaptiveConcurrencyFilterConfig { AdaptiveConcurrencyFilterConfig( const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency& adaptive_concurrency, - Runtime::Loader& runtime, const std::string& stats_prefix, Stats::Scope& scope, + Runtime::Loader& runtime, std::string stats_prefix, Stats::Scope& scope, TimeSource& time_source); Runtime::Loader& runtime() { return runtime_; } - const std::string& statsPrefix() { return stats_prefix_; } + const std::string& statsPrefix() const { return stats_prefix_; } Stats::Scope& scope() { return scope_; } TimeSource& timeSource() { return time_source_; } diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h index 3204f2250681..a41bfff65b51 100644 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h @@ -2,6 +2,8 @@ #include +#include "envoy/common/pure.h" + namespace Envoy { namespace Extensions { namespace HttpFilters { From e873d817690edbcf14e01583627a6b542a467e86 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Fri, 2 Aug 2019 21:36:38 -0700 Subject: [PATCH 07/17] format Signed-off-by: Tony Allen --- .../http/adaptive_concurrency/adaptive_concurrency_filter.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index 5619e5a3f58d..d0decc7b2ca4 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -17,7 +17,8 @@ AdaptiveConcurrencyFilterConfig::AdaptiveConcurrencyFilterConfig( const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency&, Runtime::Loader& runtime, std::string stats_prefix, Stats::Scope& scope, TimeSource& time_source) - : runtime_(runtime), stats_prefix_(std::move(stats_prefix)), scope_(scope), time_source_(time_source) {} + : runtime_(runtime), stats_prefix_(std::move(stats_prefix)), scope_(scope), + time_source_(time_source) {} AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller) From 66bbf3e851af714570051e6e67d2a08123e0659b Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Sun, 4 Aug 2019 14:32:08 -0700 Subject: [PATCH 08/17] test passes Signed-off-by: Tony Allen --- source/extensions/extensions_build_config.bzl | 1 + .../filters/http/adaptive_concurrency/BUILD | 28 +++++ .../adaptive_concurrency_filter_test.cc | 117 ++++++++++++++++++ 3 files changed, 146 insertions(+) create mode 100644 test/extensions/filters/http/adaptive_concurrency/BUILD create mode 100644 test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index c18d6d6fa47e..c77c8dbacc9d 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -52,6 +52,7 @@ EXTENSIONS = { "envoy.filters.http.router": "//source/extensions/filters/http/router:config", "envoy.filters.http.squash": "//source/extensions/filters/http/squash:config", "envoy.filters.http.tap": "//source/extensions/filters/http/tap:config", + "envoy.filters.http.adaptive_concurrency": "//source/extensions/filters/http/adaptive_concurrency:config", # # Listener filters diff --git a/test/extensions/filters/http/adaptive_concurrency/BUILD b/test/extensions/filters/http/adaptive_concurrency/BUILD new file mode 100644 index 000000000000..14aa0a108be2 --- /dev/null +++ b/test/extensions/filters/http/adaptive_concurrency/BUILD @@ -0,0 +1,28 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test_library", + "envoy_package", +) +load( + "//test/extensions:extensions_build_system.bzl", + "envoy_extension_cc_test", +) + +envoy_package() + +envoy_extension_cc_test( + name = "adaptive_concurrency_filter_test", + srcs = ["adaptive_concurrency_filter_test.cc"], + extension_name = "envoy.filters.http.adaptive_concurrency", + deps = [ + "//source/common/http:header_map_lib", + "//source/common/http:headers_lib", + "//source/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_filter_lib", + "//source/extensions/filters/http/adaptive_concurrency/concurrency_controller:concurrency_controller_lib", + "//test/mocks/http:http_mocks", + "//test/test_common:simulated_time_system_lib", + "//test/test_common:utility_lib", + ], +) diff --git a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc new file mode 100644 index 000000000000..a7ecd8e4f4b2 --- /dev/null +++ b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc @@ -0,0 +1,117 @@ +#include + +#include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" +#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" + +#include "test/mocks/http/mocks.h" +#include "test/mocks/stream_info/mocks.h" +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::Return; + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace AdaptiveConcurrency { +namespace { + +class MockConcurrencyController : public ConcurrencyController::ConcurrencyController { +public: + MOCK_METHOD0(tryForwardRequest, bool()); + MOCK_METHOD1(recordLatencySample, void(const std::chrono::nanoseconds&)); +}; + +class AdaptiveConcurrencyFilterTest : public testing::Test { +public: + void SetupTest(); + + std::unique_ptr filter_; + Event::SimulatedTimeSystem time_system_; + Stats::IsolatedStoreImpl stats_; + NiceMock runtime_; + std::shared_ptr controller_{new MockConcurrencyController()}; + NiceMock decoder_callbacks_; +}; + +void AdaptiveConcurrencyFilterTest::SetupTest() { + filter_.reset(); + + const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency config; + auto config_ptr = std::make_shared( + config, runtime_, "testprefix.", stats_, time_system_); + + filter_ = std::make_unique(config_ptr, controller_); + filter_->setDecoderFilterCallbacks(decoder_callbacks_); +} + +// Verify the parts of the filter that aren't doing the work don't return +// anything unexpected. +TEST_F(AdaptiveConcurrencyFilterTest, UnusedFuncsTest) { + SetupTest(); + + Buffer::OwnedImpl request_body; + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(request_body, false)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(request_body, true)); + + Http::TestHeaderMapImpl request_trailers; + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers)); + + Http::TestHeaderMapImpl response_headers; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_->encode100ContinueHeaders(response_headers)); + + Buffer::OwnedImpl response_body; + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_body, false)); + + Http::TestHeaderMapImpl response_trailers; + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers)); + + Http::MetadataMap metadata; + EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_->encodeMetadata(metadata)); +} + +TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTest) { + SetupTest(); + + Http::TestHeaderMapImpl request_headers; + + EXPECT_CALL(*controller_, tryForwardRequest()).Times(1).WillRepeatedly(Return(true)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); + + EXPECT_CALL(*controller_, tryForwardRequest()).Times(1).WillRepeatedly(Return(false)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); + EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::ServiceUnavailable, _, _, _, _)) + .Times(1); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers, true)); +} + +TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersTest) { + SetupTest(); + + // Get the filter to record the request start time via decode. + Http::TestHeaderMapImpl request_headers; + EXPECT_CALL(*controller_, tryForwardRequest()).Times(1).WillRepeatedly(Return(true)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); + + const std::chrono::nanoseconds advance_time = std::chrono::milliseconds(42); + const auto mt = time_system_.monotonicTime(); + time_system_.setMonotonicTime(mt + advance_time); + + Http::TestHeaderMapImpl response_headers; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); + + EXPECT_CALL(*controller_, recordLatencySample(advance_time)).Times(1); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, true)); +} + +} // namespace +} // namespace AdaptiveConcurrency +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy From 4431de4efdbe077ba288c740ccb6231a06e3c4a0 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Mon, 5 Aug 2019 16:06:17 -0700 Subject: [PATCH 09/17] Address comments Signed-off-by: Tony Allen --- source/extensions/extensions_build_config.bzl | 4 +- .../filters/http/adaptive_concurrency/BUILD | 1 + .../adaptive_concurrency_filter.cc | 54 ++++++++-------- .../adaptive_concurrency_filter.h | 47 +++----------- .../concurrency_controller.h | 17 +++++- .../concurrency_controller/noop_controller.h | 2 +- .../adaptive_concurrency_filter_test.cc | 61 ++++++++----------- 7 files changed, 83 insertions(+), 103 deletions(-) diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index c77c8dbacc9d..3fcd0d2bf863 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -29,6 +29,7 @@ EXTENSIONS = { # HTTP filters # + "envoy.filters.http.adaptive_concurrency": "//source/extensions/filters/http/adaptive_concurrency:config", "envoy.filters.http.buffer": "//source/extensions/filters/http/buffer:config", "envoy.filters.http.cors": "//source/extensions/filters/http/cors:config", "envoy.filters.http.csrf": "//source/extensions/filters/http/csrf:config", @@ -37,9 +38,9 @@ EXTENSIONS = { "envoy.filters.http.ext_authz": "//source/extensions/filters/http/ext_authz:config", "envoy.filters.http.fault": "//source/extensions/filters/http/fault:config", "envoy.filters.http.grpc_http1_bridge": "//source/extensions/filters/http/grpc_http1_bridge:config", + "envoy.filters.http.grpc_http1_reverse_bridge": "//source/extensions/filters/http/grpc_http1_reverse_bridge:config", "envoy.filters.http.grpc_json_transcoder": "//source/extensions/filters/http/grpc_json_transcoder:config", "envoy.filters.http.grpc_web": "//source/extensions/filters/http/grpc_web:config", - "envoy.filters.http.grpc_http1_reverse_bridge": "//source/extensions/filters/http/grpc_http1_reverse_bridge:config", "envoy.filters.http.gzip": "//source/extensions/filters/http/gzip:config", "envoy.filters.http.header_to_metadata": "//source/extensions/filters/http/header_to_metadata:config", "envoy.filters.http.health_check": "//source/extensions/filters/http/health_check:config", @@ -52,7 +53,6 @@ EXTENSIONS = { "envoy.filters.http.router": "//source/extensions/filters/http/router:config", "envoy.filters.http.squash": "//source/extensions/filters/http/squash:config", "envoy.filters.http.tap": "//source/extensions/filters/http/tap:config", - "envoy.filters.http.adaptive_concurrency": "//source/extensions/filters/http/adaptive_concurrency:config", # # Listener filters diff --git a/source/extensions/filters/http/adaptive_concurrency/BUILD b/source/extensions/filters/http/adaptive_concurrency/BUILD index 3ab5db00c1ca..30a8ffc147a6 100644 --- a/source/extensions/filters/http/adaptive_concurrency/BUILD +++ b/source/extensions/filters/http/adaptive_concurrency/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( deps = [ "//include/envoy/http:filter_interface", "//source/extensions/filters/http:well_known_names", + "//source/extensions/filters/http/common:pass_through_filter_lib", "//source/extensions/filters/http/adaptive_concurrency/concurrency_controller:concurrency_controller_lib", "@envoy_api//envoy/config/filter/http/adaptive_concurrency/v2alpha:adaptive_concurrency_cc", ], diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index d0decc7b2ca4..c8dd8737a823 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -1,11 +1,11 @@ -#include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" - #include #include #include #include #include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" +#include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" +#include "common/common/assert.h" #include "extensions/filters/http/well_known_names.h" namespace Envoy { @@ -18,7 +18,14 @@ AdaptiveConcurrencyFilterConfig::AdaptiveConcurrencyFilterConfig( Runtime::Loader& runtime, std::string stats_prefix, Stats::Scope& scope, TimeSource& time_source) : runtime_(runtime), stats_prefix_(std::move(stats_prefix)), scope_(scope), - time_source_(time_source) {} + time_source_(time_source) { + // TODO (tonya11en): Remove these noop calls when stats/runtime values are + // implemented. + // + // Calling for test coverage. + runtime_.snapshot(); + scope_.constSymbolTable(); +} AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller) @@ -26,36 +33,33 @@ AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() = default; -Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, - bool end_stream) { - if (!end_stream) { - return Http::FilterHeadersStatus::Continue; +Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) { + if (!forwarding_action_) { + forwarding_action_ = + std::make_unique(controller_->forwardingDecision()); } - if (controller_->tryForwardRequest()) { - rq_start_time_ = config_->timeSource().monotonicTime(); - return Http::FilterHeadersStatus::Continue; + if (*forwarding_action_ == ConcurrencyController::RequestForwardingAction::Block) { + // TODO (tonya11en): Remove filler words. + decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "filler words", nullptr, + absl::nullopt, "more filler words"); + return Http::FilterHeadersStatus::StopIteration; } - // TODO (tonya11en): Remove filler words. - decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "filler words", nullptr, - absl::nullopt, "more filler words"); - return Http::FilterHeadersStatus::StopIteration; -} - -void AdaptiveConcurrencyFilter::onDestroy() { - // TODO (tonya11en). + return Http::FilterHeadersStatus::Continue; } -Http::FilterHeadersStatus AdaptiveConcurrencyFilter::encodeHeaders(Http::HeaderMap&, - bool end_stream) { - if (end_stream) { - const std::chrono::nanoseconds rq_latency = - config_->timeSource().monotonicTime() - rq_start_time_; - controller_->recordLatencySample(rq_latency); +void AdaptiveConcurrencyFilter::decodeComplete() { + ASSERT(forwarding_action_ != nullptr); + if (*forwarding_action_ == ConcurrencyController::RequestForwardingAction::MustForward) { + rq_start_time_ = config_->timeSource().monotonicTime(); } +} - return Http::FilterHeadersStatus::Continue; +void AdaptiveConcurrencyFilter::encodeComplete() { + const std::chrono::nanoseconds rq_latency = + config_->timeSource().monotonicTime() - rq_start_time_; + controller_->recordLatencySample(rq_latency); } } // namespace AdaptiveConcurrency diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h index a3f6932e4fda..fca3bf787a63 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -9,6 +9,7 @@ #include "envoy/common/time.h" #include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" #include "envoy/http/filter.h" +#include "extensions/filters/http/common/pass_through_filter.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/scope.h" #include "envoy/stats/stats_macros.h" @@ -31,10 +32,7 @@ class AdaptiveConcurrencyFilterConfig { Runtime::Loader& runtime, std::string stats_prefix, Stats::Scope& scope, TimeSource& time_source); - Runtime::Loader& runtime() { return runtime_; } - const std::string& statsPrefix() const { return stats_prefix_; } - Stats::Scope& scope() { return scope_; } - TimeSource& timeSource() { return time_source_; } + TimeSource& timeSource() const { return time_source_; } private: Runtime::Loader& runtime_; @@ -43,7 +41,7 @@ class AdaptiveConcurrencyFilterConfig { TimeSource& time_source_; }; -using AdaptiveConcurrencyFilterConfigSharedPtr = std::shared_ptr; +using AdaptiveConcurrencyFilterConfigSharedPtr = std::shared_ptr; using ConcurrencyControllerSharedPtr = std::shared_ptr; @@ -51,51 +49,24 @@ using ConcurrencyControllerSharedPtr = * A filter that samples request latencies and dynamically adjusts the request * concurrency window. */ -class AdaptiveConcurrencyFilter : public Http::StreamFilter, Logger::Loggable { +class AdaptiveConcurrencyFilter : public Http::PassThroughFilter, Logger::Loggable { public: AdaptiveConcurrencyFilter(AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller); ~AdaptiveConcurrencyFilter() override; - // Http::StreamFilterBase - void onDestroy() override; - // Http::StreamDecoderFilter - Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool end_stream) override; - Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override { - return Http::FilterDataStatus::Continue; - } - Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) override { - return Http::FilterTrailersStatus::Continue; - } - void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override { - decoder_callbacks_ = &callbacks; - } + void decodeComplete() override; + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override; // Http::StreamEncoderFilter - Http::FilterHeadersStatus encode100ContinueHeaders(Http::HeaderMap&) override { - return Http::FilterHeadersStatus::Continue; - } - Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap&, bool) override; - Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override { - return Http::FilterDataStatus::Continue; - } - Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap&) override { - return Http::FilterTrailersStatus::Continue; - } - Http::FilterMetadataStatus encodeMetadata(Http::MetadataMap&) override { - return Http::FilterMetadataStatus::Continue; - } - void setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callbacks) override { - encoder_callbacks_ = &callbacks; - } + void encodeComplete() override; private: AdaptiveConcurrencyFilterConfigSharedPtr config_; - Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; - Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; - ConcurrencyControllerSharedPtr controller_; + const ConcurrencyControllerSharedPtr controller_; MonotonicTime rq_start_time_; + std::unique_ptr forwarding_action_; }; } // namespace AdaptiveConcurrency diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h index a41bfff65b51..9607b41d9f62 100644 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h @@ -10,6 +10,18 @@ namespace HttpFilters { namespace AdaptiveConcurrency { namespace ConcurrencyController { +/** + * The controller's decision on whether a request will be forwarded. + */ +enum RequestForwardingAction { + // The concurrency limit is exceeded, so the request cannot be forwarded. + Block, + + // The controller has allowed the request through and changed its internal + // state. The request must be forwarded. + MustForward +}; + /** * Adaptive concurrency controller interface. All implementations of this * interface must be thread-safe. @@ -20,10 +32,9 @@ class ConcurrencyController { /** * Called during decoding when the adaptive concurrency filter is attempting - * to forward a request. Returns true once the controller's internal state is - * updated and the request can be forwarded. + * to forward a request. Returns its decision on whether to forward a request. */ - virtual bool tryForwardRequest() PURE; + virtual RequestForwardingAction forwardingDecision() PURE; /** * Called during encoding when the request latency is known. Records the diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h index c304e3313458..1ffede76ea61 100644 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h @@ -16,7 +16,7 @@ namespace ConcurrencyController { class NoopController : public ConcurrencyController { public: // ConcurrencyController. - bool tryForwardRequest() override { return true; } + RequestAction forwardingDecision() override { return RequestForwardingAction::MustForward; } void recordLatencySample(const std::chrono::nanoseconds&) override {} }; diff --git a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc index a7ecd8e4f4b2..35b50fb7934d 100644 --- a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc +++ b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc @@ -19,9 +19,11 @@ namespace HttpFilters { namespace AdaptiveConcurrency { namespace { +using ConcurrencyController::RequestForwardingAction; + class MockConcurrencyController : public ConcurrencyController::ConcurrencyController { public: - MOCK_METHOD0(tryForwardRequest, bool()); + MOCK_METHOD0(forwardingDecision, RequestForwardingAction()); MOCK_METHOD1(recordLatencySample, void(const std::chrono::nanoseconds&)); }; @@ -48,66 +50,57 @@ void AdaptiveConcurrencyFilterTest::SetupTest() { filter_->setDecoderFilterCallbacks(decoder_callbacks_); } -// Verify the parts of the filter that aren't doing the work don't return -// anything unexpected. -TEST_F(AdaptiveConcurrencyFilterTest, UnusedFuncsTest) { +TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestForwarding) { SetupTest(); - Buffer::OwnedImpl request_body; - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(request_body, false)); - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(request_body, true)); - - Http::TestHeaderMapImpl request_trailers; - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers)); - - Http::TestHeaderMapImpl response_headers; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_->encode100ContinueHeaders(response_headers)); - - Buffer::OwnedImpl response_body; - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_body, false)); - - Http::TestHeaderMapImpl response_trailers; - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers)); + Http::TestHeaderMapImpl request_headers; - Http::MetadataMap metadata; - EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_->encodeMetadata(metadata)); + EXPECT_CALL(*controller_, forwardingDecision()).Times(1).WillRepeatedly(Return(RequestForwardingAction::MustForward)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); } -TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTest) { +TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestBlock) { SetupTest(); Http::TestHeaderMapImpl request_headers; - EXPECT_CALL(*controller_, tryForwardRequest()).Times(1).WillRepeatedly(Return(true)); - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); - - EXPECT_CALL(*controller_, tryForwardRequest()).Times(1).WillRepeatedly(Return(false)); - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); + EXPECT_CALL(*controller_, forwardingDecision()).Times(1).WillRepeatedly(Return(RequestForwardingAction::Block)); EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::ServiceUnavailable, _, _, _, _)) .Times(1); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, true)); } -TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersTest) { +TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersDeathTest) { + SetupTest(); + + Http::TestHeaderMapImpl response_headers; + + // The start time was never set. + EXPECT_DEATH({ filter_->decodeComplete(); }, ""); +} + +TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersValidTest) { SetupTest(); + auto mt = time_system_.monotonicTime(); + time_system_.setMonotonicTime(mt + std::chrono::milliseconds(123)); + // Get the filter to record the request start time via decode. Http::TestHeaderMapImpl request_headers; - EXPECT_CALL(*controller_, tryForwardRequest()).Times(1).WillRepeatedly(Return(true)); + EXPECT_CALL(*controller_, forwardingDecision()).Times(1).WillRepeatedly(Return(RequestForwardingAction::MustForward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); + filter_->decodeComplete(); const std::chrono::nanoseconds advance_time = std::chrono::milliseconds(42); - const auto mt = time_system_.monotonicTime(); + mt = time_system_.monotonicTime(); time_system_.setMonotonicTime(mt + advance_time); Http::TestHeaderMapImpl response_headers; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); - EXPECT_CALL(*controller_, recordLatencySample(advance_time)).Times(1); - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, true)); + filter_->encodeComplete(); } } // namespace From 2355305f8a1aa147498ed645632e93964dee9958 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Mon, 5 Aug 2019 16:07:43 -0700 Subject: [PATCH 10/17] fix format Signed-off-by: Tony Allen --- .../filters/http/adaptive_concurrency/BUILD | 2 +- .../adaptive_concurrency_filter.cc | 18 ++++++++++-------- .../adaptive_concurrency_filter.h | 8 +++++--- .../adaptive_concurrency_filter_test.cc | 12 +++++++++--- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/BUILD b/source/extensions/filters/http/adaptive_concurrency/BUILD index 30a8ffc147a6..914eeeaf84e7 100644 --- a/source/extensions/filters/http/adaptive_concurrency/BUILD +++ b/source/extensions/filters/http/adaptive_concurrency/BUILD @@ -19,8 +19,8 @@ envoy_cc_library( deps = [ "//include/envoy/http:filter_interface", "//source/extensions/filters/http:well_known_names", - "//source/extensions/filters/http/common:pass_through_filter_lib", "//source/extensions/filters/http/adaptive_concurrency/concurrency_controller:concurrency_controller_lib", + "//source/extensions/filters/http/common:pass_through_filter_lib", "@envoy_api//envoy/config/filter/http/adaptive_concurrency/v2alpha:adaptive_concurrency_cc", ], ) diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index c8dd8737a823..78eb6ed48710 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -1,11 +1,13 @@ +#include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" + #include #include #include #include -#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" -#include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" #include "common/common/assert.h" + +#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" #include "extensions/filters/http/well_known_names.h" namespace Envoy { @@ -35,15 +37,15 @@ AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() = default; Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) { if (!forwarding_action_) { - forwarding_action_ = - std::make_unique(controller_->forwardingDecision()); + forwarding_action_ = std::make_unique( + controller_->forwardingDecision()); } if (*forwarding_action_ == ConcurrencyController::RequestForwardingAction::Block) { - // TODO (tonya11en): Remove filler words. - decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "filler words", nullptr, - absl::nullopt, "more filler words"); - return Http::FilterHeadersStatus::StopIteration; + // TODO (tonya11en): Remove filler words. + decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "filler words", nullptr, + absl::nullopt, "more filler words"); + return Http::FilterHeadersStatus::StopIteration; } return Http::FilterHeadersStatus::Continue; diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h index fca3bf787a63..b7f07dbf9140 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -9,12 +9,12 @@ #include "envoy/common/time.h" #include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" #include "envoy/http/filter.h" -#include "extensions/filters/http/common/pass_through_filter.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/scope.h" #include "envoy/stats/stats_macros.h" #include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" +#include "extensions/filters/http/common/pass_through_filter.h" namespace Envoy { namespace Extensions { @@ -41,7 +41,8 @@ class AdaptiveConcurrencyFilterConfig { TimeSource& time_source_; }; -using AdaptiveConcurrencyFilterConfigSharedPtr = std::shared_ptr; +using AdaptiveConcurrencyFilterConfigSharedPtr = + std::shared_ptr; using ConcurrencyControllerSharedPtr = std::shared_ptr; @@ -49,7 +50,8 @@ using ConcurrencyControllerSharedPtr = * A filter that samples request latencies and dynamically adjusts the request * concurrency window. */ -class AdaptiveConcurrencyFilter : public Http::PassThroughFilter, Logger::Loggable { +class AdaptiveConcurrencyFilter : public Http::PassThroughFilter, + Logger::Loggable { public: AdaptiveConcurrencyFilter(AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller); diff --git a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc index 35b50fb7934d..7e3d48efd310 100644 --- a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc +++ b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc @@ -55,7 +55,9 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestForwarding) { Http::TestHeaderMapImpl request_headers; - EXPECT_CALL(*controller_, forwardingDecision()).Times(1).WillRepeatedly(Return(RequestForwardingAction::MustForward)); + EXPECT_CALL(*controller_, forwardingDecision()) + .Times(1) + .WillRepeatedly(Return(RequestForwardingAction::MustForward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); } @@ -65,7 +67,9 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestBlock) { Http::TestHeaderMapImpl request_headers; - EXPECT_CALL(*controller_, forwardingDecision()).Times(1).WillRepeatedly(Return(RequestForwardingAction::Block)); + EXPECT_CALL(*controller_, forwardingDecision()) + .Times(1) + .WillRepeatedly(Return(RequestForwardingAction::Block)); EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::ServiceUnavailable, _, _, _, _)) .Times(1); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, @@ -89,7 +93,9 @@ TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersValidTest) { // Get the filter to record the request start time via decode. Http::TestHeaderMapImpl request_headers; - EXPECT_CALL(*controller_, forwardingDecision()).Times(1).WillRepeatedly(Return(RequestForwardingAction::MustForward)); + EXPECT_CALL(*controller_, forwardingDecision()) + .Times(1) + .WillRepeatedly(Return(RequestForwardingAction::MustForward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); filter_->decodeComplete(); From 879a5d4c394d5136376df861d58f509ff4877965 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Mon, 5 Aug 2019 16:20:52 -0700 Subject: [PATCH 11/17] format Signed-off-by: Tony Allen --- .../concurrency_controller/noop_controller.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h index 1ffede76ea61..025f9abca7a5 100644 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h @@ -16,7 +16,9 @@ namespace ConcurrencyController { class NoopController : public ConcurrencyController { public: // ConcurrencyController. - RequestAction forwardingDecision() override { return RequestForwardingAction::MustForward; } + RequestForwardingAction forwardingDecision() override { + return RequestForwardingAction::MustForward; + } void recordLatencySample(const std::chrono::nanoseconds&) override {} }; From ef09c5d09a7d0338539207df631e3c6b8b054913 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Tue, 6 Aug 2019 13:15:42 -0700 Subject: [PATCH 12/17] comments round 2 Signed-off-by: Tony Allen --- .../adaptive_concurrency_filter.cc | 17 ++++------------- .../adaptive_concurrency_filter.h | 1 - .../concurrency_controller.h | 2 +- .../extensions/filters/http/well_known_names.h | 2 +- .../adaptive_concurrency_filter_test.cc | 17 ++++++----------- 5 files changed, 12 insertions(+), 27 deletions(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index 78eb6ed48710..c8f8148caafb 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -24,7 +24,7 @@ AdaptiveConcurrencyFilterConfig::AdaptiveConcurrencyFilterConfig( // TODO (tonya11en): Remove these noop calls when stats/runtime values are // implemented. // - // Calling for test coverage. + // Calling so the builds pass with these currently unused variables. runtime_.snapshot(); scope_.constSymbolTable(); } @@ -36,28 +36,19 @@ AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() = default; Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) { - if (!forwarding_action_) { - forwarding_action_ = std::make_unique( - controller_->forwardingDecision()); - } + const auto forwarding_action = controller_->forwardingDecision(); - if (*forwarding_action_ == ConcurrencyController::RequestForwardingAction::Block) { + if (forwarding_action == ConcurrencyController::RequestForwardingAction::Block) { // TODO (tonya11en): Remove filler words. decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "filler words", nullptr, absl::nullopt, "more filler words"); return Http::FilterHeadersStatus::StopIteration; } + rq_start_time_ = config_->timeSource().monotonicTime(); return Http::FilterHeadersStatus::Continue; } -void AdaptiveConcurrencyFilter::decodeComplete() { - ASSERT(forwarding_action_ != nullptr); - if (*forwarding_action_ == ConcurrencyController::RequestForwardingAction::MustForward) { - rq_start_time_ = config_->timeSource().monotonicTime(); - } -} - void AdaptiveConcurrencyFilter::encodeComplete() { const std::chrono::nanoseconds rq_latency = config_->timeSource().monotonicTime() - rq_start_time_; diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h index b7f07dbf9140..031aa5eba964 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -58,7 +58,6 @@ class AdaptiveConcurrencyFilter : public Http::PassThroughFilter, ~AdaptiveConcurrencyFilter() override; // Http::StreamDecoderFilter - void decodeComplete() override; Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override; // Http::StreamEncoderFilter diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h index 9607b41d9f62..f1702114f3b5 100644 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h @@ -13,7 +13,7 @@ namespace ConcurrencyController { /** * The controller's decision on whether a request will be forwarded. */ -enum RequestForwardingAction { +enum class RequestForwardingAction { // The concurrency limit is exceeded, so the request cannot be forwarded. Block, diff --git a/source/extensions/filters/http/well_known_names.h b/source/extensions/filters/http/well_known_names.h index 4f5c45a83026..82341e13739f 100644 --- a/source/extensions/filters/http/well_known_names.h +++ b/source/extensions/filters/http/well_known_names.h @@ -54,7 +54,7 @@ class HttpFilterNameValues { const std::string HeaderToMetadata = "envoy.filters.http.header_to_metadata"; // Tap filter const std::string Tap = "envoy.filters.http.tap"; - // Adaptive concurrency limit filter. + // Adaptive concurrency limit filter const std::string AdaptiveConcurrency = "envoy.filters.http.adaptive_concurrency"; // Original Src Filter const std::string OriginalSrc = "envoy.filters.http.original_src"; diff --git a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc index 7e3d48efd310..2a2e5b573852 100644 --- a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc +++ b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc @@ -59,7 +59,12 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestForwarding) { .Times(1) .WillRepeatedly(Return(RequestForwardingAction::MustForward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); + + Buffer::OwnedImpl request_body; + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(request_body, false)); + + Http::TestHeaderMapImpl request_trailers; + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers)); } TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestBlock) { @@ -76,15 +81,6 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestBlock) { filter_->decodeHeaders(request_headers, true)); } -TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersDeathTest) { - SetupTest(); - - Http::TestHeaderMapImpl response_headers; - - // The start time was never set. - EXPECT_DEATH({ filter_->decodeComplete(); }, ""); -} - TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersValidTest) { SetupTest(); @@ -97,7 +93,6 @@ TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersValidTest) { .Times(1) .WillRepeatedly(Return(RequestForwardingAction::MustForward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); - filter_->decodeComplete(); const std::chrono::nanoseconds advance_time = std::chrono::milliseconds(42); mt = time_system_.monotonicTime(); From c91c58b84e4c463bb985713f0aff85bccee97ac3 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Tue, 6 Aug 2019 17:46:03 -0700 Subject: [PATCH 13/17] rip out noop controller Signed-off-by: Tony Allen --- .../adaptive_concurrency_filter.cc | 20 +++--------- .../adaptive_concurrency_filter.h | 4 --- .../concurrency_controller/BUILD | 1 - .../concurrency_controller.h | 2 +- .../concurrency_controller/noop_controller.h | 29 ----------------- .../http/adaptive_concurrency/config.cc | 17 ++-------- .../adaptive_concurrency_filter_test.cc | 32 +++++++------------ 7 files changed, 20 insertions(+), 85 deletions(-) delete mode 100644 source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index c8f8148caafb..29327d8539dc 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -17,17 +17,8 @@ namespace AdaptiveConcurrency { AdaptiveConcurrencyFilterConfig::AdaptiveConcurrencyFilterConfig( const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency&, - Runtime::Loader& runtime, std::string stats_prefix, Stats::Scope& scope, - TimeSource& time_source) - : runtime_(runtime), stats_prefix_(std::move(stats_prefix)), scope_(scope), - time_source_(time_source) { - // TODO (tonya11en): Remove these noop calls when stats/runtime values are - // implemented. - // - // Calling so the builds pass with these currently unused variables. - runtime_.snapshot(); - scope_.constSymbolTable(); -} + Runtime::Loader&, std::string stats_prefix, Stats::Scope&, TimeSource& time_source) + : stats_prefix_(std::move(stats_prefix)), time_source_(time_source) {} AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller) @@ -36,9 +27,7 @@ AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() = default; Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) { - const auto forwarding_action = controller_->forwardingDecision(); - - if (forwarding_action == ConcurrencyController::RequestForwardingAction::Block) { + if (controller_->forwardingDecision() == ConcurrencyController::RequestForwardingAction::Block) { // TODO (tonya11en): Remove filler words. decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "filler words", nullptr, absl::nullopt, "more filler words"); @@ -50,8 +39,7 @@ Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderM } void AdaptiveConcurrencyFilter::encodeComplete() { - const std::chrono::nanoseconds rq_latency = - config_->timeSource().monotonicTime() - rq_start_time_; + const auto rq_latency = config_->timeSource().monotonicTime() - rq_start_time_; controller_->recordLatencySample(rq_latency); } diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h index 031aa5eba964..0ea7bd6790ac 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -3,8 +3,6 @@ #include #include #include -#include -#include #include "envoy/common/time.h" #include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" @@ -35,9 +33,7 @@ class AdaptiveConcurrencyFilterConfig { TimeSource& timeSource() const { return time_source_; } private: - Runtime::Loader& runtime_; const std::string stats_prefix_; - Stats::Scope& scope_; TimeSource& time_source_; }; diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/BUILD b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/BUILD index 75763e2bd8f4..d213690d63c6 100644 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/BUILD +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/BUILD @@ -17,7 +17,6 @@ envoy_cc_library( srcs = [], hdrs = [ "concurrency_controller.h", - "noop_controller.h", ], deps = [ ], diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h index f1702114f3b5..0c0dbe456c7d 100644 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h +++ b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h @@ -19,7 +19,7 @@ enum class RequestForwardingAction { // The controller has allowed the request through and changed its internal // state. The request must be forwarded. - MustForward + Forward }; /** diff --git a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h b/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h deleted file mode 100644 index 025f9abca7a5..000000000000 --- a/source/extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h +++ /dev/null @@ -1,29 +0,0 @@ -#pragma once - -#include - -#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/concurrency_controller.h" - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace AdaptiveConcurrency { -namespace ConcurrencyController { - -/** - * Adaptive concurrency controller that does nothing. - */ -class NoopController : public ConcurrencyController { -public: - // ConcurrencyController. - RequestForwardingAction forwardingDecision() override { - return RequestForwardingAction::MustForward; - } - void recordLatencySample(const std::chrono::nanoseconds&) override {} -}; - -} // namespace ConcurrencyController -} // namespace AdaptiveConcurrency -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/http/adaptive_concurrency/config.cc b/source/extensions/filters/http/adaptive_concurrency/config.cc index 95929df82ad2..e087ea0f8f07 100644 --- a/source/extensions/filters/http/adaptive_concurrency/config.cc +++ b/source/extensions/filters/http/adaptive_concurrency/config.cc @@ -6,7 +6,6 @@ #include "common/config/filter_json.h" #include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" -#include "extensions/filters/http/adaptive_concurrency/concurrency_controller/noop_controller.h" namespace Envoy { namespace Extensions { @@ -14,20 +13,10 @@ namespace HttpFilters { namespace AdaptiveConcurrency { Http::FilterFactoryCb AdaptiveConcurrencyFilterFactory::createFilterFactoryFromProtoTyped( - const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency& config, - const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { + const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency&, + const std::string&, Server::Configuration::FactoryContext&) { - // TODO (tonya11en): Noop controller needs to be replaced with an actual - // implementation in a future patch. - std::shared_ptr noop_ctl = - std::make_shared(); - - AdaptiveConcurrencyFilterConfigSharedPtr filter_config(new AdaptiveConcurrencyFilterConfig( - config, context.runtime(), stats_prefix, context.scope(), context.timeSource())); - - return [filter_config, noop_ctl](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamFilter(std::make_shared(filter_config, noop_ctl)); - }; + return [](Http::FilterChainFactoryCallbacks&) -> void {}; } /** diff --git a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc index 2a2e5b573852..ee1284b7bb11 100644 --- a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc +++ b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc @@ -29,7 +29,16 @@ class MockConcurrencyController : public ConcurrencyController::ConcurrencyContr class AdaptiveConcurrencyFilterTest : public testing::Test { public: - void SetupTest(); + AdaptiveConcurrencyFilterTest() { + filter_.reset(); + + const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency config; + auto config_ptr = std::make_shared( + config, runtime_, "testprefix.", stats_, time_system_); + + filter_ = std::make_unique(config_ptr, controller_); + filter_->setDecoderFilterCallbacks(decoder_callbacks_); + } std::unique_ptr filter_; Event::SimulatedTimeSystem time_system_; @@ -39,25 +48,12 @@ class AdaptiveConcurrencyFilterTest : public testing::Test { NiceMock decoder_callbacks_; }; -void AdaptiveConcurrencyFilterTest::SetupTest() { - filter_.reset(); - - const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency config; - auto config_ptr = std::make_shared( - config, runtime_, "testprefix.", stats_, time_system_); - - filter_ = std::make_unique(config_ptr, controller_); - filter_->setDecoderFilterCallbacks(decoder_callbacks_); -} - TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestForwarding) { - SetupTest(); - Http::TestHeaderMapImpl request_headers; EXPECT_CALL(*controller_, forwardingDecision()) .Times(1) - .WillRepeatedly(Return(RequestForwardingAction::MustForward)); + .WillRepeatedly(Return(RequestForwardingAction::Forward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); Buffer::OwnedImpl request_body; @@ -68,8 +64,6 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestForwarding) { } TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestBlock) { - SetupTest(); - Http::TestHeaderMapImpl request_headers; EXPECT_CALL(*controller_, forwardingDecision()) @@ -82,8 +76,6 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestBlock) { } TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersValidTest) { - SetupTest(); - auto mt = time_system_.monotonicTime(); time_system_.setMonotonicTime(mt + std::chrono::milliseconds(123)); @@ -91,7 +83,7 @@ TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersValidTest) { Http::TestHeaderMapImpl request_headers; EXPECT_CALL(*controller_, forwardingDecision()) .Times(1) - .WillRepeatedly(Return(RequestForwardingAction::MustForward)); + .WillRepeatedly(Return(RequestForwardingAction::Forward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); const std::chrono::nanoseconds advance_time = std::chrono::milliseconds(42); From d3d20221c7918d387468a7aca5bb9a4ae1861c1c Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Wed, 7 Aug 2019 11:28:26 -0700 Subject: [PATCH 14/17] remove config files Signed-off-by: Tony Allen --- .../filters/http/adaptive_concurrency/BUILD | 13 -------- .../http/adaptive_concurrency/config.cc | 31 ------------------ .../http/adaptive_concurrency/config.h | 32 ------------------- 3 files changed, 76 deletions(-) delete mode 100644 source/extensions/filters/http/adaptive_concurrency/config.cc delete mode 100644 source/extensions/filters/http/adaptive_concurrency/config.h diff --git a/source/extensions/filters/http/adaptive_concurrency/BUILD b/source/extensions/filters/http/adaptive_concurrency/BUILD index 914eeeaf84e7..5d86773ebf8c 100644 --- a/source/extensions/filters/http/adaptive_concurrency/BUILD +++ b/source/extensions/filters/http/adaptive_concurrency/BUILD @@ -24,16 +24,3 @@ envoy_cc_library( "@envoy_api//envoy/config/filter/http/adaptive_concurrency/v2alpha:adaptive_concurrency_cc", ], ) - -envoy_cc_library( - name = "config", - srcs = ["config.cc"], - hdrs = ["config.h"], - deps = [ - "//include/envoy/registry", - "//source/common/config:filter_json_lib", - "//source/extensions/filters/http:well_known_names", - "//source/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_filter_lib", - "//source/extensions/filters/http/common:factory_base_lib", - ], -) diff --git a/source/extensions/filters/http/adaptive_concurrency/config.cc b/source/extensions/filters/http/adaptive_concurrency/config.cc deleted file mode 100644 index e087ea0f8f07..000000000000 --- a/source/extensions/filters/http/adaptive_concurrency/config.cc +++ /dev/null @@ -1,31 +0,0 @@ -#include "extensions/filters/http/adaptive_concurrency/config.h" - -#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.validate.h" -#include "envoy/registry/registry.h" - -#include "common/config/filter_json.h" - -#include "extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h" - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace AdaptiveConcurrency { - -Http::FilterFactoryCb AdaptiveConcurrencyFilterFactory::createFilterFactoryFromProtoTyped( - const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency&, - const std::string&, Server::Configuration::FactoryContext&) { - - return [](Http::FilterChainFactoryCallbacks&) -> void {}; -} - -/** - * Static registration for the adaptive_concurrency filter. @see RegisterFactory. - */ -REGISTER_FACTORY(AdaptiveConcurrencyFilterFactory, - Server::Configuration::NamedHttpFilterConfigFactory); - -} // namespace AdaptiveConcurrency -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/http/adaptive_concurrency/config.h b/source/extensions/filters/http/adaptive_concurrency/config.h deleted file mode 100644 index bce0e0fc6c71..000000000000 --- a/source/extensions/filters/http/adaptive_concurrency/config.h +++ /dev/null @@ -1,32 +0,0 @@ -#pragma once - -#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.h" -#include "envoy/config/filter/http/adaptive_concurrency/v2alpha/adaptive_concurrency.pb.validate.h" - -#include "extensions/filters/http/common/factory_base.h" -#include "extensions/filters/http/well_known_names.h" - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace AdaptiveConcurrency { - -/** - * Config registration for the adaptive concurrency limit filter. @see NamedHttpFilterConfigFactory. - */ -class AdaptiveConcurrencyFilterFactory - : public Common::FactoryBase< - envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency> { -public: - AdaptiveConcurrencyFilterFactory() : FactoryBase(HttpFilterNames::get().AdaptiveConcurrency) {} - - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( - const envoy::config::filter::http::adaptive_concurrency::v2alpha::AdaptiveConcurrency& - proto_config, - const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; -}; - -} // namespace AdaptiveConcurrency -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy From 2cfbfefbced5d5d6220e5eb7c06fcf7b63b685ae Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Wed, 7 Aug 2019 11:51:14 -0700 Subject: [PATCH 15/17] filter lib Signed-off-by: Tony Allen --- source/extensions/extensions_build_config.bzl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index 3fcd0d2bf863..b96c047dab66 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -29,7 +29,9 @@ EXTENSIONS = { # HTTP filters # - "envoy.filters.http.adaptive_concurrency": "//source/extensions/filters/http/adaptive_concurrency:config", + # NOTE: The adaptive concurrency filter does not have a proper filter + # implemented right now. We are just referencing the filter lib here. + "envoy.filters.http.adaptive_concurrency": "//source/extensions/filters/http/adaptive_concurrency:adaptive_concurrency_filter_lib", "envoy.filters.http.buffer": "//source/extensions/filters/http/buffer:config", "envoy.filters.http.cors": "//source/extensions/filters/http/cors:config", "envoy.filters.http.csrf": "//source/extensions/filters/http/csrf:config", From ab794b2bbfa924fb8cb1575251d091140fb7f7a3 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Wed, 7 Aug 2019 15:28:17 -0700 Subject: [PATCH 16/17] final nits Signed-off-by: Tony Allen --- .../adaptive_concurrency_filter.cc | 2 -- .../adaptive_concurrency_filter.h | 1 - .../adaptive_concurrency_filter_test.cc | 14 +++++--------- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc index 29327d8539dc..1ec4dd8247e2 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.cc @@ -24,8 +24,6 @@ AdaptiveConcurrencyFilter::AdaptiveConcurrencyFilter( AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller) : config_(std::move(config)), controller_(std::move(controller)) {} -AdaptiveConcurrencyFilter::~AdaptiveConcurrencyFilter() = default; - Http::FilterHeadersStatus AdaptiveConcurrencyFilter::decodeHeaders(Http::HeaderMap&, bool) { if (controller_->forwardingDecision() == ConcurrencyController::RequestForwardingAction::Block) { // TODO (tonya11en): Remove filler words. diff --git a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h index 0ea7bd6790ac..88070180272b 100644 --- a/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h +++ b/source/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter.h @@ -51,7 +51,6 @@ class AdaptiveConcurrencyFilter : public Http::PassThroughFilter, public: AdaptiveConcurrencyFilter(AdaptiveConcurrencyFilterConfigSharedPtr config, ConcurrencyControllerSharedPtr controller); - ~AdaptiveConcurrencyFilter() override; // Http::StreamDecoderFilter Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override; diff --git a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc index ee1284b7bb11..b3553a1482cb 100644 --- a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc +++ b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc @@ -52,8 +52,7 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestForwarding) { Http::TestHeaderMapImpl request_headers; EXPECT_CALL(*controller_, forwardingDecision()) - .Times(1) - .WillRepeatedly(Return(RequestForwardingAction::Forward)); + .WillOnce(Return(RequestForwardingAction::Forward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); Buffer::OwnedImpl request_body; @@ -67,10 +66,8 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestBlock) { Http::TestHeaderMapImpl request_headers; EXPECT_CALL(*controller_, forwardingDecision()) - .Times(1) - .WillRepeatedly(Return(RequestForwardingAction::Block)); - EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::ServiceUnavailable, _, _, _, _)) - .Times(1); + .WillOnce(Return(RequestForwardingAction::Block)); + EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::ServiceUnavailable, _, _, _, _)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, true)); } @@ -82,8 +79,7 @@ TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersValidTest) { // Get the filter to record the request start time via decode. Http::TestHeaderMapImpl request_headers; EXPECT_CALL(*controller_, forwardingDecision()) - .Times(1) - .WillRepeatedly(Return(RequestForwardingAction::Forward)); + .WillOnce(Return(RequestForwardingAction::Forward)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, true)); const std::chrono::nanoseconds advance_time = std::chrono::milliseconds(42); @@ -92,7 +88,7 @@ TEST_F(AdaptiveConcurrencyFilterTest, EncodeHeadersValidTest) { Http::TestHeaderMapImpl response_headers; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false)); - EXPECT_CALL(*controller_, recordLatencySample(advance_time)).Times(1); + EXPECT_CALL(*controller_, recordLatencySample(advance_time)); filter_->encodeComplete(); } From 513ee6cb16cad3c42dab24f05f12cac431c95141 Mon Sep 17 00:00:00 2001 From: Tony Allen Date: Wed, 7 Aug 2019 15:28:59 -0700 Subject: [PATCH 17/17] final nits Signed-off-by: Tony Allen --- .../adaptive_concurrency/adaptive_concurrency_filter_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc index b3553a1482cb..60ad871dc3e5 100644 --- a/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc +++ b/test/extensions/filters/http/adaptive_concurrency/adaptive_concurrency_filter_test.cc @@ -65,8 +65,7 @@ TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestForwarding) { TEST_F(AdaptiveConcurrencyFilterTest, DecodeHeadersTestBlock) { Http::TestHeaderMapImpl request_headers; - EXPECT_CALL(*controller_, forwardingDecision()) - .WillOnce(Return(RequestForwardingAction::Block)); + EXPECT_CALL(*controller_, forwardingDecision()).WillOnce(Return(RequestForwardingAction::Block)); EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::ServiceUnavailable, _, _, _, _)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers, true));