Support envoy to provide stats generated by mixer filter. by JimmyCYJ · Pull Request #891 · istio/proxy

JimmyCYJ · 2018-01-03T01:54:28Z

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #132
istio/old_mixerclient_repo#132

Special notes for your reviewer:

Release note:

Support Envoy to provide stats generated by mixer filter

istio-merge-robot · 2018-01-03T01:54:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: linsun

Assign the PR to them by writing /assign @linsun in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mandarjog · 2018-01-03T02:03:30Z

src/envoy/mixer/http_filter.cc

+ */
+// clang-format off
+#define ALL_HTTP_MIXER_FILTER_STATS(COUNTER, GAUGE)                           \
+  COUNTER(total_check_calls)                                                  \


We also want separate check and report latencies as histograms. The histograms should use the same bucketization as used by envoy upstream_XXX counters.
upstream_mixer_XXX stats mash checks and reports together.

@JimmyCYJ please file a separate issue to add Check/Report latency stats. Don't want to put too much change into one PR.

I filed a another issue here. istio/old_mixerclient_repo#324
Thanks.

qiwzhang · 2018-01-03T02:18:08Z

src/envoy/mixer/http_filter.cc

        &check_data, &header_update,
        CheckTransport::GetFunc(mixer_control_.cm(), &headers),
        [this](const Status& status) { completeCheck(status); });
+    CheckAndUpdateStats(old_stats);


Use a timer of every 10 seconds to update counters. Not on every call.

qiwzhang · 2018-01-03T02:19:23Z

src/envoy/mixer/http_filter.cc

+ */
+struct InstanceStats {
+  ALL_HTTP_MIXER_FILTER_STATS(GENERATE_COUNTER_STRUCT)
+};


Maybe we can extract this into a separate header file so tcp_filter can use it too. tcp has the same counters with a different prefix.

qiwzhang · 2018-01-04T02:41:43Z

src/envoy/mixer/stats.h

