Provide stats for mixer client. (istio#315)

* Provide stats for mixer client.

* Addressed some review comments.

* Fix presubmit test failure.

* Fix presubmit test failure.

Author: JimmyCYJ

mixerclient/control/src/mock_mixer_client.h

@@ -32,6 +32,8 @@ class MockMixerClient : public ::istio::mixer_client::MixerClient {
                    ::istio::mixer_client::TransportCheckFunc transport,
                    ::istio::mixer_client::DoneFunc on_done));
   MOCK_METHOD1(Report, void(const ::istio::mixer::v1::Attributes& attributes));
+  MOCK_CONST_METHOD1(GetStatistics,
+                     void(::istio::mixer_client::Statistics* stat));
 };
 
 }  // namespace mixer_control

mixerclient/include/client.h

@@ -48,6 +48,28 @@ struct MixerClientOptions {
   Environment env;
 };
 
+// The statistics recorded by mixerclient library.
+struct Statistics {
+  // Total number of check calls.
+  uint64_t total_check_calls;
+  // Total number of remote check calls.
+  uint64_t total_remote_check_calls;
+  // Total number of remote check calls that blocking origin requests.
+  uint64_t total_blocking_remote_check_calls;
+
+  // Total number of quota calls.
+  uint64_t total_quota_calls;
+  // Total number of remote quota calls.
+  uint64_t total_remote_quota_calls;
+  // Total number of remote quota calls that blocking origin requests.
+  uint64_t total_blocking_remote_quota_calls;
+
+  // Total number of report calls.
+  uint64_t total_report_calls;
+  // Total number of remote report calls.
+  uint64_t total_remote_report_calls;
+};
+
 class MixerClient {
  public:
   // Destructor
@@ -69,6 +91,9 @@ class MixerClient {
 
   // A report call.
   virtual void Report(const ::istio::mixer::v1::Attributes& attributes) = 0;
+
+  // Get statistics.
+  virtual void GetStatistics(Statistics* stat) const = 0;
 };
 
 // Creates a MixerClient object.

mixerclient/src/client_impl.cc

@@ -39,6 +39,13 @@ MixerClientImpl::MixerClientImpl(const MixerClientOptions &options)
   if (options_.env.uuid_generate_func) {
     deduplication_id_base_ = options_.env.uuid_generate_func();
   }
+
+  total_check_calls_ = 0;
+  total_remote_check_calls_ = 0;
+  total_blocking_remote_check_calls_ = 0;
+  total_quota_calls_ = 0;
+  total_remote_quota_calls_ = 0;
+  total_blocking_remote_quota_calls_ = 0;
 }
 
 MixerClientImpl::~MixerClientImpl() {}
