From 85c153eb15b7a174c72ab7faefc582de32a82664 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 30 Jan 2018 14:14:57 -0800 Subject: [PATCH 1/3] Revert "Fix time interval setup. (#954)" This reverts commit 8d2fc52965b8f6fb515a6d909441277ea36a2b08. --- src/envoy/mixer/stats.cc | 12 ++++++------ src/envoy/mixer/stats.h | 5 ----- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/envoy/mixer/stats.cc b/src/envoy/mixer/stats.cc index 5a24bce5467..3c738b0c41b 100644 --- a/src/envoy/mixer/stats.cc +++ b/src/envoy/mixer/stats.cc @@ -31,15 +31,15 @@ MixerStatsObject::MixerStatsObject(Event::Dispatcher& dispatcher, const std::string& name, Stats::Scope& scope, int update_interval_ms, GetStatsFunc func) : stats_{ALL_MIXER_FILTER_STATS(POOL_COUNTER_PREFIX(scope, name))}, - get_stats_func_(func), - stats_update_interval_(update_interval_ms > 0 - ? update_interval_ms - : kStatsUpdateIntervalInMs) { + get_stats_func_(func) { memset(&old_stats_, 0, sizeof(old_stats_)); + // If stats update interval from config is 0, then set interval to 10 seconds. + int stats_update_interval = + update_interval_ms > 0 ? update_interval_ms : kStatsUpdateIntervalInMs; if (get_stats_func_) { timer_ = dispatcher.createTimer([this]() { OnTimer(); }); - timer_->enableTimer(std::chrono::milliseconds(stats_update_interval_)); + timer_->enableTimer(std::chrono::milliseconds(stats_update_interval)); } } @@ -49,7 +49,7 @@ void MixerStatsObject::OnTimer() { if (get_stats) { CheckAndUpdateStats(new_stats); } - timer_->enableTimer(std::chrono::milliseconds(stats_update_interval_)); + timer_->enableTimer(std::chrono::milliseconds(kStatsUpdateIntervalInMs)); } void MixerStatsObject::CheckAndUpdateStats( diff --git a/src/envoy/mixer/stats.h b/src/envoy/mixer/stats.h index 2a40e74deeb..19cfda1b70b 100644 --- a/src/envoy/mixer/stats.h +++ b/src/envoy/mixer/stats.h @@ -75,11 +75,6 @@ class MixerStatsObject { // These members are used for creating a timer which update Envoy stats // periodically. ::Envoy::Event::TimerPtr timer_; - - // Time interval at which Envoy stats get updated. If stats update interval - // from config is larger than 0, then store configured interval here. - // Otherwise, set interval to 10 seconds. - const int stats_update_interval_; }; } // namespace Mixer From 084e9d3bb653e7e3d2d37a839a505732c91c9830 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 30 Jan 2018 14:32:45 -0800 Subject: [PATCH 2/3] Revert "Pass stats update interval (#945)" This reverts commit d125d04cfe3990cab9fd097e5fd702668dde7ca0. --- src/envoy/mixer/mixer_control.cc | 19 ++++++++----------- src/envoy/mixer/stats.cc | 7 ++----- src/envoy/mixer/stats.h | 3 +-- 3 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/envoy/mixer/mixer_control.cc b/src/envoy/mixer/mixer_control.cc index c64794a689f..5b057310301 100644 --- a/src/envoy/mixer/mixer_control.cc +++ b/src/envoy/mixer/mixer_control.cc @@ -67,16 +67,14 @@ HttpMixerControl::HttpMixerControl(const HttpMixerConfig& mixer_config, Runtime::RandomGenerator& random, Stats::Scope& scope) : cm_(cm), - stats_obj_( - dispatcher, kHttpStatsPrefix, scope, - mixer_config.http_config.transport().stats_update_interval_ms(), - [this](Statistics* stat) -> bool { - if (!controller_) { - return false; - } - controller_->GetStatistics(stat); - return true; - }) { + stats_obj_(dispatcher, kHttpStatsPrefix, scope, + [this](Statistics* stat) -> bool { + if (!controller_) { + return false; + } + controller_->GetStatistics(stat); + return true; + }) { ::istio::mixer_control::http::Controller::Options options( mixer_config.http_config, mixer_config.legacy_quotas); @@ -93,7 +91,6 @@ TcpMixerControl::TcpMixerControl(const TcpMixerConfig& mixer_config, Runtime::RandomGenerator& random, Stats::Scope& scope) : stats_obj_(dispatcher, kTcpStatsPrefix, scope, - mixer_config.tcp_config.transport().stats_update_interval_ms(), [this](Statistics* stat) -> bool { if (!controller_) { return false; diff --git a/src/envoy/mixer/stats.cc b/src/envoy/mixer/stats.cc index 3c738b0c41b..6796aa851a6 100644 --- a/src/envoy/mixer/stats.cc +++ b/src/envoy/mixer/stats.cc @@ -29,17 +29,14 @@ const int kStatsUpdateIntervalInMs = 10000; MixerStatsObject::MixerStatsObject(Event::Dispatcher& dispatcher, const std::string& name, Stats::Scope& scope, - int update_interval_ms, GetStatsFunc func) + GetStatsFunc func) : stats_{ALL_MIXER_FILTER_STATS(POOL_COUNTER_PREFIX(scope, name))}, get_stats_func_(func) { memset(&old_stats_, 0, sizeof(old_stats_)); - // If stats update interval from config is 0, then set interval to 10 seconds. - int stats_update_interval = - update_interval_ms > 0 ? update_interval_ms : kStatsUpdateIntervalInMs; if (get_stats_func_) { timer_ = dispatcher.createTimer([this]() { OnTimer(); }); - timer_->enableTimer(std::chrono::milliseconds(stats_update_interval)); + timer_->enableTimer(std::chrono::milliseconds(kStatsUpdateIntervalInMs)); } } diff --git a/src/envoy/mixer/stats.h b/src/envoy/mixer/stats.h index 19cfda1b70b..5c2a05e8770 100644 --- a/src/envoy/mixer/stats.h +++ b/src/envoy/mixer/stats.h @@ -53,8 +53,7 @@ typedef std::function GetStatsFunc; class MixerStatsObject { public: MixerStatsObject(Event::Dispatcher& dispatcher, const std::string& name, - Stats::Scope& scope, int update_interval_ms, - GetStatsFunc func); + Stats::Scope& scope, GetStatsFunc func); private: // This function is invoked when timer event fires. From 16f80e27be20adc2bd7703b5ba0c869b146d03b6 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 30 Jan 2018 14:34:45 -0800 Subject: [PATCH 3/3] Revert "Support envoy to provide stats generated by mixer filter. (#891)" This reverts commit 147edd139e86567dd8036c48740e24aea60e106d. --- src/envoy/mixer/BUILD | 2 - src/envoy/mixer/http_filter.cc | 13 ++-- src/envoy/mixer/mixer_control.cc | 29 +-------- src/envoy/mixer/mixer_control.h | 9 +-- src/envoy/mixer/stats.cc | 100 ------------------------------- src/envoy/mixer/stats.h | 81 ------------------------- src/envoy/mixer/tcp_filter.cc | 13 ++-- 7 files changed, 17 insertions(+), 230 deletions(-) delete mode 100644 src/envoy/mixer/stats.cc delete mode 100644 src/envoy/mixer/stats.h diff --git a/src/envoy/mixer/BUILD b/src/envoy/mixer/BUILD index 8b3e0a086d0..055fd301661 100644 --- a/src/envoy/mixer/BUILD +++ b/src/envoy/mixer/BUILD @@ -30,8 +30,6 @@ envoy_cc_library( "http_filter.cc", "mixer_control.cc", "mixer_control.h", - "stats.cc", - "stats.h", "tcp_filter.cc", "utils.cc", "utils.h", diff --git a/src/envoy/mixer/http_filter.cc b/src/envoy/mixer/http_filter.cc index c408ceeba2c..3f37c5b9cbe 100644 --- a/src/envoy/mixer/http_filter.cc +++ b/src/envoy/mixer/http_filter.cc @@ -115,13 +115,12 @@ class Config { tls_(context.threadLocal().allocateSlot()) { mixer_config_.Load(config); Runtime::RandomGenerator& random = context.random(); - Stats::Scope& scope = context.scope(); - tls_->set([this, &random, &scope](Event::Dispatcher& dispatcher) - -> ThreadLocal::ThreadLocalObjectSharedPtr { - return ThreadLocal::ThreadLocalObjectSharedPtr( - new HttpMixerControl(mixer_config_, cm_, dispatcher, - random, scope)); - }); + tls_->set( + [this, &random](Event::Dispatcher& dispatcher) + -> ThreadLocal::ThreadLocalObjectSharedPtr { + return ThreadLocal::ThreadLocalObjectSharedPtr( + new HttpMixerControl(mixer_config_, cm_, dispatcher, random)); + }); std::vector> issuers; CreateAuthIssuers(mixer_config_, &issuers); diff --git a/src/envoy/mixer/mixer_control.cc b/src/envoy/mixer/mixer_control.cc index 5b057310301..0bfca19e403 100644 --- a/src/envoy/mixer/mixer_control.cc +++ b/src/envoy/mixer/mixer_control.cc @@ -16,16 +16,11 @@ #include "src/envoy/mixer/mixer_control.h" #include "src/envoy/mixer/grpc_transport.h" -using ::istio::mixer_client::Statistics; - namespace Envoy { namespace Http { namespace Mixer { namespace { -const std::string kHttpStatsPrefix("http_mixer_filter."); -const std::string kTcpStatsPrefix("tcp_mixer_filter."); - // A class to wrap envoy timer for mixer client timer. class EnvoyTimer : public ::istio::mixer_client::Timer { public: @@ -64,17 +59,8 @@ void CreateEnvironment(Upstream::ClusterManager& cm, HttpMixerControl::HttpMixerControl(const HttpMixerConfig& mixer_config, Upstream::ClusterManager& cm, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, - Stats::Scope& scope) - : cm_(cm), - stats_obj_(dispatcher, kHttpStatsPrefix, scope, - [this](Statistics* stat) -> bool { - if (!controller_) { - return false; - } - controller_->GetStatistics(stat); - return true; - }) { + Runtime::RandomGenerator& random) + : cm_(cm) { ::istio::mixer_control::http::Controller::Options options( mixer_config.http_config, mixer_config.legacy_quotas); @@ -88,16 +74,7 @@ HttpMixerControl::HttpMixerControl(const HttpMixerConfig& mixer_config, TcpMixerControl::TcpMixerControl(const TcpMixerConfig& mixer_config, Upstream::ClusterManager& cm, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, - Stats::Scope& scope) - : stats_obj_(dispatcher, kTcpStatsPrefix, scope, - [this](Statistics* stat) -> bool { - if (!controller_) { - return false; - } - controller_->GetStatistics(stat); - return true; - }) { + Runtime::RandomGenerator& random) { ::istio::mixer_control::tcp::Controller::Options options( mixer_config.tcp_config); diff --git a/src/envoy/mixer/mixer_control.h b/src/envoy/mixer/mixer_control.h index 7f072e46217..8fdd63a7e01 100644 --- a/src/envoy/mixer/mixer_control.h +++ b/src/envoy/mixer/mixer_control.h @@ -22,7 +22,6 @@ #include "envoy/thread_local/thread_local.h" #include "envoy/upstream/cluster_manager.h" #include "src/envoy/mixer/config.h" -#include "src/envoy/mixer/stats.h" namespace Envoy { namespace Http { @@ -33,7 +32,7 @@ class HttpMixerControl final : public ThreadLocal::ThreadLocalObject { // The constructor. HttpMixerControl(const HttpMixerConfig& mixer_config, Upstream::ClusterManager& cm, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, Stats::Scope& scope); + Runtime::RandomGenerator& random); Upstream::ClusterManager& cm() { return cm_; } @@ -50,8 +49,6 @@ class HttpMixerControl final : public ThreadLocal::ThreadLocalObject { std::unique_ptr<::istio::mixer_control::http::Controller> controller_; // has v2 config; bool has_v2_config_; - - MixerStatsObject stats_obj_; }; class TcpMixerControl final : public ThreadLocal::ThreadLocalObject { @@ -59,7 +56,7 @@ class TcpMixerControl final : public ThreadLocal::ThreadLocalObject { // The constructor. TcpMixerControl(const TcpMixerConfig& mixer_config, Upstream::ClusterManager& cm, Event::Dispatcher& dispatcher, - Runtime::RandomGenerator& random, Stats::Scope& scope); + Runtime::RandomGenerator& random); ::istio::mixer_control::tcp::Controller* controller() { return controller_.get(); @@ -68,8 +65,6 @@ class TcpMixerControl final : public ThreadLocal::ThreadLocalObject { private: // The mixer control std::unique_ptr<::istio::mixer_control::tcp::Controller> controller_; - - MixerStatsObject stats_obj_; }; } // namespace Mixer diff --git a/src/envoy/mixer/stats.cc b/src/envoy/mixer/stats.cc deleted file mode 100644 index 6796aa851a6..00000000000 --- a/src/envoy/mixer/stats.cc +++ /dev/null @@ -1,100 +0,0 @@ -/* Copyright 2017 Istio Authors. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include - -#include "src/envoy/mixer/stats.h" - -namespace Envoy { -namespace Http { -namespace Mixer { -namespace { - -// The time interval for envoy stats update. -const int kStatsUpdateIntervalInMs = 10000; - -} // namespace - -MixerStatsObject::MixerStatsObject(Event::Dispatcher& dispatcher, - const std::string& name, Stats::Scope& scope, - GetStatsFunc func) - : stats_{ALL_MIXER_FILTER_STATS(POOL_COUNTER_PREFIX(scope, name))}, - get_stats_func_(func) { - memset(&old_stats_, 0, sizeof(old_stats_)); - - if (get_stats_func_) { - timer_ = dispatcher.createTimer([this]() { OnTimer(); }); - timer_->enableTimer(std::chrono::milliseconds(kStatsUpdateIntervalInMs)); - } -} - -void MixerStatsObject::OnTimer() { - ::istio::mixer_client::Statistics new_stats; - bool get_stats = get_stats_func_(&new_stats); - if (get_stats) { - CheckAndUpdateStats(new_stats); - } - timer_->enableTimer(std::chrono::milliseconds(kStatsUpdateIntervalInMs)); -} - -void MixerStatsObject::CheckAndUpdateStats( - const ::istio::mixer_client::Statistics& new_stats) { - if (new_stats.total_check_calls > old_stats_.total_check_calls) { - stats_.total_check_calls_.add(new_stats.total_check_calls - - old_stats_.total_check_calls); - } - if (new_stats.total_remote_check_calls > - old_stats_.total_remote_check_calls) { - stats_.total_remote_check_calls_.add(new_stats.total_remote_check_calls - - old_stats_.total_remote_check_calls); - } - if (new_stats.total_blocking_remote_check_calls > - old_stats_.total_blocking_remote_check_calls) { - stats_.total_blocking_remote_check_calls_.add( - new_stats.total_blocking_remote_check_calls - - old_stats_.total_blocking_remote_check_calls); - } - if (new_stats.total_quota_calls > old_stats_.total_quota_calls) { - stats_.total_quota_calls_.add(new_stats.total_quota_calls - - old_stats_.total_quota_calls); - } - if (new_stats.total_remote_quota_calls > - old_stats_.total_remote_quota_calls) { - stats_.total_remote_quota_calls_.add(new_stats.total_remote_quota_calls - - old_stats_.total_remote_quota_calls); - } - if (new_stats.total_blocking_remote_quota_calls > - old_stats_.total_blocking_remote_quota_calls) { - stats_.total_blocking_remote_quota_calls_.add( - new_stats.total_blocking_remote_quota_calls - - old_stats_.total_blocking_remote_quota_calls); - } - if (new_stats.total_report_calls > old_stats_.total_report_calls) { - stats_.total_report_calls_.add(new_stats.total_report_calls - - old_stats_.total_report_calls); - } - if (new_stats.total_remote_report_calls > - old_stats_.total_remote_report_calls) { - stats_.total_remote_report_calls_.add(new_stats.total_remote_report_calls - - old_stats_.total_remote_report_calls); - } - - // Copy new_stats to old_stats_ for next stats update. - old_stats_ = new_stats; -} - -} // namespace Mixer -} // namespace Http -} // namespace Envoy diff --git a/src/envoy/mixer/stats.h b/src/envoy/mixer/stats.h deleted file mode 100644 index 5c2a05e8770..00000000000 --- a/src/envoy/mixer/stats.h +++ /dev/null @@ -1,81 +0,0 @@ -/* Copyright 2017 Istio Authors. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#pragma once - -#include "envoy/event/dispatcher.h" -#include "envoy/event/timer.h" -#include "envoy/stats/stats_macros.h" -#include "include/client.h" - -namespace Envoy { -namespace Http { -namespace Mixer { - -/** - * All mixer filter stats. @see stats_macros.h - */ -// clang-format off -#define ALL_MIXER_FILTER_STATS(COUNTER) \ - COUNTER(total_check_calls) \ - COUNTER(total_remote_check_calls) \ - COUNTER(total_blocking_remote_check_calls) \ - COUNTER(total_quota_calls) \ - COUNTER(total_remote_quota_calls) \ - COUNTER(total_blocking_remote_quota_calls) \ - COUNTER(total_report_calls) \ - COUNTER(total_remote_report_calls) -// clang-format on - -/** - * Struct definition for all mixer filter stats. @see stats_macros.h - */ -struct MixerFilterStats { - ALL_MIXER_FILTER_STATS(GENERATE_COUNTER_STRUCT) -}; - -typedef std::function GetStatsFunc; - -// MixerStatsObject maintains statistics for number of check, quota and report -// calls issued by a mixer filter. -class MixerStatsObject { - public: - MixerStatsObject(Event::Dispatcher& dispatcher, const std::string& name, - Stats::Scope& scope, GetStatsFunc func); - - private: - // This function is invoked when timer event fires. - void OnTimer(); - - // Compares old stats with new stats and updates envoy stats. - void CheckAndUpdateStats(const ::istio::mixer_client::Statistics& new_stats); - - // A set of Envoy stats for the number of check, quota and report calls. - MixerFilterStats stats_; - // Stores a function which gets statistics from mixer controller. - GetStatsFunc get_stats_func_; - - // stats from last call to get_stats_func_. This is needed to calculate the - // variances of stats and update envoy stats. - ::istio::mixer_client::Statistics old_stats_; - - // These members are used for creating a timer which update Envoy stats - // periodically. - ::Envoy::Event::TimerPtr timer_; -}; - -} // namespace Mixer -} // namespace Http -} // namespace Envoy diff --git a/src/envoy/mixer/tcp_filter.cc b/src/envoy/mixer/tcp_filter.cc index 158d89dab07..a18c983d586 100644 --- a/src/envoy/mixer/tcp_filter.cc +++ b/src/envoy/mixer/tcp_filter.cc @@ -44,13 +44,12 @@ class TcpConfig : public Logger::Loggable { tls_(context.threadLocal().allocateSlot()) { mixer_config_.Load(config); Runtime::RandomGenerator& random = context.random(); - Stats::Scope& scope = context.scope(); - tls_->set([this, &random, &scope](Event::Dispatcher& dispatcher) - -> ThreadLocal::ThreadLocalObjectSharedPtr { - return ThreadLocal::ThreadLocalObjectSharedPtr( - new TcpMixerControl(mixer_config_, cm_, dispatcher, - random, scope)); - }); + tls_->set( + [this, &random](Event::Dispatcher& dispatcher) + -> ThreadLocal::ThreadLocalObjectSharedPtr { + return ThreadLocal::ThreadLocalObjectSharedPtr( + new TcpMixerControl(mixer_config_, cm_, dispatcher, random)); + }); } TcpMixerControl& mixer_control() { return tls_->getTyped(); }