Ap 547: Add other healthchecks to Geodata #71

yzhoubk · 2026-01-15T00:47:59Z

Add http head check to geoservers and spatial servers

anarchivist

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!

anarchivist · 2026-01-15T17:42:50Z

lib/check_server.rb

Why is this class necessary? I'm not sure why we can't just register checks for four URLs directly in the OkComputer initializer.

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.

I think my question is why can't we do something like the following in the OkComputer initializer:

OkComputer::Registry.register 'geoserver-public', GeodataHealthCheck::HttpHeadCheck.new(config.x.servers[:geoserver]) OkComputer::Registry.register 'geoserver-secure', GeodataHealthCheck::HttpHeadCheck.new(config.x.servers[:secure_geoserver]) # et cetera

It might be possible that we need additional helper methods to build out the URL for the GetCapabilities method.

oh, you'd like all the registry lines in the same file. I'll update it accordingly. Thanks

anarchivist · 2026-01-15T17:44:04Z

lib/check_server.rb

+      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)


Adding the checks in the registry should only happen in the initializer.

The method where the registry check line located is called in initializer/okcomputer.rb

I'm talking specifically about invoking the method OkComputer::Registry.register. Why are we invoking this method in this part of the code?

anarchivist · 2026-01-15T17:44:11Z

lib/check_server.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)


Adding the checks in the registry should only happen in the initializer.

anarchivist · 2026-01-15T17:45:03Z

lib/http_client.rb

+
+      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)


Why is GET an allowed method here? We need a custom module/health check specifically for HEAD requests.

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

anarchivist · 2026-01-15T17:52:05Z

lib/http_head_check.rb

+    def skip_check
+      mark_failure
+      mark_message 'No URL configured; health check was skipped...'
+    end


Why is this method necessary? Shouldn't the rescue on line 27 catch this if url is not defined?

Originally, I thought this would improve the output of the health/check page, but it may not be a good idea here.

I think it's not necessary - the rescue pattern will catch if the url is not set.

anarchivist · 2026-01-15T17:54:03Z

lib/http_client.rb

+require 'openssl'
+
+module GeoDataHealthCheck
+  class HttpClient


Does this need to be its own class? (See related comment below about method naming/location.)

If we do not use HTTP GET, I will make it simpler

anarchivist · 2026-01-15T17:57:50Z

lib/http_client.rb

+    ConnectionFailed = Class.new(StandardError)
+
+    class << self
+      def request(method, url, timeout: 5)


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?

Yes, to inherited from OkComputer::HttpCheck

anarchivist · 2026-01-15T17:59:39Z

lib/http_head_check.rb

@@ -0,0 +1,39 @@
+module GeoDataHealthCheck
+  class HttpHeadCheck < OkComputer::Check


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.

It sounds good to inherited from HttpCheck. Thanks

anarchivist · 2026-01-16T19:55:22Z

lib/check_server.rb

+      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)


I'm talking specifically about invoking the method OkComputer::Registry.register. Why are we invoking this method in this part of the code?

anarchivist · 2026-01-16T19:55:37Z

lib/check_server.rb

I think my question is why can't we do something like the following in the OkComputer initializer:

OkComputer::Registry.register 'geoserver-public', GeodataHealthCheck::HttpHeadCheck.new(config.x.servers[:geoserver]) OkComputer::Registry.register 'geoserver-secure', GeodataHealthCheck::HttpHeadCheck.new(config.x.servers[:secure_geoserver]) # et cetera

It might be possible that we need additional helper methods to build out the URL for the GetCapabilities method.

anarchivist · 2026-01-16T19:57:05Z

lib/http_head_check.rb

+    def skip_check
+      mark_failure
+      mark_message 'No URL configured; health check was skipped...'
+    end


I think it's not necessary - the rescue pattern will catch if the url is not set.

anarchivist · 2026-01-16T19:59:37Z

lib/check_server.rb

+    end
+
+    def spatial_server(type)
+      return wrong_type(type) unless %w[public UCB].include? type


Can you clarify about why we need separate checks for public and UCB-only datasets on spatial?

Just see this note. The UCB-only is configured to redirect to the CAS authentication, should we just ignore it?

my primary concern for this healthcheck is whether spatial itself reachable - i'm less ocncered about it implicitly checking CAS.

Ok, the ucb spatial endpoint check is removed.

yzhoubk · 2026-01-16T22:30:07Z

@anarchivist I did the updates accordingly, could you please take a look? Thanks

yzhoubk added 5 commits January 12, 2026 12:58

Add http head check etc.

cfa5c6a

consolidated

b8fed47

uncheck Lint/MissingSuper rule

5c6fbc0

code formating

abc3ad6

Handling incorrect server type

b3fb91e

yzhoubk requested a review from anarchivist January 15, 2026 00:48

yzhoubk self-assigned this Jan 15, 2026

change url check

b474242

anarchivist requested changes Jan 15, 2026

View reviewed changes

anarchivist reviewed Jan 16, 2026

View reviewed changes

Refactor to follow the same registry call style in okcomputer.rb

36a1dfc

Remove spatial server ucb endpoint check

bb8d737

		@@ -0,0 +1,39 @@
		module GeoDataHealthCheck
		class HttpHeadCheck < OkComputer::Check

Ap 547: Add other healthchecks to Geodata #71

Are you sure you want to change the base?

Ap 547: Add other healthchecks to Geodata #71

Conversation

yzhoubk commented Jan 15, 2026

Uh oh!

anarchivist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yzhoubk commented Jan 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants