CP-1664: modify prometheus-cpp #1

fcimeson · 2019-12-19T16:10:11Z

The prometheus-cpp lib provides functionality to expose internal data to Prometheus via a series of sockets (unique to each node). Typically, each metric will get exposed for the lifetime of the process (node). Thus, if a node created 1000 metrics at the beginning of the program and never updated them again for the entire process, prometheus-cpp would continue exposing them to the socket for the entire process (similar to a while loop with 1000 print lines).

Additionally, prometheus-cpp uses a number of return by references, which poses the following issue:

auto& family = BuildCounter()....
auto& metric = family.Add(labels);
family.Remove(metric);
metric.Increment(); // Segfault

This can be fixed by returning a shared pointer instead. However, the nuance to this is that a shared pointer that has been removed may update its data but that data will not get exposed.

Expired metrics

No time taking under lock.

Less timestamping inside of locked code.

Conflicts: core/include/prometheus/detail/builder.h core/include/prometheus/detail/counter_builder.h core/include/prometheus/detail/histogram_builder.h core/include/prometheus/detail/summary_builder.h core/include/prometheus/family.h core/include/prometheus/registry.h core/src/counter.cc core/src/detail/counter_builder.cc core/src/detail/gauge_builder.cc core/src/detail/histogram_builder.cc core/src/detail/summary_builder.cc core/src/gauge.cc core/src/histogram.cc core/src/registry.cc core/src/summary.cc

…idbots/prometheus-cpp into CP-1664-modify-prometheus-cpp

fcimeson · 2019-12-19T16:22:11Z

core/include/prometheus/counter.h

  ClientMetric Collect() const;

 private:
-  Gauge gauge_{0.0};


The original code used other metrics to keep track of it's data. However, since the new code keeps track of its own retention time I have modified it to keep track of its own data to reduce duplication of the retention time variables.

fcimeson · 2019-12-19T16:29:32Z

