-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump yabeda-prometheus from 0.1.4 to 0.1.5 #283
Conversation
90fa83f
to
472869a
Compare
472869a
to
2b56fda
Compare
3dcd870
to
cf9bbbb
Compare
0710fb5
to
72bcf1a
Compare
72bcf1a
to
62f01b2
Compare
b3dc46b
to
8f94a15
Compare
726bb5c
to
dc4abe4
Compare
7a17052
to
d6f1b72
Compare
14ad65d
to
e247aa0
Compare
ea50828
to
43a858c
Compare
43a858c
to
d93d1c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the test failing...
This is because of a change in the Yabeda Ruby gem: https://github.com/yabeda-rb/yabeda/compare/v0.1.3..v0.8.0. Particularly, the way Yabeda.configure
works.
TL;DR
There is a now a new method Yabeda.configure!
(with the bang) that needs to be called once (and only once) after all "configurators" have been called.
The long story
Yabeda.configure
can be still called multiple times. It is usually – always, actually, in zync's case – called when the classes, modules, etc are loaded. When the ActiveJobSubscriber
class is loaded, Yabeda.configure
is called once, for a group named que
. In the old version of https://github.com/yabeda-rb/yabeda (currently in zync's master branch), whenever Yabeda.configure
was invoked with a group, the gem "registered" this group name in its internal cache of groups of metrics.
This was of course also the case of when ActiveJobSubscriber
invoked Yabeda.configure
– the gem then registered a group called que
and also defined a singleton class method Yabeda.que
. Later on, when the application was already running and some event created a job, an instance of ActiveJobSubscriber
was initiated, which called Yabeda.que
. The method exists because it was "registered" in the gem.
The new version of the yabeda gem now demands an invocation of another method to effectively register groups and metrics. This new method is the Yabeda.configure!
method (with a bang). The new recommended usage is: first, store all "configurators" by calling Yabeda.configure
with a block as many times as you want, then call Yabeda.configure!
once and only once. It is only after calling this new method that the gem will really register the groups and metrics and therefore also create any corresponding singleton class methods (e.g. Yabeda.que
).
In this PR, since we aren't invoking Yabeda.configure!
anywhere, the groups and metrics haven't been registered. Possibly, by just adding Yabeda.configure!
to https://github.com/3scale/zync/blob/master/config/initializers/prometheus.rb, the problem will be fixed, but we must be careful, because there's also another process that we start in OpenShift that configures Yabeda groups and metrics, i.e. the zync-que process. This process requires another ruby file, https://github.com/3scale/zync/blob/master/lib/que/prometheus.rb. However, if we invoke Yabeda.configure!
from this file as well, it will not work, because, as I mentioned, this method can be called only once.
In the end, we may need to move some code, perhaps rearrange the order Yabeda.configure
is called the multiple times and then ensure Yabeda.configure!
is called, once and only once, for both processes, i.e., the Rails server and the zync que process.
@guicassolato Yes I am working at it Registering then configuring is the way to go, otherwise you could override some configurations ... Nothing wrong in Yabeda fixing it. The issue lies on
# File activesupport/lib/active_support/subscriber.rb, line 30
def attach_to(namespace, subscriber = new, notifier = ActiveSupport::Notifications)
@namespace = namespace
@subscriber = subscriber
@notifier = notifier
subscribers << subscriber
# Add event subscribers for all existing methods on the class.
subscriber.public_methods(false).each do |event|
add_event_subscriber(event)
end
end We must do that after The way our code is organized is a bit disorganized :) Commit coming soon but the idea is to use https://guides.rubyonrails.org/configuring.html#initialization-events |
Signed-off-by: HemaHG <hhg@redhat.com>
8406341
to
21aac65
Compare
waiting = gauge :waiting, comment: 'Number of waiting in the queue of the connection pool' | ||
|
||
no_labels = {}.freeze | ||
no_labels = { state: nil }.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because you can't set different labels (empty and other with :state
) to the same gauge
A gauge needs to consistently set the same labels
size = gauge :size, comment: 'Size of the connection pool' | ||
connections = gauge :connections, comment: 'Number of connections in the connection pool' | ||
waiting = gauge :waiting, comment: 'Number of waiting in the queue of the connection pool' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moved the definition of gauges below the labels
@@ -30,7 +30,7 @@ def execute | |||
|
|||
class WorkerStats < Stats | |||
WORKER_STATS = <<~SQL | |||
SELECT SUM(worker_count) AS workers_count, COUNT(*) AS nodes_count FROM que_lockers | |||
SELECT COALESCE(SUM("worker_count"),0) AS workers_count, COUNT(*) AS nodes_count FROM que_lockers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no record or the worker_count
column is NULL
then SUM
will return nil
The gem update validates the value is an number
@@ -101,7 +101,7 @@ def expired | |||
JOB_CLASSES = %w[ApplicationJob ProcessEntryJob ProcessIntegrationEntryJob UpdateJob].inspect.tr('"', "'").freeze | |||
private_constant :JOB_CLASSES | |||
|
|||
JOB_CLASS_COLUMN_NAME = 'job' | |||
JOB_CLASS_COLUMN_NAME = 'job_name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
job
is a reserved label.
It was not validated before this update
@@ -136,7 +136,7 @@ def call(type = nil) | |||
protected | |||
|
|||
def type_hash_and_stats_count(type) | |||
type_hash = type ? { type: type } : {} | |||
type_hash = { type: type } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sending an empty hash, same as https://github.com/3scale/zync/pull/283/files#diff-f903aea79c99fca32c1fafbb2b9e0cd134020edf41088b78f090b2a00faf7978R5
@@ -153,7 +153,7 @@ def initialize(gauge, stats, grouped_by:) | |||
def call(type = nil) | |||
type_hash, grouped_stats_count = type_hash_and_stats_count(type) | |||
grouped_stats_count.each do |stats_count| | |||
gauge.set({ grouped_by => stats_count.fetch(grouped_by) }.merge(type_hash), stats_count.fetch('count')) | |||
gauge.set({ grouped_by.to_sym => stats_count.fetch(grouped_by) }.merge(type_hash), stats_count.fetch('count')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
labels must be Symbol
@@ -173,7 +173,7 @@ def call(type = nil) | |||
group :que do | |||
jobs = gauge :jobs_scheduled_total, comment: 'Que Jobs to be executed' | |||
job_stats = Prometheus::QueStats::JobStats.new | |||
collector = Prometheus::QueStats::GroupedStatsCollector.new(jobs, job_stats, grouped_by: 'job') | |||
collector = Prometheus::QueStats::GroupedStatsCollector.new(jobs, job_stats, grouped_by: 'job_name') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
job
is a reserved label
@@ -1,2 +1,5 @@ | |||
require 'prometheus/active_job_subscriber' | |||
require 'prometheus/active_record' | |||
|
|||
Yabeda.configure! if Yabeda.respond_to?(:configure!) | |||
Prometheus::ActiveJobSubscriber.attach_to :active_job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be called after the Yabeda.configure!
otherwise it will instanciate a Prometheus::ActiveJobSubscriber
instance in the class definition which is not possible because it uses a not yet defined Yabeda.que
method
* COALESCE a sum for que workers * Rename `job` label to `job_name` * Call `Yabeda.configure!`` after all definition * `attach_to :active_job` subscriber after Yabeda.configure! otherwise `Yabeda.que` is not defined
21aac65
to
7706422
Compare
Bumps yabeda-prometheus from 0.1.4 to 0.1.5.
Changelog
Sourced from yabeda-prometheus's changelog.
Commits
3ff352f
Release v0.1.5 with fix for counters with prometheus-client-mmapff28135
Clean up unused code after changes in #8a702bf5
Fix for counter metricts with prometheus-client-mmap (#8)Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)@dependabot use these labels
will set the current labels as the default for future PRs for this repo and language@dependabot use these reviewers
will set the current reviewers as the default for future PRs for this repo and language@dependabot use these assignees
will set the current assignees as the default for future PRs for this repo and language@dependabot use this milestone
will set the current milestone as the default for future PRs for this repo and language@dependabot badge me
will comment on this PR with code to add a "Dependabot enabled" badge to your readmeAdditionally, you can set the following in your Dependabot dashboard: