-
-
Notifications
You must be signed in to change notification settings - Fork 26
Drop Prometheus middleware from dramatiq settings #148
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
base: main
Are you sure you want to change the base?
Conversation
Prometheus client became optional in dramatiq v2: https://github.com/Bogdanp/dramatiq/releases/tag/v2.0.0 I think we don't need to cover all our dependencies's subdependencies, so it is fine to drop it.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
===========================================
- Coverage 97.76% 81.98% -15.78%
===========================================
Files 17 17
Lines 805 805
===========================================
- Hits 787 660 -127
- Misses 18 145 +127 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR removes the Prometheus middleware from the dramatiq broker configuration in the test application settings, aligning with dramatiq v2 where the Prometheus client became optional.
- Removes
"dramatiq.middleware.Prometheus"from the MIDDLEWARE list in DRAMATIQ_BROKER configuration
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codingjoe I couldn't figure this out from the first look. Broker is now raising exceptions immediately when tasks retry. |
codingjoe
left a comment
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.
Thanks, @amureki, for thinking about me and contributing this change <3
|
@amureki dramatic tests fail though 🙈 |
yeah, as I mentioned above, I am not sure how could we proceed here - need to evaluate this more. I'd need to invest more time into it, but also if you have suggestions, I'd be happy to hear! |
Prometheus client became optional in dramatiq v2:
https://github.com/Bogdanp/dramatiq/releases/tag/v2.0.0
I think we don't need to cover all our dependencies's subdependencies, so it is fine to drop it.