Return info msg for /health endpoint (#1465)

* Return info msg for `/health` endpoint

Return 200 OK for status `ready` and `upTimeStats`

Resolves #1450

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

* Address PR comments

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

* Fix failing test

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

* Address PR comments

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

* Retrigger tests

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

* Retrigger tests

Signed-off-by: stefan vassilev <stefanvassilev1@gmail.com>

* Make thread-safe

Signed-off-by: Yuri Shkuro <ys@uber.com>

crossdock/main.go

@@ -102,10 +102,14 @@ func (h *clientHandler) isInitialized() bool {
 	return atomic.LoadUint64(&h.initialized) != 0
 }
 
+func is2xxStatusCode(statusCode int) bool {
+	return statusCode >= 200 && statusCode <= 299
+}
+
 func httpHealthCheck(logger *zap.Logger, service, healthURL string) {
 	for i := 0; i < 240; i++ {
 		res, err := http.Get(healthURL)
-		if err == nil && res.StatusCode == 204 {
+		if err == nil && is2xxStatusCode(res.StatusCode) {
 			logger.Info("Health check successful", zap.String("service", service))
 			return
 		}

pkg/healthcheck/handler.go

@@ -15,8 +15,11 @@
 package healthcheck
 
 import (
+	"encoding/json"
+	"fmt"
 	"net/http"
 	"sync/atomic"
+	"time"
 
 	"go.uber.org/zap"
 )
@@ -46,24 +49,43 @@ func (s Status) String() string {
 	}
 }
 
+type healthCheckResponse struct {
+	statusCode int
+	StatusMsg  string    `json:"status"`
+	UpSince    time.Time `json:"upSince"`
+	Uptime     string    `json:"uptime"`
+}
+
+type state struct {
+	status  Status
+	upSince time.Time
+}
+
 // HealthCheck provides an HTTP endpoint that returns the health status of the service
 type HealthCheck struct {
-	state   int32 // atomic, keep at the top to be word-aligned
-	logger  *zap.Logger
-	mapping map[Status]int
-	server  *http.Server
+	state     atomic.Value // stores state struct
+	logger    *zap.Logger
+	responses map[Status]healthCheckResponse
+	server    *http.Server
 }
 
 // New creates a HealthCheck with the specified initial state.
 func New() *HealthCheck {
 	hc := &HealthCheck{
-		state: int32(Unavailable),
-		mapping: map[Status]int{
-			Unavailable: http.StatusServiceUnavailable,
-			Ready:       http.StatusNoContent,
-		},
 		logger: zap.NewNop(),
+		responses: map[Status]healthCheckResponse{
+			Unavailable: {
+				statusCode: http.StatusServiceUnavailable,
+				StatusMsg:  "Server not available",
+			},
+			Ready: {
+				statusCode: http.StatusOK,
+				StatusMsg:  "Server available",
+			},
+		},
+		server: nil,
 	}
+	hc.state.Store(state{status: Unavailable})
 	return hc
 }
 
@@ -75,21 +97,45 @@ func (hc *HealthCheck) SetLogger(logger *zap.Logger) {
 // Handler creates a new HTTP handler.
 func (hc *HealthCheck) Handler() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-		w.WriteHeader(hc.mapping[hc.Get()])
-		// this is written only for response with an entity, so, it won't be used for a 204 - No content
-		w.Write([]byte("Server not available"))
+		state := hc.getState()
+		template := hc.responses[state.status]
+		w.WriteHeader(template.statusCode)
+
+		w.Header().Set("Content-Type", "application/json")
+		w.Write(hc.createRespBody(state, template))
 	})
 }
 
+func (hc *HealthCheck) createRespBody(state state, template healthCheckResponse) []byte {
+	resp := template // clone
+	if state.status == Ready {
+		resp.UpSince = state.upSince
+		resp.Uptime = fmt.Sprintf("%v", time.Since(state.upSince))
+	}
+	healthCheckStatus, _ := json.Marshal(resp)
+	return healthCheckStatus
+}
+
 // Set a new health check status
-func (hc *HealthCheck) Set(state Status) {
-	atomic.StoreInt32(&hc.state, int32(state))
-	hc.logger.Info("Health Check state change", zap.Stringer("status", hc.Get()))
+func (hc *HealthCheck) Set(status Status) {
+	oldState := hc.getState()
+	newState := state{status: status}
+	if status == Ready {
+		if oldState.status != Ready {
+			newState.upSince = time.Now()
+		}
+	}
+	hc.state.Store(newState)
+	hc.logger.Info("Health Check state change", zap.Stringer("status", status))
 }
 
 // Get the current status of this health check
 func (hc *HealthCheck) Get() Status {
-	return Status(atomic.LoadInt32(&hc.state))
+	return hc.getState().status
+}
+
+func (hc *HealthCheck) getState() state {
+	return hc.state.Load().(state)
 }
 
 // Ready is a shortcut for Set(Ready) (kept for backwards compatibility)

pkg/healthcheck/internal_test.go

@@ -15,9 +15,12 @@
 package healthcheck
 
 import (
+	"encoding/json"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -34,5 +37,35 @@ func TestHttpCall(t *testing.T) {
 
 	resp, err := http.Get(server.URL + "/")
 	require.NoError(t, err)
-	assert.Equal(t, http.StatusNoContent, resp.StatusCode)
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+
+	hr := parseHealthCheckResponse(t, resp)
+	assert.Equal(t, "Server available", hr.StatusMsg)
+	// de-serialized timestamp loses monotonic clock value, so assert.Equals() doesn't work.
+	// https://github.com/stretchr/testify/issues/502
+	if want, have := hc.getState().upSince, hr.UpSince; !assert.True(t, want.Equal(have)) {
+		t.Logf("want='%v', have='%v'", want, have)
+	}
+	assert.NotZero(t, hr.Uptime)
+	t.Logf("uptime=%v", hr.Uptime)
+
+	time.Sleep(time.Millisecond)
+	hc.Set(Unavailable)
+
+	resp, err = http.Get(server.URL + "/")
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode)
+
+	hrNew := parseHealthCheckResponse(t, resp)
+	assert.Zero(t, hrNew.Uptime)
+	assert.Zero(t, hrNew.UpSince)
+}
+
+func parseHealthCheckResponse(t *testing.T, resp *http.Response) healthCheckResponse {
+	body, err := ioutil.ReadAll(resp.Body)
+	require.NoError(t, err)
+	var hr healthCheckResponse
+	err = json.Unmarshal(body, &hr)
+	require.NoError(t, err)
+	return hr
 }