You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/08/07 08:22:50 UTC

[GitHub] [incubator-doris] chaoyli commented on a change in pull request #4115: [metrics] Redesign metrics to 3 layers

chaoyli commented on a change in pull request #4115:
URL: https://github.com/apache/incubator-doris/pull/4115#discussion_r466895319



##########
File path: be/src/util/metrics.h
##########
@@ -193,230 +174,163 @@ class CoreLocalCounter : public Metric {
     void increment(const T& delta) {
         __sync_fetch_and_add(_value.access(), delta);
     }
+
+    rj::Value to_json_value() const override {
+        return rj::Value(value());
+    }
+
 protected:
     CoreLocalValue<T> _value;
 };
 
 template<typename T>
 class AtomicCounter : public AtomicMetric<T> {
 public:
-    AtomicCounter(MetricUnit unit)
-            : AtomicMetric<T>(MetricType::COUNTER, unit) {}
-    virtual ~AtomicCounter() { }
+    AtomicCounter() {}
+    virtual ~AtomicCounter() {}
 };
 
 template<typename T>
 class AtomicGauge : public AtomicMetric<T> {
 public:
-    AtomicGauge(MetricUnit unit)
-            : AtomicMetric<T>(MetricType::GAUGE, unit) {}
-    virtual ~AtomicGauge() { }
+    AtomicGauge() : AtomicMetric<T>() {}
+    virtual ~AtomicGauge() {}
 };
 
 template<typename T>
 class LockCounter : public LockSimpleMetric<T> {
 public:
-    LockCounter(MetricUnit unit)
-      : LockSimpleMetric<T>(MetricType::COUNTER, unit) {}
-    virtual ~LockCounter() { }
+    LockCounter() : LockSimpleMetric<T>() {}
+    virtual ~LockCounter() {}
 };
 
 // This can only used for trival type
 template<typename T>
 class LockGauge : public LockSimpleMetric<T> {
 public:
-    LockGauge(MetricUnit unit)
-      : LockSimpleMetric<T>(MetricType::GAUGE, unit) {}
-    virtual ~LockGauge() { }
+    LockGauge() : LockSimpleMetric<T>() {}
+    virtual ~LockGauge() {}
 };
 
