Modernize common/check_messages_to_submit#144
Conversation
dchristidis
left a comment
dchristidis
left a comment
There was a problem hiding this comment.
I would recommend waiting for the completion of #133 because I expect my comments will apply to this PR as well.
voetberg
force-pushed
the
129-modern-messages-to-submit
branch
2 times, most recently
from
41eca34 to
ae9d997
Compare
dchristidis
left a comment
dchristidis
left a comment
There was a problem hiding this comment.
Also reminder about the commit message subject.
voetberg
force-pushed
the
129-modern-messages-to-submit
branch
2 times, most recently
from
aa11b84 to
5da363c
Compare
* Add prom pusher
* update to sqla2.0
* sort imports
* update header
* change except to except Exception
voetberg
force-pushed
the
129-modern-messages-to-submit
branch
from
5da363c to
2517d25
Compare
dchristidis
left a comment
dchristidis
left a comment
There was a problem hiding this comment.
Excellent. I can merge as soon as we get @ericvaandering’s approval.
common/check_messages_to_submit
Outdated
|
|
||
| with PrometheusPusher() as manager: | ||
| (manager.gauge( | ||
| "messages_to_submit.queues.messages", |
There was a problem hiding this comment.
Requires further review.
There was a problem hiding this comment.
This changes the metric value, but is fine for CMS. I think the long-discussed goal is to group things appropriately.
There was a problem hiding this comment.
Changing the metric name can be acceptable. The problem is that the new metric name is less discoverable than before.
This (and the other two PRs) populate a very inherent metric of the respective daemons: their backlog. So, it’s unconventional to chose a metric name that doesn’t even reference the daemon.
And if the name is changed, then the commit message should very clearly state so.
There was a problem hiding this comment.
OK. @voetberg let me know if you need help coming up with a better metric name.
There was a problem hiding this comment.
Okay, I was populating the new name based off the job name from the gateway push not the gauge itself. I don't see a problem including the name of the daemon here.
There was a problem hiding this comment.
Please allow me to give it a bit more thought and get back to you. Once this small kink is ironed out, all three PRs can be merged and we can move on to the remaining probes.
common/check_messages_to_submit
Outdated
|
|
||
| with PrometheusPusher() as manager: | ||
| (manager.gauge( | ||
| "messages_to_submit.queues.messages", |
There was a problem hiding this comment.
This changes the metric value, but is fine for CMS. I think the long-discussed goal is to group things appropriately.
hermes_queues_messages -> hermes.queues.messages
|
@voetberg is the intention here to change the metric value back to the original? |
|
@ericvaandering It was changed back but replacing '_' with '.' for grouping, I'm just waiting for @dchristidis' feedback |
|
Closing in favor of #164 |
3 participants
Add prom pusher, update query to sqla2.0, sort imports and update header