replace statsd with prometheus client #6897
Conversation
|
I don't personally use either statsd nor Prometheus, so I don't have strong feelings with way. 😄 @getredash/maintainers What do you reckon? |
cannot use this statsd library with victoriametrics. it has hardcoded colon as a separator, which is not configurable and looks like library is not well maintained |
AndrewChubatiuk
force-pushed
the
prometheus-endpoint
branch
from
37397fb
to
1472624
Compare
AndrewChubatiuk
force-pushed
the
prometheus-endpoint
branch
from
d8e311a
to
b3f094d
Compare
lucasfcnunes
changed the title
AndrewChubatiuk
force-pushed
the
prometheus-endpoint
branch
4 times, most recently
from
2881606
to
ec4b4cc
Compare
|
Thank you for refactoring Job serializer and Athena config as well! This is a huge improvement, would love to see this merged! cc @justinclift |
|
Uh oh, this is a very large PR. 😦 Can it be broken up into smaller pieces that each individually work by themselves (aka not break things)? That would make the review process be far more likely to go ok. 😉 |
I agree, it's difficult to give something this large a proper review to as-is. Even if everything in this PR is related to this feature (I'm not sure?), it looks at a glance like some changes (e.g. the frontend changes) could be landed seperately. There's a number of tools that can be used to support a stacking workflow online: https://stacking.dev/stacking-site |
AndrewChubatiuk
force-pushed
the
prometheus-endpoint
branch
from
745b0d0
to
d4f8b20
Compare
|
@AndrewChubatiuk Cool, that seems like a good start. 😄 |
AndrewChubatiuk
force-pushed
the
prometheus-endpoint
branch
from
d4f8b20
to
1c4d47b
Compare
AndrewChubatiuk
force-pushed
the
prometheus-endpoint
branch
from
1c4d47b
to
b06991b
Compare
There was a problem hiding this comment.
I took a pass at this. This mostly looks reasonable to me, except for maybe adding a gotcha about statsd no longer being unconditionally enabled -- do we need to update any documentation or release notes?
I'm a little hestitant to hit the approve button just because I'm not super familiar with statsd or prometheus in the context of redash. I'd rather leave that to @eradman or someone else who has more domain context.
| @@ -22,9 +22,11 @@ | |||
| REDIS_URL = add_decode_responses_to_redis_url(_REDIS_URL) | |||
| PROXIES_COUNT = int(os.environ.get("REDASH_PROXIES_COUNT", "1")) | |||
|
|
|||
| STATSD_ENABLED = parse_boolean(os.environ.get("REDASH_STATSD_ENABLED", "false")) | |||
There was a problem hiding this comment.
It looks like STATSD was always unconditionally enabled before? Do we need to update the documentation?
There was a problem hiding this comment.
it makes no sense to enable statsd by default if it has no remote endpoint configured
There was a problem hiding this comment.
statsd uses push model, and prometheus - pull. for statsd you should specify remote url to push metrics to and prometheus exposes metrics at configured endpoint
| self.factory.create_query() | ||
| timing.assert_called_with("db.changes.insert", ANY) | ||
| observe.assert_called_with(ANY) |
There was a problem hiding this comment.
Feels like we're not testing as much as we were before here?
There was a problem hiding this comment.
interface has changed and observe function calculates histograms and expect time at the input. we cannot predict the time it takes to run a function, so ANY value was set here. regarding metric name, it should not be passed anymore as it's initialized in a different manner from statsd python client
What type of PR is this?
currently there's only one option to expose metrics - statsd client, which doesn't support custom metric name -> value separator
added metrics endpoint and ability to enable statsd output using prometheus-client
Description
How is this tested?
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)