mgr: respect perf counter prio_adjust in MgrClient

This awkwardly involves re-ordering some definitions
in perf_counters.h in order to refer to the prio
names defined in PerfCountersBuilder.

Signed-off-by: John Spray <john.spray@redhat.com>
(cherry picked from commit 88163749b572ffd2bfe0850136fad5dbed2a9180)

diff --git a/src/common/perf_counters.cc b/src/common/perf_counters.cc
index 34c6d5e29428633eead8975eaf7607d928fd04f2..048c93a64334a20ef59abdbc4eff08d167979ad7 100644 (file)
--- a/src/common/perf_counters.cc
+++ b/src/common/perf_counters.cc
@@ -53,7 +53,7 @@ void PerfCountersCollection::add(class PerfCounters *l)
     path += ".";
     path += data.name;
 
-    by_path[path] = &data;
+    by_path[path] = {&data, l};
   }
 }
 
@@ -396,10 +396,7 @@ void PerfCounters::dump_formatted_generic(Formatter *f, bool schema,
       } else {
         f->dump_string("nick", "");
       }
-      int p = std::max(std::min(d->prio + prio_adjust,
-                                (int)PerfCountersBuilder::PRIO_CRITICAL),
-                       0);
-      f->dump_int("priority", p);
+      f->dump_int("priority", get_adjusted_priority(d->prio));
       f->close_section();
     } else {
       if (d->type & PERFCOUNTER_LONGRUNAVG) {

diff --git a/src/common/perf_counters.h b/src/common/perf_counters.h
index 10cd0450e5230c9c6e151d9b273b9e12554d8c40..846e407ad2f0d0cd008c9435e88fe35012e0787e 100644 (file)
--- a/src/common/perf_counters.h
+++ b/src/common/perf_counters.h
@@ -42,6 +42,80 @@ enum perfcounter_type_d : uint8_t
 };
 
 
+/* Class for constructing a PerfCounters object.
+ *
+ * This class performs some validation that the parameters we have supplied are
+ * correct in create_perf_counters().
+ *
+ * In the future, we will probably get rid of the first/last arguments, since
+ * PerfCountersBuilder can deduce them itself.
+ */
+class PerfCountersBuilder
+{
+public:
+  PerfCountersBuilder(CephContext *cct, const std::string &name,
+                   int first, int last);
+  ~PerfCountersBuilder();
+
+  // prio values: higher is better, and higher values get included in
+  // 'ceph daemonperf' (and similar) results.
+  // Use of priorities enables us to add large numbers of counters
+  // internally without necessarily overwhelming consumers.
+  enum {
+    PRIO_CRITICAL = 10,
+    // 'interesting' is the default threshold for `daemonperf` output
+    PRIO_INTERESTING = 8,
+    // `useful` is the default threshold for transmission to ceph-mgr
+    // and inclusion in prometheus/influxdb plugin output
+    PRIO_USEFUL = 5,
+    PRIO_UNINTERESTING = 2,
+    PRIO_DEBUGONLY = 0,
+  };
+  void add_u64(int key, const char *name,
+              const char *description=NULL, const char *nick = NULL,
+              int prio=0);
+  void add_u64_counter(int key, const char *name,
+                      const char *description=NULL,
+                      const char *nick = NULL,
+                      int prio=0);
+  void add_u64_avg(int key, const char *name,
+                  const char *description=NULL,
+                  const char *nick = NULL,
+                  int prio=0);
+  void add_time(int key, const char *name,
+               const char *description=NULL,
+               const char *nick = NULL,
+               int prio=0);
+  void add_time_avg(int key, const char *name,
+                   const char *description=NULL,
+                   const char *nick = NULL,
+                   int prio=0);
+  void add_u64_counter_histogram(
+    int key, const char* name,
+    PerfHistogramCommon::axis_config_d x_axis_config,
+    PerfHistogramCommon::axis_config_d y_axis_config,
+    const char *description=NULL,
+    const char* nick = NULL,
+    int prio=0);
+
+  void set_prio_default(int prio_)
+  {
+    prio_default = prio_;
+  }
+
+  PerfCounters* create_perf_counters();
+private:
+  PerfCountersBuilder(const PerfCountersBuilder &rhs);
+  PerfCountersBuilder& operator=(const PerfCountersBuilder &rhs);
+  void add_impl(int idx, const char *name,
+                const char *description, const char *nick, int prio, int ty,
+                unique_ptr<PerfHistogram<>> histogram = nullptr);
+
+  PerfCounters *m_perf_counters;
+
+  int prio_default = 0;
+};
+
 /*
  * A PerfCounters object is usually associated with a single subsystem.
  * It contains counters which we modify to track performance and throughput
@@ -179,6 +253,12 @@ public:
     prio_adjust = p;
   }
 
+  int get_adjusted_priority(int p) const {
+    return std::max(std::min(p + prio_adjust,
+                             (int)PerfCountersBuilder::PRIO_CRITICAL),
+                    0);
+  }
+
 private:
   PerfCounters(CephContext *cct, const std::string &name,
             int lower_bound, int upper_bound);
@@ -240,8 +320,17 @@ public:
     dump_formatted_generic(f, schema, true, logger, counter);
   }
 
+  // A reference to a perf_counter_data_any_d, with an accompanying
+  // pointer to the enclosing PerfCounters, in order that the consumer
+  // can see the prio_adjust
+  class PerfCounterRef
+  {
+    public:
+    PerfCounters::perf_counter_data_any_d *data;
+    PerfCounters *perf_counters;
+  };
   typedef std::map<std::string,
-          PerfCounters::perf_counter_data_any_d *> CounterMap;
+          PerfCounterRef> CounterMap;
 
   void with_counters(std::function<void(const CounterMap &)>) const;
 
@@ -257,83 +346,11 @@ private:
 
   perf_counters_set_t m_loggers;
 
-  std::map<std::string, PerfCounters::perf_counter_data_any_d *> by_path; 
+  CounterMap by_path; 
 
   friend class PerfCountersCollectionTest;
 };
 
-/* Class for constructing a PerfCounters object.
- *
- * This class performs some validation that the parameters we have supplied are
- * correct in create_perf_counters().
- *
- * In the future, we will probably get rid of the first/last arguments, since
- * PerfCountersBuilder can deduce them itself.
- */
-class PerfCountersBuilder
-{
-public:
-  PerfCountersBuilder(CephContext *cct, const std::string &name,
-                   int first, int last);
-  ~PerfCountersBuilder();
 
-  // prio values: higher is better, and higher values get included in
-  // 'ceph daemonperf' (and similar) results.
-  // Use of priorities enables us to add large numbers of counters
-  // internally without necessarily overwhelming consumers.
-  enum {
-    PRIO_CRITICAL = 10,
-    // 'interesting' is the default threshold for `daemonperf` output
-    PRIO_INTERESTING = 8,
-    // `useful` is the default threshold for transmission to ceph-mgr
-    // and inclusion in prometheus/influxdb plugin output
-    PRIO_USEFUL = 5,
-    PRIO_UNINTERESTING = 2,
-    PRIO_DEBUGONLY = 0,
-  };
-  void add_u64(int key, const char *name,
-              const char *description=NULL, const char *nick = NULL,
-              int prio=0);
-  void add_u64_counter(int key, const char *name,
-                      const char *description=NULL,
-                      const char *nick = NULL,
-                      int prio=0);
-  void add_u64_avg(int key, const char *name,
-                  const char *description=NULL,
-                  const char *nick = NULL,
-                  int prio=0);
-  void add_time(int key, const char *name,
-               const char *description=NULL,
-               const char *nick = NULL,
-               int prio=0);
-  void add_time_avg(int key, const char *name,
-                   const char *description=NULL,
-                   const char *nick = NULL,
-                   int prio=0);
-  void add_u64_counter_histogram(
-    int key, const char* name,
-    PerfHistogramCommon::axis_config_d x_axis_config,
-    PerfHistogramCommon::axis_config_d y_axis_config,
-    const char *description=NULL,
-    const char* nick = NULL,
-    int prio=0);
-
-  void set_prio_default(int prio_)
-  {
-    prio_default = prio_;
-  }
-
-  PerfCounters* create_perf_counters();
-private:
-  PerfCountersBuilder(const PerfCountersBuilder &rhs);
-  PerfCountersBuilder& operator=(const PerfCountersBuilder &rhs);
-  void add_impl(int idx, const char *name,
-                const char *description, const char *nick, int prio, int ty,
-                unique_ptr<PerfHistogram<>> histogram = nullptr);
-
-  PerfCounters *m_perf_counters;
-
-  int prio_default = 0;
-};
 
 #endif

diff --git a/src/mgr/MgrClient.cc b/src/mgr/MgrClient.cc
index dd35e27141720a2861c49b181f470b7259b2cb5e..0e4b01931ef2e84ec9c4c36f5ea58d6feafa8419 100644 (file)
--- a/src/mgr/MgrClient.cc
+++ b/src/mgr/MgrClient.cc
@@ -227,27 +227,43 @@ void MgrClient::send_report()
   pcc->with_counters([this, report](
         const PerfCountersCollection::CounterMap &by_path)
   {
+    // Helper for checking whether a counter should be included
     auto include_counter = [this](
-        const PerfCounters::perf_counter_data_any_d &ctr)
+        const PerfCounters::perf_counter_data_any_d &ctr,
+        const PerfCounters &perf_counters)
     {
-      return ctr.prio >= (int)stats_threshold;
+      return perf_counters.get_adjusted_priority(ctr.prio) >= (int)stats_threshold;
+    };
+
+    // Helper for cases where we want to forget a counter
+    auto undeclare = [report, this](const std::string &path)
+    {
+      report->undeclare_types.push_back(path);
+      ldout(cct,20) << " undeclare " << path << dendl;
+      session->declared.erase(path);
     };
 
     ENCODE_START(1, 1, report->packed);
+
+    // Find counters that no longer exist, and undeclare them
     for (auto p = session->declared.begin(); p != session->declared.end(); ) {
-      if (by_path.count(*p) == 0 || !include_counter(*(by_path.at(*p)))) {
-       report->undeclare_types.push_back(*p);
-       ldout(cct,20) << __func__ << " undeclare " << *p << dendl;
-       p = session->declared.erase(p);
-      } else {
-       ++p;
+      const auto &path = *(p++);
+      if (by_path.count(path) == 0) {
+        undeclare(path);
       }
     }
+
     for (const auto &i : by_path) {
       auto& path = i.first;
-      auto& data = *(i.second);
-
-      if (!include_counter(data)) {
+      auto& data = *(i.second.data);
+      auto& perf_counters = *(i.second.perf_counters);
+
+      // Find counters that still exist, but are no longer permitted by
+      // stats_threshold
+      if (!include_counter(data, perf_counters)) {
+        if (session->declared.count(path)) {
+          undeclare(path);
+        }
         continue;
       }
 
@@ -262,7 +278,7 @@ void MgrClient::send_report()
          type.nick = data.nick;
        }
        type.type = data.type;
-        type.priority = data.prio;
+        type.priority = perf_counters.get_adjusted_priority(data.prio);
        report->declare_types.push_back(std::move(type));
        session->declared.insert(path);
       }