protomaps / go-pmtiles

Single-file executable tool for working with PMTiles archives
BSD 3-Clause "New" or "Revised" License
356 stars 49 forks source link

/health endpoint when serving #115

Closed zifeo closed 7 months ago

zifeo commented 8 months ago

useful for health checks

bdon commented 8 months ago

What information would this provide over making a HEAD request for root path /?

I recommend more full-fledged HTTP server functionality can be achieved by embedding the pmtiles module inside of Caddy.

zifeo commented 8 months ago

Just an empty GET /. We are already having a reverse-proxy in our setup, is there any other advantage to use caddy instead of pmtiles?

bdon commented 8 months ago

The root path of pmtiles serve already returns a 404, is that a sufficient health check? Otherwise we need to represent a resource that corresponds to no pmtiles archive.

Caddy provides reverse proxy functionality and SSL termination along with other features, so we don't need to implement any of those in go-pmtiles.

zifeo commented 8 months ago

Got it, we have this elsewhere in the stack so no need to duplicate :). Usually, probes follow this rule:

Any code greater than or equal to 200 and less than 400 indicates success. Any other code indicates failure.

bdon commented 8 months ago

It seems reasonable to change this line to https://github.com/protomaps/go-pmtiles/blob/main/pmtiles/server.go#L392 to a 204 in the case of an empty path - an empty archive name is impossible

zifeo commented 8 months ago

Seems reasonable, maybe /health/[archive] → 200 is easier to understand (not sure the json helps unless the return is actual json metadata).

bdon commented 8 months ago

Is your goal to determine if an archive exists, or just that the server is running? Adding another route like /health is application-specific and out of scope.

zifeo commented 8 months ago

Just if the server is running, whether the archive or even the tile is available is not needed.

bdon commented 8 months ago

in 1.12.0 https://github.com/protomaps/go-pmtiles/releases/tag/v1.12.0 the root path / now returns a 204 No Content instead of 404 Not Found.

bdon commented 7 months ago

Closing this as the 204 for / should be sufficient.