Chi: Heartbeat improvements

[rickb777]

The Heartbeat middleware is useful but leaves scope for a few improvements:

• the response should be 204-no content; there should not be any response data.
• the URL matching should be case-sensitive (because no humans are involved)
• it should also handle HEAD requests (good HTTP practice).

Code suggestion

func Heartbeat(endpoint string) func(http.Handler) http.Handler {
    f := func(h http.Handler) http.Handler {
        fn := func(w http.ResponseWriter, r *http.Request) {
            if (r.Method == "GET" || r.Method == "HEAD") && r.URL.Path == endpoint {
                w.WriteHeader(http.StatusNoContent)
                return
            }
            h.ServeHTTP(w, r)
        }
        return http.HandlerFunc(fn)
    }
    return f
}

[pkieltyka]

Great suggestions. Totally agree with all, but why not return a response body?

[rickb777]

204-No-Content doesn't need/allow a response body, so saving a few bytes. There will be fewer headers too.

Win+win!

[thedevsaddam]

@pkieltyka It seems a good idea, heartbeat doesn't need to write data in response body. May I send a PR to update the code?

[VojtechVitek]

Changing status to HTTP 204 would be a breaking change.

I suggest not making any changes in chi's middleware pkg & closing this issue.

[thedevsaddam]

@VojtechVitek You are correct. But for next major release this changes won't be a problem right?

[VojtechVitek]

Technically we could do this in the next major release - but I'm not sure if we want to. What's the point of changing the status code anyway? To avoid the single byte in the response body?

--

The /ping endpoints are usually called by external health checks (load balancers, status pages, reverse proxies etc.). I think this feature request is not worth the price of potential regressions in large systems, where the infrastructure and/or status pages might be controlled by these little /ping endpoints and something might rely on HTTP 200 specifically.

Take a look at https://github.com/influxdata/influxdb/issues/9772 for example.. InfluxDB had to switch from HTTP 204 to HTTP 200 to support health checks in Google Kubernetes
Cloud.

Based on the above, I recommend to close this issue.

Anyone can easily fork or copy/paste this middleware if HTTP 200 and . doesn't work for them for some reason. I don't see a value in changing something, what works well, as designed.

[gayanhewa]

I would agree with @VojtechVitek if you look at AWS ELB (Classic) it has a similar case where the ELB health checks would fail for anything other than a 200 response. I guess the majority of the external services that would depend on the heartbeat endpoint would naturally expect a 200 response to be returned determine if the service is healthy. Newer products seem to have configurable options for this ie. AWS Application Load Balancer

[rickb777]

This change saves more than a single byte: the Content-Type header doesn't need to be sent either.

[gayanhewa]

@rickb777 wouldn't compatibility with existing system become an issue? If I run my services in an environment where I have no control over how the heartbeats are used for health checks this would essentially result in these services identifying all my services as unhealthy when they are not. I do get your point on the savings from what is being sent. May be allowing it to be configurable to choose between what a heartbeat should respond with would give control to the developers consuming the library.

[rickb777]

I would expect any health checking system to accept 2xx responses (especially 200 and 204) as 'good' - that's the normative nature of the success statuses.

However I understand your worry here. Existing systems may be buggy (in the sense of allowing 200 but not any other 2xx).

I have three suggestions: (a) there might be two flavours of hearbeat middleware covering the two cases, or (b) the existing middleware could be changed as I suggested but still use 200, or (c) the status code could be an option.

[bgv]

In my view this middleware should be taken as just an example.

On its own its just that, unless your service is completely self contained and have no other dependencies.

However most of the time services depend on database or 3rd party API and in this case you are going to get successful health checks while in reality users consuming the service face errors.

Best way is copy it and tweak it to your particular needs.

Just my 2¢

[VojtechVitek]

Cool. Let's close this issue then. The core library need to stay on 200 OK for the reasons mentioned above.

We're trying to keep the core as minimal as possible, so if you need a second flavor of the heartbeat pkg, please fork the repo or copy/paste the function. A reference link would be useful, I'm sure some people will find it useful too. Thanks!