-// one key-value pair used to
-struct MetricLabel {
+using Labels = std::unordered_map<std::string, std::string>;
+struct MetricPrototype {
+public:
+    MetricPrototype(MetricType type_,
+                    MetricUnit unit_,
+                    std::string name_,
+                    std::string description_ = "",
+                    std::string group_name_ = "",
+                    Labels labels_ = Labels(),
+                    bool is_core_metric_ = false)
+        : is_core_metric(is_core_metric_),
+          type(type_),
+          unit(unit_),
+          name(std::move(name_)),
+          description(std::move(description_)),
+          group_name(std::move(group_name_)),
+          labels(std::move(labels_)) {}
+
+    std::string simple_name() const;
+    std::string combine_name(const std::string& registry_name) const;
+
+    bool is_core_metric;
+    MetricType type;
+    MetricUnit unit;
     std::string name;
-    std::string value;
+    std::string description;
+    std::string group_name;
+    Labels labels;
+};
 
-    MetricLabel() { }
-    MetricLabel(const std::string& name_, const std::string& value_) :name(name_), value(value_) {
-    }
+#define DEFINE_METRIC(name, type, unit, desc, group, labels, core)      \

Review comment:
       may be used DEFINE_METRIC_PROTOTYPE better

##########
File path: be/src/util/metrics.h
##########
@@ -193,230 +174,163 @@ class CoreLocalCounter : public Metric {
     void increment(const T& delta) {
         __sync_fetch_and_add(_value.access(), delta);
     }
+
+    rj::Value to_json_value() const override {
+        return rj::Value(value());
+    }
+
 protected:
     CoreLocalValue<T> _value;
 };
 
 template<typename T>
 class AtomicCounter : public AtomicMetric<T> {
 public:
-    AtomicCounter(MetricUnit unit)
-            : AtomicMetric<T>(MetricType::COUNTER, unit) {}
-    virtual ~AtomicCounter() { }
+    AtomicCounter() {}
+    virtual ~AtomicCounter() {}
 };
 
 template<typename T>
 class AtomicGauge : public AtomicMetric<T> {
 public:
-    AtomicGauge(MetricUnit unit)
-            : AtomicMetric<T>(MetricType::GAUGE, unit) {}
-    virtual ~AtomicGauge() { }
+    AtomicGauge() : AtomicMetric<T>() {}
+    virtual ~AtomicGauge() {}
 };
 
 template<typename T>
 class LockCounter : public LockSimpleMetric<T> {
 public:
-    LockCounter(MetricUnit unit)
-      : LockSimpleMetric<T>(MetricType::COUNTER, unit) {}
-    virtual ~LockCounter() { }
+    LockCounter() : LockSimpleMetric<T>() {}
+    virtual ~LockCounter() {}
 };
 
 // This can only used for trival type
 template<typename T>
 class LockGauge : public LockSimpleMetric<T> {
 public:
-    LockGauge(MetricUnit unit)
-      : LockSimpleMetric<T>(MetricType::GAUGE, unit) {}
-    virtual ~LockGauge() { }
+    LockGauge() : LockSimpleMetric<T>() {}
+    virtual ~LockGauge() {}
 };
 
-// one key-value pair used to
-struct MetricLabel {
+using Labels = std::unordered_map<std::string, std::string>;
+struct MetricPrototype {

Review comment:
       What's the purpose of MetricPrototype?
   Under which circumstance, MetricPrototype can be used to split the Metric.

##########
File path: be/src/util/metrics.h
##########
@@ -193,230 +174,163 @@ class CoreLocalCounter : public Metric {
     void increment(const T& delta) {
         __sync_fetch_and_add(_value.access(), delta);
     }
+
+    rj::Value to_json_value() const override {
+        return rj::Value(value());
+    }
+
 protected:
     CoreLocalValue<T> _value;
 };
 
 template<typename T>
 class AtomicCounter : public AtomicMetric<T> {
 public:
-    AtomicCounter(MetricUnit unit)
-            : AtomicMetric<T>(MetricType::COUNTER, unit) {}
-    virtual ~AtomicCounter() { }
+    AtomicCounter() {}
+    virtual ~AtomicCounter() {}
 };
 
 template<typename T>
 class AtomicGauge : public AtomicMetric<T> {
 public:
-    AtomicGauge(MetricUnit unit)
-            : AtomicMetric<T>(MetricType::GAUGE, unit) {}
-    virtual ~AtomicGauge() { }
+    AtomicGauge() : AtomicMetric<T>() {}
+    virtual ~AtomicGauge() {}
 };
 
 template<typename T>
 class LockCounter : public LockSimpleMetric<T> {
 public:
-    LockCounter(MetricUnit unit)
-      : LockSimpleMetric<T>(MetricType::COUNTER, unit) {}
-    virtual ~LockCounter() { }
+    LockCounter() : LockSimpleMetric<T>() {}
+    virtual ~LockCounter() {}
 };
 
 // This can only used for trival type
 template<typename T>
 class LockGauge : public LockSimpleMetric<T> {
 public:
-    LockGauge(MetricUnit unit)
-      : LockSimpleMetric<T>(MetricType::GAUGE, unit) {}
-    virtual ~LockGauge() { }
+    LockGauge() : LockSimpleMetric<T>() {}
+    virtual ~LockGauge() {}
 };
 
-// one key-value pair used to
-struct MetricLabel {
+using Labels = std::unordered_map<std::string, std::string>;
+struct MetricPrototype {
+public:
+    MetricPrototype(MetricType type_,
+                    MetricUnit unit_,
+                    std::string name_,
+                    std::string description_ = "",
+                    std::string group_name_ = "",
+                    Labels labels_ = Labels(),
+                    bool is_core_metric_ = false)
+        : is_core_metric(is_core_metric_),
+          type(type_),
+          unit(unit_),
+          name(std::move(name_)),
+          description(std::move(description_)),
+          group_name(std::move(group_name_)),
+          labels(std::move(labels_)) {}
+
+    std::string simple_name() const;
+    std::string combine_name(const std::string& registry_name) const;
+
+    bool is_core_metric;
+    MetricType type;
+    MetricUnit unit;
     std::string name;
-    std::string value;
+    std::string description;
+    std::string group_name;
+    Labels labels;
+};
 
-    MetricLabel() { }
-    MetricLabel(const std::string& name_, const std::string& value_) :name(name_), value(value_) {
-    }
+#define DEFINE_METRIC(name, type, unit, desc, group, labels, core)      \
+    ::doris::MetricPrototype METRIC_##name(type, unit, #name, desc, group, labels, core)
 
-    bool operator==(const MetricLabel& other) const {
-        return name == other.name && value == other.value;
-    }
-    bool operator!=(const MetricLabel& other) const {
-        return !(*this == other);
-    }
-    bool operator<(const MetricLabel& other) const {
-        auto res = name.compare(other.name);
-        if (res == 0) {
-            return value < other.value;
-        }
-        return res < 0;
-    }
-    int compare(const MetricLabel& other) const {
-        auto res = name.compare(other.name);
-        if (res == 0) {
-            return value.compare(other.value);
-        }
-        return res;
-    }
-    std::string to_string() const {
-        return name + "=" + value;
-    }
-};
+#define DEFINE_COUNTER_METRIC_2ARG(name, unit)                          \
+    DEFINE_METRIC(name, MetricType::COUNTER, unit, "", "", Labels(), false)
 
-struct MetricLabels {
-    static MetricLabels EmptyLabels;
-    // used std::set to sort MetricLabel so that we can get compare two MetricLabels
-    std::set<MetricLabel> labels;
+#define DEFINE_COUNTER_METRIC_3ARG(name, unit, desc)                    \
+    DEFINE_METRIC(name, MetricType::COUNTER, unit, desc, "", Labels(), false)
 
-    MetricLabels& add(const std::string& name, const std::string& value) {
-        labels.emplace(name, value);
-        return *this;
-    }
+#define DEFINE_COUNTER_METRIC_5ARG(name, unit, desc, group, labels)     \
+    DEFINE_METRIC(name, MetricType::COUNTER, unit, desc, #group, labels, false)
 
-    bool operator==(const MetricLabels& other) const {
-        if (labels.size() != other.labels.size()) {
-            return false;
-        }
-        auto it = labels.begin();
-        auto other_it = other.labels.begin();
-        while (it != labels.end()) {
-            if (*it != *other_it) {
-                return false;
-            }
-            ++it;
-            ++other_it;
-        }
-        return true;
-    }
-    bool operator<(const MetricLabels& other) const {
-        auto it = labels.begin();
-        auto other_it = other.labels.begin();
-        while (it != labels.end() && other_it != other.labels.end()) {
-            auto res = it->compare(*other_it);
-            if (res < 0) {
-                return true;
-            } else if (res > 0) {
-                return false;
-            }
-            ++it;
-            ++other_it;
-        }
-        if (it == labels.end()) {
-            if (other_it == other.labels.end()) {
-                return false;
-            }
-            return true;
-        } else {
-            return false;
-        }
-    }
-    bool empty() const {
-        return labels.empty();
+#define DEFINE_GAUGE_METRIC_2ARG(name, unit)                            \
+    DEFINE_METRIC(name, MetricType::GAUGE, unit, "", "", Labels(), false)
+
+#define DEFINE_CORE_GAUGE_METRIC_2ARG(name, unit)                       \
+    DEFINE_METRIC(name, MetricType::GAUGE, unit, "", "", Labels(), true)
+
+#define DEFINE_GAUGE_METRIC_3ARG(name, unit, desc)                      \
+    DEFINE_METRIC(name, MetricType::GAUGE, unit, desc, "", Labels(), false)
+
+#define METRIC_REGISTER(entity, metric)                                 \
+    entity->register_metric(&METRIC_##metric, &metric)
+
+#define METRIC_DEREGISTER(entity, metric)                               \
+    entity->deregister_metric(&METRIC_##metric)
+
+// For 'metrics' in MetricEntity.
+struct MetricPrototypeHash {
+    size_t operator()(const MetricPrototype* metric_prototype) const {
+        return std::hash<std::string>()(metric_prototype->group_name.empty() ? metric_prototype->name : metric_prototype->group_name);
     }
+};
 
-    std::string to_string() const {
-        std::stringstream ss;
-        int i = 0;
-        for (auto& label : labels) {
-            if (i++ > 0) {
-                ss << ",";
-            }
-            ss << label.to_string();
-        }
-        return ss.str();
+struct MetricPrototypeEqualTo {
+    bool operator()(const MetricPrototype* first, const MetricPrototype* second) const {
+        return first->group_name == second->group_name && first->name == second->name;
     }
 };
 
-class MetricCollector;
+using MetricByType = std::unordered_map<const MetricPrototype*, Metric*, MetricPrototypeHash, MetricPrototypeEqualTo>;

Review comment:
       MetricMap is better name or not?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org