Ap 547: Add other healthchecks to Geodata #71
Conversation
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
I think this is a good start but I'd like to see this check be closer in alignment to how OkComputer::Check defines its API. I have some questions about whether some of the classes and methods are necessary. Let me know if you'd like to discuss this further!
There was a problem hiding this comment.
Why is this class necessary? I'm not sure why we can't just register checks for four URLs directly in the OkComputer initializer.
There was a problem hiding this comment.
Do not use the full urls here. This allows us to check other Geoserver endpoints without modifying the Docker Swarm stack file. If we don't this is necessary, we can put the 4 full urls.
| server_name = type == 'public' ? GEOSERVER_NAME : SECURE_GEOSERVER_NAME | ||
| url = host_url(server_name) | ||
| geoserver_url = url && "#{url.chomp('/')}/wms?service=WMS&request=GetCapabilities" | ||
| OkComputer::Registry.register clr_msg(server_name), GeoDataHealthCheck::HttpHeadCheck.new(geoserver_url) |
There was a problem hiding this comment.
Adding the checks in the registry should only happen in the initializer.
There was a problem hiding this comment.
The method where the registry check line located is called in initializer/okcomputer.rb
| msg = "#{type} #{SPATIAL_SERVER_NAME}" | ||
| url = host_url(SPATIAL_SERVER_NAME) | ||
| spatial_url = url && "#{url.chomp('/')}/#{type}/berkeley-status/data.zip" | ||
| OkComputer::Registry.register clr_msg(msg), GeoDataHealthCheck::HttpHeadCheck.new(spatial_url) |
There was a problem hiding this comment.
Adding the checks in the registry should only happen in the initializer.
|
|
||
| def build_request(method, uri) | ||
| req_method = method.downcase.to_sym | ||
| raise ConnectionFailed, "Incorrect http request method: #{method}" unless %i[head get].include?(req_method) |
There was a problem hiding this comment.
Why is GET an allowed method here? We need a custom module/health check specifically for HEAD requests.
There was a problem hiding this comment.
Originally, I planned to issue a Get request if the initial HEAD request returned a redirect, so I added it here. Since we don't need it, I will change it
| def skip_check | ||
| mark_failure | ||
| mark_message 'No URL configured; health check was skipped...' | ||
| end |
There was a problem hiding this comment.
Why is this method necessary? Shouldn't the rescue on line 27 catch this if url is not defined?
There was a problem hiding this comment.
Originally, I thought this would improve the output of the health/check page, but it may not be a good idea here.
| require 'openssl' | ||
|
|
||
| module GeoDataHealthCheck | ||
| class HttpClient |
There was a problem hiding this comment.
Does this need to be its own class? (See related comment below about method naming/location.)
There was a problem hiding this comment.
If we do not use HTTP GET, I will make it simpler
| ConnectionFailed = Class.new(StandardError) | ||
|
|
||
| class << self | ||
| def request(method, url, timeout: 5) |
There was a problem hiding this comment.
What would you think about renaming this method to be perform_request and moving it into HttpHeadCheck to follow the same pattern that OkComputer::HttpCheck uses?
There was a problem hiding this comment.
Yes, to inherited from OkComputer::HttpCheck
| @@ -0,0 +1,39 @@ | |||
| module GeoDataHealthCheck | |||
| class HttpHeadCheck < OkComputer::Check | |||
There was a problem hiding this comment.
Is there any advantage to inheriting from OkComputer::HttpCheck instead? You'd then have access to OkComputer::HttpCheck::ConnectionFailed and perhaps could simplify the logic in your #check method below.
There was a problem hiding this comment.
It sounds good to inherited from HttpCheck. Thanks
Add http head check to geoservers and spatial servers