-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
replace statsd with prometheus client #6897
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 |
37397fb
to
1472624
Compare
d8e311a
to
b3f094d
Compare
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 |
745b0d0
to
d4f8b20
Compare
@AndrewChubatiuk Cool, that seems like a good start. 😄 |
d4f8b20
to
1c4d47b
Compare
1c4d47b
to
b06991b
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like STATSD
was always unconditionally enabled before? Do we need to update the documentation?
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.
it makes no sense to enable statsd by default if it has no remote endpoint configured
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.
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
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.
Got it, makes sense!
self.factory.create_query() | ||
timing.assert_called_with("db.changes.insert", ANY) | ||
observe.assert_called_with(ANY) |
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.
Feels like we're not testing as much as we were before here?
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.
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)