@@ -47,6 +54,8 @@ CancelFunc MixerClientImpl::Check(
     const Attributes &attributes,
     const std::vector<::istio::quota::Requirement> &quotas,
     TransportCheckFunc transport, DoneFunc on_done) {
+  ++total_check_calls_;
+
   std::unique_ptr<CheckCache::CheckResult> check_result(
       new CheckCache::CheckResult);
   check_cache_->Check(attributes, check_result.get());
@@ -55,6 +64,9 @@ CancelFunc MixerClientImpl::Check(
     return nullptr;
   }
 
+  if (!quotas.empty()) {
+    ++total_quota_calls_;
+  }
   std::unique_ptr<QuotaCache::CheckResult> quota_result(
       new QuotaCache::CheckResult);
   // Only use quota cache if Check is using cache with OK status.
@@ -87,6 +99,18 @@ CancelFunc MixerClientImpl::Check(
   if (!transport) {
     transport = options_.env.check_transport;
   }
+  // We are going to make a remote call now.
+  ++total_remote_check_calls_;
+  if (!quotas.empty()) {
+    ++total_remote_quota_calls_;
+  }
+  if (on_done) {
+    ++total_blocking_remote_check_calls_;
+    if (!quotas.empty()) {
+      ++total_blocking_remote_quota_calls_;
+    }
+  }
+
   return transport(
       request, response, [this, request_copy, response, raw_check_result,
                           raw_quota_result, on_done](const Status &status) {
@@ -114,6 +138,17 @@ void MixerClientImpl::Report(const Attributes &attributes) {
   report_batch_->Report(attributes);
 }
 
+void MixerClientImpl::GetStatistics(Statistics *stat) const {
+  stat->total_check_calls = total_check_calls_;
+  stat->total_remote_check_calls = total_remote_check_calls_;
+  stat->total_blocking_remote_check_calls = total_blocking_remote_check_calls_;
+  stat->total_quota_calls = total_quota_calls_;
+  stat->total_remote_quota_calls = total_remote_quota_calls_;
+  stat->total_blocking_remote_quota_calls = total_blocking_remote_quota_calls_;
+  stat->total_report_calls = report_batch_->total_report_calls();
+  stat->total_remote_report_calls = report_batch_->total_remote_report_calls();
+}
+
 // Creates a MixerClient object.
 std::unique_ptr<MixerClient> CreateMixerClient(
     const MixerClientOptions &options) {

mixerclient/src/client_impl.h

@@ -35,11 +35,12 @@ class MixerClientImpl : public MixerClient {
   // Destructor
   virtual ~MixerClientImpl();
 
-  virtual CancelFunc Check(
-      const ::istio::mixer::v1::Attributes& attributes,
-      const std::vector<::istio::quota::Requirement>& quotas,
-      TransportCheckFunc transport, DoneFunc on_done);
-  virtual void Report(const ::istio::mixer::v1::Attributes& attributes);
+  CancelFunc Check(const ::istio::mixer::v1::Attributes& attributes,
+                   const std::vector<::istio::quota::Requirement>& quotas,
+                   TransportCheckFunc transport, DoneFunc on_done) override;
+  void Report(const ::istio::mixer::v1::Attributes& attributes) override;
+
+  void GetStatistics(Statistics* stat) const override;
 
  private:
   // Store the options
@@ -59,6 +60,18 @@ class MixerClientImpl : public MixerClient {
   std::string deduplication_id_base_;
   std::atomic<std::uint64_t> deduplication_id_;
 
+  // Atomic objects for recording statistics.
+  // check cache miss rate:
+  // total_blocking_remote_check_calls_ / total_check_calls_.
+  // quota cache miss rate:
+  // total_blocking_remote_quota_calls_ / total_quota_calls_.
+  std::atomic_int_fast64_t total_check_calls_;
+  std::atomic_int_fast64_t total_remote_check_calls_;
+  std::atomic_int_fast64_t total_blocking_remote_check_calls_;
+  std::atomic_int_fast64_t total_quota_calls_;
+  std::atomic_int_fast64_t total_remote_quota_calls_;
+  std::atomic_int_fast64_t total_blocking_remote_quota_calls_;
+
   GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(MixerClientImpl);
 };
 

mixerclient/src/client_impl_test.cc

@@ -87,12 +87,23 @@ TEST_F(MixerClientImplTest, TestSuccessCheck) {
   EXPECT_TRUE(done_status.ok());
 
   for (int i = 0; i < 10; i++) {
-    // Other calls should ba cached.
+    // Other calls should be cached.
     Status done_status1 = Status::UNKNOWN;
     client_->Check(request_, empty_quotas, empty_transport_,
                    [&done_status1](Status status) { done_status1 = status; });
     EXPECT_TRUE(done_status1.ok());
   }
+
+  Statistics stat;
+  client_->GetStatistics(&stat);
+  EXPECT_EQ(stat.total_check_calls, 11);
+  // The first check call is a remote blocking check call.
+  EXPECT_EQ(stat.total_remote_check_calls, 1);
+  EXPECT_EQ(stat.total_blocking_remote_check_calls, 1);
+  // Empty quota does not trigger any quota call.
+  EXPECT_EQ(stat.total_quota_calls, 0);
+  EXPECT_EQ(stat.total_remote_quota_calls, 0);
+  EXPECT_EQ(stat.total_blocking_remote_quota_calls, 0);
 }
 
 TEST_F(MixerClientImplTest, TestPerRequestTransport) {
@@ -116,12 +127,23 @@ TEST_F(MixerClientImplTest, TestPerRequestTransport) {
   EXPECT_TRUE(done_status.ok());
 
   for (int i = 0; i < 10; i++) {
-    // Other calls should ba cached.
+    // Other calls should be cached.
     Status done_status1 = Status::UNKNOWN;
     client_->Check(request_, empty_quotas, local_check_transport.GetFunc(),
                    [&done_status1](Status status) { done_status1 = status; });
     EXPECT_TRUE(done_status1.ok());
   }
+
+  Statistics stat;
+  client_->GetStatistics(&stat);
+  EXPECT_EQ(stat.total_check_calls, 11);
+  // The first check call is a remote blocking check call.
+  EXPECT_EQ(stat.total_remote_check_calls, 1);
+  EXPECT_EQ(stat.total_blocking_remote_check_calls, 1);
+  // Empty quota does not trigger any quota call.
+  EXPECT_EQ(stat.total_quota_calls, 0);
+  EXPECT_EQ(stat.total_remote_quota_calls, 0);
+  EXPECT_EQ(stat.total_blocking_remote_quota_calls, 0);
 }
 
 TEST_F(MixerClientImplTest, TestNoCheckCache) {
@@ -146,14 +168,23 @@ TEST_F(MixerClientImplTest, TestNoCheckCache) {
   EXPECT_TRUE(done_status.ok());
 
   for (int i = 0; i < 10; i++) {
-    // Other calls should ba cached.
+    // Other calls are not cached.
     Status done_status1 = Status::UNKNOWN;
     client_->Check(request_, quotas_, empty_transport_,
                    [&done_status1](Status status) { done_status1 = status; });
     EXPECT_TRUE(done_status1.ok());
   }
   // Call count 11 since check is not cached.
-  EXPECT_LE(call_counts, 11);
+  EXPECT_EQ(call_counts, 11);
+  Statistics stat;
+  client_->GetStatistics(&stat);
+  // Because there is no check cache, we make remote blocking call every time.
+  EXPECT_EQ(stat.total_check_calls, 11);
+  EXPECT_EQ(stat.total_remote_check_calls, 11);
+  EXPECT_EQ(stat.total_blocking_remote_check_calls, 11);
+  EXPECT_EQ(stat.total_quota_calls, 11);
+  EXPECT_EQ(stat.total_remote_quota_calls, 11);
+  EXPECT_EQ(stat.total_blocking_remote_quota_calls, 11);
 }
 
 TEST_F(MixerClientImplTest, TestNoQuotaCache) {
@@ -178,14 +209,23 @@ TEST_F(MixerClientImplTest, TestNoQuotaCache) {
   EXPECT_TRUE(done_status.ok());
 
   for (int i = 0; i < 10; i++) {
-    // Other calls should ba cached.
+    // Other calls should be cached.
     Status done_status1 = Status::UNKNOWN;
     client_->Check(request_, quotas_, empty_transport_,
                    [&done_status1](Status status) { done_status1 = status; });
     EXPECT_TRUE(done_status1.ok());
   }
   // Call count 11 since quota is not cached.
-  EXPECT_LE(call_counts, 11);
+  EXPECT_EQ(call_counts, 11);
+  Statistics stat;
+  client_->GetStatistics(&stat);
+  // Because there is no quota cache, we make remote blocking call every time.
+  EXPECT_EQ(stat.total_check_calls, 11);
+  EXPECT_EQ(stat.total_remote_check_calls, 11);
+  EXPECT_EQ(stat.total_blocking_remote_check_calls, 11);
+  EXPECT_EQ(stat.total_quota_calls, 11);
+  EXPECT_EQ(stat.total_remote_quota_calls, 11);
+  EXPECT_EQ(stat.total_blocking_remote_quota_calls, 11);
 }
 
 TEST_F(MixerClientImplTest, TestSuccessCheckAndQuota) {
@@ -208,14 +248,24 @@ TEST_F(MixerClientImplTest, TestSuccessCheckAndQuota) {
   EXPECT_TRUE(done_status.ok());
 
   for (int i = 0; i < 10; i++) {
-    // Other calls should ba cached.
+    // Other calls should be cached.
     Status done_status1 = Status::UNKNOWN;
     client_->Check(request_, quotas_, empty_transport_,
                    [&done_status1](Status status) { done_status1 = status; });
     EXPECT_TRUE(done_status1.ok());
   }
   // Call count should be less than 4
   EXPECT_LE(call_counts, 3);
+  Statistics stat;
+  client_->GetStatistics(&stat);
+  // Less than 4 remote calls are made for prefetching, and they are
+  // non-blocking remote calls.
+  EXPECT_EQ(stat.total_check_calls, 11);
+  EXPECT_LE(stat.total_remote_check_calls, 3);
+  EXPECT_EQ(stat.total_blocking_remote_check_calls, 1);
+  EXPECT_EQ(stat.total_quota_calls, 11);
+  EXPECT_LE(stat.total_remote_quota_calls, 3);
+  EXPECT_EQ(stat.total_blocking_remote_quota_calls, 1);
 }
 
 TEST_F(MixerClientImplTest, TestFailedCheckAndQuota) {
@@ -238,12 +288,22 @@ TEST_F(MixerClientImplTest, TestFailedCheckAndQuota) {
   EXPECT_ERROR_CODE(Code::FAILED_PRECONDITION, done_status);
 
   for (int i = 0; i < 10; i++) {
-    // Other calls should ba cached.
+    // Other calls should be cached.
     Status done_status1 = Status::UNKNOWN;
     client_->Check(request_, quotas_, empty_transport_,
                    [&done_status1](Status status) { done_status1 = status; });
     EXPECT_ERROR_CODE(Code::FAILED_PRECONDITION, done_status1);
   }
+  Statistics stat;
+  client_->GetStatistics(&stat);
+  // The first call is a remote blocking call, which returns failed precondition
+  // in check response. Following calls only make check cache calls and return.
+  EXPECT_EQ(stat.total_check_calls, 11);
+  EXPECT_EQ(stat.total_remote_check_calls, 1);
+  EXPECT_EQ(stat.total_blocking_remote_check_calls, 1);
+  EXPECT_EQ(stat.total_quota_calls, 1);
+  EXPECT_EQ(stat.total_remote_quota_calls, 1);
+  EXPECT_EQ(stat.total_blocking_remote_quota_calls, 1);
 }
 
 }  // namespace

mixerclient/src/report_batch.cc

@@ -32,12 +32,15 @@ ReportBatch::ReportBatch(const ReportOptions& options,
     : options_(options),
       transport_(transport),
       timer_create_(timer_create),
-      compressor_(compressor) {}
+      compressor_(compressor),
+      total_report_calls_(0),
+      total_remote_report_calls_(0) {}
 
 ReportBatch::~ReportBatch() { Flush(); }
 
 void ReportBatch::Report(const Attributes& request) {
   std::lock_guard<std::mutex> lock(mutex_);
+  ++total_report_calls_;
   if (!batch_compressor_) {
     batch_compressor_ = compressor_.CreateBatchCompressor();
   }
@@ -66,6 +69,7 @@ void ReportBatch::FlushWithLock() {
     return;
   }
 
+  ++total_remote_report_calls_;
   std::unique_ptr<ReportRequest> request = batch_compressor_->Finish();
   batch_compressor_.reset();
   if (timer_) {

mixerclient/src/report_batch.h

@@ -16,11 +16,12 @@
 #ifndef MIXERCLIENT_REPORT_BATCH_H
 #define MIXERCLIENT_REPORT_BATCH_H
 
-#include <mutex>
-
 #include "include/client.h"
 #include "src/attribute_compressor.h"
 
+#include <atomic>
+#include <mutex>
+
 namespace istio {
 namespace mixer_client {
 
@@ -32,12 +33,17 @@ class ReportBatch {
 
   virtual ~ReportBatch();
 
-  // Make batched report call
+  // Make batched report call.
   void Report(const ::istio::mixer::v1::Attributes& request);
 
   // Flush out batched reports.
   void Flush();
 
+  uint64_t total_report_calls() const { return total_report_calls_; }
+  uint64_t total_remote_report_calls() const {
+    return total_remote_report_calls_;
+  }
+
  private:
   void FlushWithLock();
 
@@ -62,6 +68,9 @@ class ReportBatch {
   // batched report compressor
   std::unique_ptr<BatchCompressor> batch_compressor_;
 
+  std::atomic_int_fast64_t total_report_calls_;
+  std::atomic_int_fast64_t total_remote_report_calls_;
+
   GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(ReportBatch);
 };