core/include/prometheus/family.h


 namespace prometheus {

+enum class RetentionBehavior {Keep, Remove};


Originally, I had this as part of the class definition of Family but I wanted to avoid templating.

fcimeson · 2019-12-19T16:32:18Z

core/include/prometheus/family.h

-    return Add(labels, detail::make_unique<T>(args...));
+  std::shared_ptr<T> Add(const std::map<std::string, std::string>& labels, Args&&... args) {
+    return Add(labels, std::make_shared<T>(std::forward<Args>(args)...));
  }


This method is hear to avoid forward declarations of Args. It is less efficient since it creates an object that may or may not get used. We could instead move the definition here but that would be messy.

fcimeson · 2019-12-19T16:34:43Z

core/include/prometheus/metric_base.h

+ protected:
+  bool has_family_{false};
+  std::atomic<double> retention_time_{1e9};
+  std::atomic<std::time_t> last_update_{std::time(nullptr)};


I'm not convinced we need these to be atomic, but it should be safer. Also, I tried to program it so that we could remove the atomic qualifier and the code would still compile.

I agree with you on this one. I think that atomic is worth having for these variables.

fcimeson · 2019-12-19T16:43:50Z

core/src/histogram.cc

-  bucket_counts_[bucket_index].Increment();
+  sum_ = sum_ + value;
+  bucket_counts_[bucket_index] = bucket_counts_[bucket_index] + 1;
+  last_update_ = std::time(nullptr);


These three lines are atomic operations:

sum_ = x; is equiv to sum_.store(x);

sum_ is equiv to sum_.load()

...

The operators +=, -= didn't seem to be supported by my compiler. Also, the use of this syntax instead of load and store allows us to interchange atomic variables with non-atomic variables.

fcimeson · 2019-12-19T16:46:58Z

core/src/summary.cc

  sum_ += value;
  quantile_values_.insert(value);
+  last_update_ = std::time(nullptr);
+  if (alert) AlertIfNoFamily();


AlertIfNoFamily => PrintAlerts

What does it mean if the alert has no family? Is it that the metric/pointer is scheduled for removal?

Well, clearly you wouldn't understand since your family is awesome!

Joking aside, prometheus keeps a list of metrics (families) for publishing to sockets (which eventually get added to the database). Now that we have the ability to expire metrics (remove them from their family), we want to alert the user if and when they are trying to use a metric that will not get published.

fcimeson · 2019-12-19T16:48:11Z

core/tests/counter_test.cc

 TEST(CounterTest, inc) {
  Counter counter;
-  counter.Increment();
+  counter.Increment(1, false);


Can't test default and suppress alerts. Could, if I brought back the Increment method with no arguments.

core/include/prometheus/family.h

sarahttan · 2020-01-22T15:28:54Z

core/src/summary.cc

  sum_ += value;
  quantile_values_.insert(value);
+  last_update_ = std::time(nullptr);
+  if (alert) AlertIfNoFamily();


What does it mean if the alert has no family? Is it that the metric/pointer is scheduled for removal?

sarahttan · 2020-01-22T16:03:18Z

core/include/prometheus/histogram.h

-  std::vector<Counter> bucket_counts_;
-  Counter sum_;
+  std::atomic<double> sum_{0.0};
+  std::vector<std::atomic<double>> bucket_counts_;


Why was this modified?

Previously histogram used a series of counters to keep track of it's buckets and counters used guages to keep track of their values. This nesting of data structures proved troublesome when we now want to keep track of retention time for each metric (we don't want each bucket to have it's own retention time, and each counter in each bucket have it's own retention time >_<)

sarahttan · 2020-01-22T16:13:09Z

core/include/prometheus/metric_base.h

+ protected:
+  bool has_family_{false};
+  std::atomic<double> retention_time_{1e9};
+  std::atomic<std::time_t> last_update_{std::time(nullptr)};


I agree with you on this one. I think that atomic is worth having for these variables.

core/src/family.cc

sarahttan · 2020-01-22T16:35:46Z

core/src/family.cc

+        if (found) {
+          matched = true;
+        } else {
+          matched = false;
+          break;
+        }


Suggested change

if (found) {

matched = true;

} else {

matched = false;

break;

}

matched = found;

if (!matched) break;

There is sort of a bug in this logic. It should only update matched if not found.

sarahttan · 2020-01-22T16:38:54Z

core/src/family.cc

+  /* Check if the name is a match */
+  if (boost::regex_match(name_, name_expr)) {
+    for (auto metric: metrics_) {
+      bool matched(true);


Based on this logic, I'm not too sure why we have both matched and found. I think it looks like you could just use matched and then set it to true under the constant or metric label conditions.

There is a bit of a bug in the logic, see followup comment.

sarahttan · 2020-01-22T16:51:12Z

core/src/gauge.cc

-  }
-  Change(value);
+void Gauge::Increment(const double& value, const bool& alert) {
+  if (value < 0.0) return;


While I think this line and the Decrement check makes sense, I also wonder if this check is necessary. Like if the user wants chooses to decrement by a negative number, there won't be any warning or error raised, so would it be better to allow the user to do so?

I agree but I'm not sure that warrants changing the original behaviour or not (the more that we leave the same, the easier it will be for merge updates).

sarahttan · 2020-01-22T16:55:02Z

core/src/summary.cc

-
-void Summary::Observe(const double value) {
+                 const std::chrono::milliseconds& max_age,
+                 const int& age_buckets)


Not sure if this change is necessary (to minimize modifications) --> in particular const int& age_buckets versus const int age_buckets

You caught me. I can change it back but I'll wait to see what the other reviewers say.

fcimeson · 2019-12-20T00:45:07Z

core/include/prometheus/registry.h

+  std::vector<std::shared_ptr<Family<Counter>>> counters_;
+  std::vector<std::shared_ptr<Family<Gauge>>> gauges_;
+  std::vector<std::shared_ptr<Family<Histogram>>> histograms_;
+  std::vector<std::shared_ptr<Family<Summary>>> summaries_;


Maybe these should change to maps, to mock the behaviour of metrics in a family.

fcimeson · 2020-01-22T19:09:09Z

core/include/prometheus/histogram.h

-  std::vector<Counter> bucket_counts_;
-  Counter sum_;
+  std::atomic<double> sum_{0.0};
+  std::vector<std::atomic<double>> bucket_counts_;


Previously histogram used a series of counters to keep track of it's buckets and counters used guages to keep track of their values. This nesting of data structures proved troublesome when we now want to keep track of retention time for each metric (we don't want each bucket to have it's own retention time, and each counter in each bucket have it's own retention time >_<)

fcimeson · 2020-01-22T19:20:39Z

core/src/family.cc

+        if (found) {
+          matched = true;
+        } else {
+          matched = false;
+          break;
+        }


There is sort of a bug in this logic. It should only update matched if not found.

fcimeson · 2020-01-22T19:21:47Z

core/src/family.cc

+  /* Check if the name is a match */
+  if (boost::regex_match(name_, name_expr)) {
+    for (auto metric: metrics_) {
+      bool matched(true);


There is a bit of a bug in the logic, see followup comment.

fcimeson · 2020-01-22T19:26:24Z

core/src/family.cc

+        if (found) {
+          matched = true;
+        } else {
+          matched = false;
+          break;
+        }


Suggested change

if (found) {

matched = true;

} else {

matched = false;

break;

}

if (!found) {

matched = false;

break;

}

fcimeson · 2020-01-22T19:28:35Z

core/src/gauge.cc

-  }
-  Change(value);
+void Gauge::Increment(const double& value, const bool& alert) {
+  if (value < 0.0) return;


I agree but I'm not sure that warrants changing the original behaviour or not (the more that we leave the same, the easier it will be for merge updates).

fcimeson · 2020-01-22T19:30:13Z

core/src/summary.cc

-
-void Summary::Observe(const double value) {
+                 const std::chrono::milliseconds& max_age,
+                 const int& age_buckets)


You caught me. I can change it back but I'll wait to see what the other reviewers say.

fcimeson · 2020-01-22T19:39:01Z

core/src/summary.cc

  sum_ += value;
  quantile_values_.insert(value);
+  last_update_ = std::time(nullptr);
+  if (alert) AlertIfNoFamily();


Well, clearly you wouldn't understand since your family is awesome!

Joking aside, prometheus keeps a list of metrics (families) for publishing to sockets (which eventually get added to the database). Now that we have the ability to expire metrics (remove them from their family), we want to alert the user if and when they are trying to use a metric that will not get published.

Co-Authored-By: sarahttan <sarah.t.tan@gmail.com>

sarahttan

To the best of my knowledge, I've reviewed the changes, but it's definitely a bit hard to switch review sites ;)

tt-avidbots · 2020-09-10T15:29:27Z

core/src/metric_base.cc

+bool MetricBase::HasFamily() { return has_family_; }
+
+void MetricBase::AlertIfNoFamily() {
+  if (!has_family_) std::cerr << "prometheus-cpp:: This metric has no family" << std::endl;


Could you extend the message to explain the consequence of not having a family (i.e. not publishing the metric to sockets) as you elaborated in one of your comments? It's not immediately clear why this important especially for new users.

tt-avidbots · 2020-09-10T15:35:47Z

core/include/prometheus/counter.h

  ///
  /// The counter will not change if the given amount is negative.
-  void Increment(double);
+  void Increment(const double& = 1, const bool& alert = true);


Could this alert be set during construction of the metric so the user doesn't have to add this argument if it is false every time they call the Increment methods on Counters, Gauges etc? It seems a bit cumbersome to add during each update when this needs to only be set once. I think it will also make the user more intentional about the alerts during the creation of the metrics.

tt-avidbots · 2020-09-10T15:41:29Z

core/include/prometheus/family.h


 namespace prometheus {

+enum class RetentionBehavior {Keep, Remove};


Could you add a comment to explain the significance/behaviour of the retention i.e. keeps metric for specified retention time, removes when retention time is exceeded? I wasn't sure how the removal worked and add to search for it.

tt-avidbots · 2020-09-10T15:47:26Z

core/include/prometheus/family.h

  ///
  /// \param name Set the metric name.
  /// \param help Set an additional description.
  /// \param constant_labels Assign a set of key-value pairs (= labels) to the


Add documentation for the retention_behavior param.

tt-avidbots · 2020-09-10T16:33:47Z

core/include/prometheus/family.h

  /// \return Zero or more samples for each dimensional data.
  std::vector<MetricFamily> Collect() override;

+  bool UpdateRetentionTime(const double& retention_time, const std::string& re_name, 


Do we need to make built-in types const ref? I don't see any benefit to this but if there is let me know.

bools, ints, doubles no. Strings can have benifit as the addresss space is smaller than the string.

Yea I meant the ints, doubles etc except string.

tt-avidbots · 2020-09-10T16:52:36Z

core/include/prometheus/summary.h

-          std::chrono::milliseconds max_age = std::chrono::seconds{60},
-          int age_buckets = 5);
+          const std::chrono::milliseconds& max_age = std::chrono::seconds{60},
+          const int& age_buckets = 5);


Again I don't see any optimizations/improvements to making the built-in types const ref

tt-avidbots · 2020-09-10T16:53:51Z

core/src/counter.cc

+}

-double Counter::Value() const { return gauge_.Value(); }
+double Counter::Value() const { return value_; }


I like the change you've made here with the counters etc keeping track of their values.

tt-avidbots · 2020-09-10T17:05:07Z

core/src/family.cc

+      if (!m.second->Expired()) {
+        family.metric.push_back(std::move(CollectMetric(m.first, m.second)));
+      } else if (retention_behavior_ == RetentionBehavior::Remove) {
+        to_be_removed.push_back(m.second);


I get that we don't want to hold the lock for a long time but if you're using unordered maps for the metrics and labels, then the removal should be constant time in the average case. If removal from the unordered maps approach linear time then I think a better hash function needs to be chosen. Hence, do we need to store the metrics to be removed and then remove them later?

That could be a solution. The Remove method does a few things, so I think I was just playing it safe. Additionally, the Remove method also has a lock, so that would either need to be removed (and only called when the data is locked, which is problematic) or change it to a scoped lock, which would need some thought.

I like the idea of calling it in place but I'm leaning towards leaving it since I like to play it safe with locks.

tt-avidbots · 2020-09-10T17:16:53Z

core/include/prometheus/family.h

  /// \return Zero or more samples for each dimensional data.
  std::vector<MetricFamily> Collect() override;

+  bool UpdateRetentionTime(const double& retention_time, const std::string& re_name, 


Also, add method documentation for "UpdateRetentionTime"

tt-avidbots · 2020-09-10T17:29:42Z

core/src/family.cc

+}
+
+template <typename T>
+bool Family<T>::UpdateRetentionTime(const double& retention_time, const std::string& re_name, 


Couldn't you just update the retention time for each metric internally every time the metric is updated (incremented, decremented etc.) and set a flag for removal every time the retention time is exceeded to circumvent the name/metric matching process?

Even if it remains as is, shouldn't this be a method that's called internally by the registry? Should the user explicitly have to call this function if they're set the retention behavior for a metric to remove?

The rentention time is updated automatically, these are just for the developer to keep the metric alive if they are not updating the metric with new data. As for flagging them for removal, we want to flag them peroidically, otherwise we might get a metric that gets data once during the whole program and then never expires.

The current implementation has the metrics update the class member last_update_ instead of calling UpdateRetentionTime since that method is used to find the metric or group of metrics and update their retention time.

…oved erroneous pass by references).

mfine and others added 22 commits April 19, 2019 05:14

Expired method.

b8e4f3d

Check for expired metrics.

75c1ef8

Expired seconds.

92543c9

Woops - compare against passed in seconds.

a9d46e8

Always set time on construction.

cf7e91a

Update family tests.

86d9a7a

Formatting.

3eaddd9

Woops.

3913a12

Merge pull request #1 from swift-nav/mfine/expired

3a502b6

Expired metrics

No time taking under lock.

811d47f

Merge pull request #2 from swift-nav/mfine/no-time

985d287

No time taking under lock.

Less timestamping inside of locked code.

7362fab

Merge pull request #3 from swift-nav/mfine/collectable-with-time

ab53cf8

Less timestamping inside of locked code.

CP-1664: Extended the expire logic

e444e06

CP-1664: Used return by shared_ptr instead of reference

359c8a1

Update README.md

93b1c40

CP-1664: Added an alert for expired metrics

9d4ab3a

Merge branch 'CP-1664-modify-prometheus-cpp' of https://github.com/av…

e488ab6

…idbots/prometheus-cpp into CP-1664-modify-prometheus-cpp

Update README.md

4a187e3

CP-1664: Fixed bug in the retention behavior

02bfdb0

CP-1664: Fixed collect/remove bug

cf6a376

fcimeson commented Dec 19, 2019

View reviewed changes

sarahttan suggested changes Jan 22, 2020

View reviewed changes

fcimeson commented Jan 22, 2020

View reviewed changes

fcimeson and others added 2 commits January 22, 2020 14:43

Apply suggestions from code review

ef1bdc1

Co-Authored-By: sarahttan <sarah.t.tan@gmail.com>

CP-1664: PR changes

ef31ccb

sarahttan approved these changes Jan 28, 2020

View reviewed changes

tt-avidbots reviewed Sep 10, 2020

View reviewed changes

CP-1664: PR changes (changed how we update alert_if_no_family and rem…

2f58238

…oved erroneous pass by references).

CP-1664: More PR changes that I missed in the previous commit.

880e4a0

sarahttan approved these changes Sep 11, 2020

View reviewed changes

tt-avidbots approved these changes Sep 14, 2020

View reviewed changes

fcimeson merged commit 198dc51 into master Sep 14, 2020

fcimeson deleted the CP-1664-modify-prometheus-cpp branch September 29, 2020 14:19


		namespace prometheus {

		enum class RetentionBehavior {Keep, Remove};

CP-1664: modify prometheus-cpp #1

CP-1664: modify prometheus-cpp #1

Uh oh!

Conversation

fcimeson commented Dec 19, 2019

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!

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!

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!

sarahttan 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