+/**
+ * Struct definition for all mixer filter stats. @see stats_macros.h
+ */
+struct InstanceStats {


call it MixerFilterStats

qiwzhang · 2018-01-04T02:42:50Z

src/envoy/mixer/mixer_control.h


+// MixerStatsObject maintains statistics for number of check, quota and report
+// calls issued by a mixer filter.
+class MixerStatsObject {


can move this class into stats.h

qiwzhang · 2018-01-04T02:47:16Z

src/envoy/mixer/mixer_control.cc

+  // Start timer for updating envoy stats periodically.
+  timer_.reset(new EnvoyTimer(
+      dispatcher.createTimer([this]() { StatsUpdateCallback(); })));
+  timer_->Start(MixerStatsObject::kStatsUpdateIntervalInMs);


Don't use EnvoyTimer.

use dispatcher.createTimer() directly

qiwzhang · 2018-01-04T02:49:34Z

src/envoy/mixer/mixer_control.cc

+  }
+
+  // Copy new_stats to old_stats_ for next stats update.
+  old_stats_.total_check_calls = new_stats.total_check_calls;


can you just do
old_stats_ = new_stats;

qiwzhang · 2018-01-04T02:55:31Z

src/envoy/mixer/mixer_control.h

+
+  // These members are needed to update envoy stats periodically.
+  MixerStatsObject stats_;
+  std::unique_ptr<::istio::mixer_client::Timer> timer_;


I think the timer logic can be moved into MixerStatsObj class.
It will take a function call in the contructor to call controller_->GetStatistitis()

qiwzhang · 2018-01-04T22:42:40Z

src/envoy/mixer/stats.h

+ * All http mixer filter stats. @see stats_macros.h
+ */
+// clang-format off
+#define ALL_HTTP_MIXER_FILTER_STATS(COUNTER)                                  \


remove "HTTP_"

qiwzhang · 2018-01-04T22:43:53Z

src/envoy/mixer/stats.h

+// calls issued by a mixer filter.
+class MixerStatsObject {
+ public:
+  static const int kStatsUpdateIntervalInMs;


this "const int" can be anonymous in cc file?

qiwzhang · 2018-01-04T22:45:24Z

src/envoy/mixer/stats.h

+
+ private:
+  MixerFilterStats stats_;
+  GetStatsFunc get_statistics_;


get_stats_func_? suffixed with func.

also add comment for each class variable.

qiwzhang · 2018-01-04T22:54:14Z

src/envoy/mixer/mixer_control.cc

+
+void MixerControlBase::SetUpStatsTimer() {
+  timer_ = dispatcher_.createTimer([this]() -> void { StatsUpdateCallback(); });
+  timer_->enableTimer(std::chrono::milliseconds(MixerStatsObject::kStatsUpdateIntervalInMs));


can move timer_ into StatsObj class

qiwzhang · 2018-01-04T22:55:08Z

src/envoy/mixer/mixer_control.h

+
+ private:
+  Event::Dispatcher& dispatcher_;
+  ::Envoy::Event::TimerPtr timer_;


if you move timer_ into StatsObject, you may not need MixerControlBase class

qiwzhang · 2018-01-04T22:55:43Z

src/envoy/mixer/mixer_control.cc

+  ::istio::mixer_client::Statistics new_stats;
+  stats_.GetStatistics(&new_stats);
+  stats_.CheckAndUpdateStats(new_stats);
+  timer_->enableTimer(std::chrono::milliseconds(MixerStatsObject::kStatsUpdateIntervalInMs));


I don't think you need to call "enable" for each call. Only need to enable it once.

qiwzhang · 2018-01-04T22:56:15Z

src/envoy/mixer/mixer_control.cc

+  timer_->enableTimer(std::chrono::milliseconds(MixerStatsObject::kStatsUpdateIntervalInMs));
+}
+
+void MixerControlBase::StatsUpdateCallback() {


call it OnTimer()

JimmyCYJ · 2018-01-05T01:12:44Z

Please take a look. Thanks!

qiwzhang · 2018-01-05T01:18:19Z

src/envoy/mixer/mixer_control.h

                   Upstream::ClusterManager& cm, Event::Dispatcher& dispatcher,
-                   Runtime::RandomGenerator& random);
+                   Runtime::RandomGenerator& random,
+                   const std::string& stats_prefix, Stats::Scope& scope);


Not need to pass in stats_prefix. It is already in HttpMixerControl constructor.

qiwzhang · 2018-01-05T01:19:47Z

src/envoy/mixer/stats.h

+  void OnTimer();
+
+  // Compares old stats with new stats and updates envoy stats.
+  void CheckAndUpdateStats(const ::istio::mixer_client::Statistics& new_stats);


Only constructor is public, all these functions can be private.

Can we pass GetStatsFunc in the constructor, not use a separate function to set it.?

qiwzhang · 2018-01-05T01:20:29Z

src/envoy/mixer/stats.h

+  // These members are used for creating a timer which update Envoy stats
+  // periodically.
+  ::Envoy::Event::TimerPtr timer_;
+  ::Envoy::Event::Dispatcher& dispatcher_;


do you need to store dispatcher_?

qiwzhang · 2018-01-05T01:22:19Z

src/envoy/mixer/stats.cc

+}
+
+void MixerStatsObject::SetUpStatsTimer() {
+  timer_ = dispatcher_.createTimer([this]() -> void { OnTimer(); });


can we create timer at constructor so dispatcher_ doesn't need to be kept?

qiwzhang · 2018-01-05T01:23:26Z

src/envoy/mixer/stats.cc

+
+void MixerStatsObject::OnTimer() {
+  ::istio::mixer_client::Statistics new_stats;
+  GetStatistics(&new_stats);


at constructor, if get_stats_func_ is null, don't create timer.

qiwzhang · 2018-01-05T01:24:01Z

src/envoy/mixer/stats.cc

+}
+
+void MixerStatsObject::InitGetStatisticsFunc(GetStatsFunc get_stats) {
+  if (get_stats) {


set function as constructor.

qiwzhang · 2018-01-05T01:24:15Z

src/envoy/mixer/stats.cc

+  if (get_stats_func_) {
+    get_stats_func_(stats);
+  } else {
+    auto& logger = Logger::Registry::getLog(Logger::Id::config);


at constructor, if get_stats_func_ is null, don't create timer. not need for this check

qiwzhang · 2018-01-05T01:26:09Z

src/envoy/mixer/http_filter.cc

 };

+const std::string kStatsPrefix("http_mixer_filter.");
+


move these two prefix into mixer_control.cc to be used at their constructors

qiwzhang · 2018-01-05T01:30:50Z

src/envoy/mixer/mixer_control.cc

+
+  stats_obj_.InitGetStatisticsFunc(
+      std::bind(&::istio::mixer_control::tcp::Controller::GetStatistics,
+                controller(), std::placeholders::_1));


please use lambda function, such as

do it in the consturctor.

stats_obj_(dispatcher, "tcp_mixer_filter.", scope,
[this](STat *stat) -> bool {
if (!controller_) return false;
controler_->GetStats(stat);
return true;
});

qiwzhang

Very good. Almost there.

We need integration test. Can be a separate PR if you like.

qiwzhang · 2018-01-05T02:30:27Z

src/envoy/mixer/stats.cc

-  } else {
-    auto& logger = Logger::Registry::getLog(Logger::Id::config);
-    ENVOY_LOG_TO_LOGGER(logger, error, "get_stats_func_ is empty");
+    timer_ = dispatcher.createTimer([this]() -> void { OnTimer(); });


you can remove " -> void"

just:

this {OnTimer(); }

qiwzhang · 2018-01-05T02:31:43Z

src/envoy/mixer/stats.cc

+    CheckAndUpdateStats(new_stats);
+    timer_->enableTimer(std::chrono::milliseconds(kStatsUpdateIntervalInMs));
+  } else {
+    auto& logger = Logger::Registry::getLog(Logger::Id::config);


not need to log anything for else cast

qiwzhang · 2018-01-05T02:34:29Z

src/envoy/mixer/mixer_control.h

 #include "control/include/http/controller.h"
 #include "control/include/tcp/controller.h"
 #include "envoy/event/dispatcher.h"
+#include "envoy/event/timer.h"


timer.h is not needed

qiwzhang · 2018-01-05T02:35:00Z

src/envoy/mixer/mixer_control.cc


-#include "src/envoy/mixer/mixer_control.h"
+#include <chrono>
+#include <memory>


why these two header files are needed here?

qiwzhang · 2018-01-05T02:35:44Z

src/envoy/mixer/stats.cc

+namespace Mixer {
+namespace {
+
+const int kStatsUpdateIntervalInMs = 10000;


Add comment for const variable.

qiwzhang · 2018-01-05T05:37:55Z

src/envoy/mixer/stats.cc


  if (get_stats_func_) {
-    timer_ = dispatcher.createTimer([this]() -> void { OnTimer(); });
+    timer_ = dispatcher.createTimer([this] { OnTimer(); });


The format is wrong, the lambda should be:

this {OnTimer(); }

.createTime(this {OnTimer(); });

you need () after square []

qiwzhang · 2018-01-05T18:08:46Z

src/envoy/mixer/mixer_control.cc


 #include "src/envoy/mixer/mixer_control.h"
 #include "src/envoy/mixer/grpc_transport.h"
+#include "src/envoy/mixer/stats.h"


no need for this include. already included in the mxer_control.h

qiwzhang · 2018-01-05T18:11:00Z

src/envoy/mixer/stats.cc

+  bool get_stats = get_stats_func_(&new_stats);
+  if (get_stats) {
+    CheckAndUpdateStats(new_stats);
+    timer_->enableTimer(std::chrono::milliseconds(kStatsUpdateIntervalInMs));


should always enable timer

Done. Thanks!

…tio#891)" This reverts commit 147edd1.

JimmyCYJ requested a review from qiwzhang January 3, 2018 01:54

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 3, 2018

mandarjog reviewed Jan 3, 2018

View reviewed changes

qiwzhang reviewed Jan 3, 2018

View reviewed changes

qiwzhang reviewed Jan 4, 2018

View reviewed changes

qiwzhang reviewed Jan 5, 2018

View reviewed changes

JimmyCYJ added 11 commits January 5, 2018 18:30

Support envoy to provide stats generated by mixer filter.

2d0a3a7

revise per review comments.

6bd7df7

Revise per comment.

395e960

Fixed format issues.

796c80a

Revise per comments

bf97be5

Revise per comment.

c1858e4

Revise per comments.

cb6e165

Revise per comments.

8178e10

Revise per comment.

6e6947e

Always enable timer for next stats update.

d0f1f09

Remove unnecessary included header file.

cd3966d

JimmyCYJ force-pushed the add_stats_to_mixer_filter branch from 2e84e4e to cd3966d Compare January 5, 2018 18:31

qiwzhang approved these changes Jan 5, 2018

View reviewed changes

qiwzhang merged commit 147edd1 into istio:master Jan 5, 2018

JimmyCYJ deleted the add_stats_to_mixer_filter branch January 5, 2018 19:26

JimmyCYJ mentioned this pull request Jan 23, 2018

Add integration tests to verify Envoy stats for mixer check, quota and report calls. istio/istio#2771

Closed

JimmyCYJ added a commit to JimmyCYJ/proxy that referenced this pull request Jan 30, 2018

Revert "Support envoy to provide stats generated by mixer filter. (is…

16f80e2

…tio#891)" This reverts commit 147edd1.

Conversation

JimmyCYJ commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-merge-robot commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimmyCYJ commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JimmyCYJ commented Jan 3, 2018 •

edited

Loading