Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/health/health_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class HealthController < AbstractHealthController
def health_checker
checkers = {
database: DatabaseHealthChecker,
redis_session: RedisSessionHealthChecker,
account_reset: AccountResetHealthChecker,
}
MultiHealthChecker.new(**checkers)
Expand Down
9 changes: 9 additions & 0 deletions app/controllers/health/redis_session_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module Health
class RedisSessionController < AbstractHealthController
private

def health_checker
RedisSessionHealthChecker
end
end
end
30 changes: 30 additions & 0 deletions app/services/redis_session_health_checker.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
module RedisSessionHealthChecker
module_function

# @return [HealthCheckSummary]
def check
HealthCheckSummary.new(healthy: health_write_and_read.present?, result: health_write_and_read)
rescue StandardError => err
NewRelic::Agent.notice_error(err)
HealthCheckSummary.new(healthy: false, result: err.message)
end

# @api private
def health_write_and_read
REDIS_POOL.with do |client|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but would we want to check all the Redis pools?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could at least add a check for the rate limiting Redis. Attempts storage is not always enabled so seems like a conditional health check would be needed for that.

Should we combine it all here in one spot and call this RedisHealthChecker?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense, yeah.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess they could be separate too, I don't have strong feelings, whatever is easier.

client.setex(health_record_key, health_record_ttl, "healthy at " + Time.now.iso8601)
client.get(health_record_key)
end
end

# @api private
def health_record_key
"healthcheck_" + Socket.gethostname
end

# @api private
def health_record_ttl
# If we can't read back within a second that is just unacceptable
1
end
end
32 changes: 32 additions & 0 deletions spec/services/redis_session_health_checker_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
require 'rails_helper'

RSpec.describe RedisSessionHealthChecker do
describe '.check' do
subject(:summary) { RedisSessionHealthChecker.check }

context 'when the redis session store is healthy' do
it 'returns a healthy check' do
expect(summary.healthy).to eq(true)
expect(summary.result).to start_with("healthy at ")
end
end

context 'when the redis session store is unhealthy' do
before do
expect(RedisSessionHealthChecker).to receive(:simple_query).
and_raise(RuntimeError.new('canceling statement due to statement timeout'))
end

it 'returns an unhealthy check' do
expect(summary.healthy).to eq(false)
expect(summary.result).to include('canceling statement due to statement timeout')
end

it 'notifies NewRelic' do
expect(NewRelic::Agent).to receive(:notice_error)

summary
end
end
end
end