-
-
Notifications
You must be signed in to change notification settings - Fork 291
export RQ statistics as prometheus metrics #666
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
Conversation
288bbf5
to
30e3eb4
Compare
79f6394
to
70f07f8
Compare
de7de29
to
84c84bf
Compare
I don't mind merging this in, but could you document this feature? A simple unit test to show that this view works would also be great. |
84c84bf
to
8066be6
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 don't mind merging this in, but could you document this feature? A simple unit test to show that this view works would also be great.
Sorry for the delay getting this updated, but here it is. The tests took a bit of finagling to get working, but I hope this is acceptable (running tests twice).
You can let me know if you have a strong preference about the metric names, but I reviewed what I put and adjusted them to match the Prometheus naming conventions (they were close but could use some updating), and I factored in the comments you made.
3dda38a
to
0463893
Compare
It looks like the test failures are related to the fix I added in #700 |
@terencehonles sorry I released version 3.0.1 in a rush because Django 5.2 is out and it's breaking admin pages in various projects. Do you mind pulling in master to this PR? I'll make a follow up release for this feature. |
0463893
to
10192f5
Compare
No worries, I understand. I rebased just now |
It looks like #704 broke the workflow file, I can possibly look at it tomorrow, but will be offline for the night |
10192f5
to
de17a2d
Compare
Looks like it was actually a bad merge on git's part since the indentation of the workflow file changed and the line it was reporting wan't the line that broke the indentation it was the container of that line. |
39c9999
to
5ebc28c
Compare
The test failures appear to be because of redis/redis-py#3614 / redis/redis-py#3622 and I fixed with #706, I had to update my test environment locally in order to reproduce the error. |
@terencehonles just a reminder that test cases for this PR is still failing. I have plans to improve django RQ after I land RQ's cron functionality so if possible, I'd like to get this merged in before then :). |
ffe0b1e
to
52e359c
Compare
52e359c
to
4e93dfd
Compare
4e93dfd
to
44ca5b2
Compare
Thanks, was just waiting on #706 to rebase, but it looks like I had a typo in the PR redis/redis-py#3628 for the class redis uses as the default backoff class 🤐. I fixed that in redis/redis-py#3668, but since there's already been a few redis releases I just added back a slightly different variant of the work-around code with 032fe34. If you'd rather have that as its own PR I can do that. |
django_rq/stats_views.py
Outdated
global registry | ||
|
||
if not RQCollector: # type: ignore[truthy-function] | ||
raise Http404 |
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.
Can we show an error message asking users to install the prometheus extras to enable this feature? If Http404 can't easily return messages, raise ImproperlyConfigured
is also ok.
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.
done, but should I not conditionally add the route then? https://github.com/rq/django-rq/pull/666/files#diff-a192d912d8bfaefa1f6acb14c8ae8a9ab8e421002139cbb8abd55ff33b3dce77R6-R8
568cc9c
to
f2346fc
Compare
f2346fc
to
af477bd
Compare
Anything else @selwin ? |
Thanks @terencehonles ! Will cut a new release sometime in the next few days. |
This change follows the implementation of
rq_exporter
and the prometheus client to export the RQ statistics as a Django view.fixes: #503