-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update SidekiqRedis healthcheck to work with Sidekiq 7 #399
Conversation
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.
Great stuff, just a couple of minor things.
@@ -5,7 +5,11 @@ def name | |||
end | |||
|
|||
def status | |||
Sidekiq.redis_info ? OK : CRITICAL | |||
if Sidekiq.respond_to?(:redis_info) |
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.
if Sidekiq.respond_to?(:redis_info) | |
if Sidekiq.respond_to?(:default_configuration) |
Shall we try do the new way first rather than using it as the fallback? Since that is the way we want to be the normal.
I think it'd be also beneficial to have a comment to state that the need for the conditional is to support both Sidekiq >= 7 and Sidekiq < 7 - so that it can be easier for someone to know when this is old.
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.
yea makes sense.
before { stub_const("Sidekiq", sidekiq) } | ||
|
||
context "when the database is connected" do | ||
context "when the Sidekiq version is >= 7.0.0" do |
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 think this (and the alternate version) should probably explicitly reference default_configuration
rather than the Sidekiq version - as the test doesn't actually reference that and you have to implicitly understand that setting up default_configuration alludes to it being Sidekiq 7 or not.
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.
cool have updated it and added a comment to the healthcheck above the conditional explaining the issue.
This healthcheck has been failing for applications that have upgraded to Sidekiq 7. Sidekiq 7 uses 'redis-client' which has a different API to 'redis' which was used in Sidekiq 6. To access redis_info you now have to call ``` Sidekiq.default_configration.redis_info ``` while in Sidekiq 6 you call ``` Sidekiq.redis_info ``` This updates the healthcheck to work with both versions of Sidekiq checking if Sidekiq.responds to :redis_info. If it does it calls that, otherwise it calls Sidekiq.default_configration.redis_info. It's not missively clear these healthchecks are actually being used for anything since this has been failing on production applications for a couple of weeks without anyone noticing. We're in the process of speaking to the platform team to see if they're actually being used for anything and if not whether they should be removed. But for now i'm just updating it to work with Sidekiq 7.
09c89b2
to
d6f52f6
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.
Nice one 👍
This healthcheck has been failing for applications that have upgraded to Sidekiq 7. Sidekiq 7 uses
redis-client
' which has a different API toredis
which was used in Sidekiq 6.To access
redis_info
you now have to callwhile in Sidekiq 6 you call
This updates the healthcheck to work with both versions of Sidekiq checking if
Sidekiq responds to
.redis_info`.If it does it calls that, otherwise it calls
Sidekiq.default_configration.redis_info
.It's not massively clear these healthchecks are actually being used for anything since this has been failing on production applications for a couple of weeks without anyone noticing.
We're in the process of speaking to the platform team to see if they're actually being used for anything and if not whether they should be removed.
But for now i'm just updating it to work with Sidekiq 7.
Refer to the existing docs if you are making changes to the supported Ruby versions.