Fixes #28506 - fix upgraded prometheus

[ezr-ondrej]

Someone forgot to check the changelog before theforeman/foreman-packaging#4439 👀

[theforeman-bot]

Issues: #28506

[ezr-ondrej]

@lzap should we pass the instance_labels? Prometheus client seems to have something called pretty similarily, but I have no clue if there is a reason we don't pass them on?

[ekohl]

Thanks for figuring this out. Was on the list of things I wanted to know. Given this was found in puppet-foreman by actually deploying, it appears there are no tests to cover this.

[ezr-ondrej]

Yeah it appears so, it would be good to add them, to know it sooner, but I'm mostly AFK this week, so I'll not be able to write them. Dne so 14. 12. 2019 14:02 uživatel Ewoud Kohl van Wijngaarden < notifications@github.com> napsal:

…

Thanks for figuring this out. Was on the list of things I wanted to know. Given this was found in puppet-foreman by actually deploying, it appears there are no tests to cover this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7249?email_source=notifications&email_token=AAWAFZBJ6I3ZOPNEPZGRJJTQYTKMNA5CNFSM4J2XQOR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4CATY#issuecomment-565715023>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWAFZF76THDOWH4PU6TGQTQYTKMNANCNFSM4J2XQORQ> .

[lzap]

Yeah thanks. Please incorporate the following:

diff --git a/config/initializers/5_telemetry.rb b/config/initializers/5_telemetry.rb
index 22570a6c0..eb2f4cc11 100644
--- a/config/initializers/5_telemetry.rb
+++ b/config/initializers/5_telemetry.rb
@@ -1,3 +1,11 @@
+# Set multiprocess-friendly data store
+require 'prometheus/client'
+require 'prometheus/client/data_stores/direct_file_store'
+PROMETHEUS_STORE_DIR = File.join(Rails.root, 'tmp', 'prometheus')
+Dir.mkdir(PROMETHEUS_STORE_DIR) unless Dir.exist?(PROMETHEUS_STORE_DIR)
+Prometheus::Client.config.data_store =
+  Prometheus::Client::DataStores::DirectFileStore.new(dir: PROMETHEUS_STORE_DIR)
+
 # Foreman telemetry global setup.
 telemetry = Foreman::Telemetry.instance
 if SETTINGS[:telemetry] && (Rails.env.production? || Rails.env.development?)
diff --git a/config/initializers/5_telemetry_metrics.rb b/config/initializers/5_telemetry_metrics.rb
index c4c4d0c35..2d5f56361 100644
--- a/config/initializers/5_telemetry_metrics.rb
+++ b/config/initializers/5_telemetry_metrics.rb
@@ -7,7 +7,7 @@
 # Plugins can add own metrics through add_counter_telemetry, add_gauge_telemetry and add_histogram_telemetry.
 #
 telemetry = Foreman::Telemetry.instance
-telemetry.add_counter(:http_requests, 'A counter of HTTP requests made', [:controller, :action])
+telemetry.add_counter(:http_requests, 'A counter of HTTP requests made', [:controller, :action, :status])
 telemetry.add_histogram(:http_request_total_duration, 'Total duration of controller action', [:controller, :action])
 telemetry.add_histogram(:http_request_db_duration, 'Time spent in database for a request', [:controller, :action])
 telemetry.add_histogram(:http_request_view_duration, 'Time spent in view for a request', [:controller, :action])
diff --git a/lib/foreman/telemetry.rb b/lib/foreman/telemetry.rb
index 7efa9b337..2d80d50c8 100644
--- a/lib/foreman/telemetry.rb
+++ b/lib/foreman/telemetry.rb
@@ -7,7 +7,7 @@ module Foreman
     extend Forwardable
     attr_accessor :prefix, :sinks
 
-    DEFAULT_BUCKETS = [0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10].freeze
+    DEFAULT_BUCKETS = [1, 5, 20, 50, 100, 250, 500, 2000].freeze
 
     def initialize
       @sinks = []
diff --git a/lib/foreman/telemetry_sinks/prometheus_sink.rb b/lib/foreman/telemetry_sinks/prometheus_sink.rb
index 2d804d31c..855957921 100644
--- a/lib/foreman/telemetry_sinks/prometheus_sink.rb
+++ b/lib/foreman/telemetry_sinks/prometheus_sink.rb
@@ -6,27 +6,27 @@ module Foreman::TelemetrySinks
     end
 
     def add_counter(name, description, instance_labels)
-      @prom.counter(name.to_sym, docstring: description)
+      @prom.counter(name.to_sym, docstring: description, labels: instance_labels)
     end
 
     def add_gauge(name, description, instance_labels)
-      @prom.gauge(name.to_sym, docstring: description)
+      @prom.gauge(name.to_sym, docstring: description, labels: instance_labels)
     end
 
     def add_histogram(name, description, instance_labels, buckets)
-      @prom.histogram(name.to_sym, docstring: description, buckets: buckets)
+      @prom.histogram(name.to_sym, docstring: description, buckets: buckets, labels: instance_labels)
     end
 
     def increment_counter(name, value, tags)
-      @prom.get(name.to_sym).increment(tags, value)
+      @prom.get(name.to_sym).increment(by: value, labels: tags)
     end
 
     def set_gauge(name, value, tags)
-      @prom.get(name.to_sym).set(tags, value)
+      @prom.get(name.to_sym).set(value, labels: tags)
     end
 
     def observe_histogram(name, value, tags)
-      @prom.get(name.to_sym).observe(tags, value)
+      @prom.get(name.to_sym).observe(value, labels: tags)
     end
   end
 end

[ekohl]

@lzap mind submitting a PR with that so we can merge it? @ezr-ondrej said he's AFK most of the week.

[ezr-ondrej]

@lzap I've done it, but I don't feel really comfortable about it as I know very little about prometheus 👀 if you would like to take over and make it better, it would be appreciated.

[ekohl]

This causes a failure:

Errno::ENOENT: No such file or directory @ dir_s_mkdir - /home/jenkins/workspace/test_develop_pr_core@2/database/postgresql/ruby/2.6/slave/fast/tmp/prometheus

Suggested change

	Dir.mkdir(PROMETHEUS_STORE_DIR) unless Dir.exist?(PROMETHEUS_STORE_DIR)
	FileUtils.mkdir_p(PROMETHEUS_STORE_DIR)

Or do it manually like in the secret token handling:

foreman/config/initializers/secret_token.rb

Lines 5 to 6 in 739ed80

	tmp = Rails.root.join("tmp")
	Dir.mkdir(tmp) unless File.exist? tmp

[lzap]

Well I would but as you have found out it currently does not work and you can't make it worse! :-) So if you have time, please incorporate Ewoud comment and let's fix this.

I've included one important change - it's now multi-process friendly. That was actually the feature we've been waiting for.

[ekohl]

Added a commit that uses mkdir_p to test it in CI. Should be squashed on merge.

[lzap]

Katello failure irrelevant.

Solved

[ekohl]

Doesn't this break all installations where the telemetry group is unavailable?

[lzap]

#7271

[lzap]

#7271

[tbrisker]

1.24 - 49dd832
please open a PR to update packaging in rpm/stable.