From 84c913f749693819cd12326d579cc600a683f40b Mon Sep 17 00:00:00 2001 From: Isaac Diamond Date: Wed, 26 Jun 2024 13:45:46 -0700 Subject: [PATCH 1/2] Add healthcheck handler --- api_handler.go | 4 ++++ handler.go | 1 + 2 files changed, 5 insertions(+) diff --git a/api_handler.go b/api_handler.go index be56f508..d35fe6a5 100644 --- a/api_handler.go +++ b/api_handler.go @@ -275,6 +275,10 @@ func (a *apiHandler) JobList(rw http.ResponseWriter, req *http.Request) { } } +func (a *apiHandler) Healthcheck(rw http.ResponseWriter, req *http.Request) { + a.writeResponse(req.Context(), rw, []byte("{\"status\": \"ok\"}")) +} + func (a *apiHandler) QueueGet(rw http.ResponseWriter, req *http.Request) { ctx, cancel := context.WithTimeout(req.Context(), 5*time.Second) defer cancel() diff --git a/handler.go b/handler.go index 6d460525..f1d260de 100644 --- a/handler.go +++ b/handler.go @@ -100,6 +100,7 @@ func NewHandler(opts *HandlerOpts) (http.Handler, error) { mux.HandleFunc("GET /api/workflows/{id}", handler.WorkflowGet) mux.HandleFunc("GET /api/states", handler.StatesAndCounts) mux.HandleFunc("/api", http.NotFound) + mux.HandleFunc("GET /healthcheck", handler.Healthcheck) mux.Handle("/", intercept404(fileServer, serveIndex)) if prefix != "/" { From 29a4eed2c3f2c45165390484165fb2a92de7cafa Mon Sep 17 00:00:00 2001 From: Brandur Date: Thu, 27 Jun 2024 19:57:49 -0700 Subject: [PATCH 2/2] Add more elaborate health check endpoint that optionally hits the database An alternative to #68 that adds a pair of health check endpoints: * `GET /api/health-checks/complete` * `GET /api/health-checks/minimal` The minimal endpoint just returns an OK response regardless of anything else so it'll return 200 as long as the Go process is up. The complete endpoint runs the additional check of pinging the database, verifying its liveliness as well. This is useful because if the database is totally down then River UI will be totally non-functional, but the minimal endpoint would still return OK, and it's nice to have an alternative that'll return an unhealthy status. Based on something I wrote about last year here [1]. Fixes #67. [1] https://brandur.org/fragments/database-health-check --- api_handler.go | 70 +++++++++++++++++++++++----- api_handler_test.go | 56 ++++++++++++++++++++-- handler.go | 2 +- handler_test.go | 2 + internal/apiendpoint/api_endpoint.go | 11 ++++- internal/apierror/api_error.go | 18 ++++++- internal/apierror/api_error_test.go | 25 ++++++++++ 7 files changed, 166 insertions(+), 18 deletions(-) diff --git a/api_handler.go b/api_handler.go index d35fe6a5..46c4ac74 100644 --- a/api_handler.go +++ b/api_handler.go @@ -42,9 +42,63 @@ type withSetBundle interface { SetBundle(bundle *apiBundle) } +type statusResponse struct { + Status string `json:"status"` +} + +var statusResponseOK = &statusResponse{Status: "ok"} //nolint:gochecknoglobals + +type healthCheckGetEndpoint struct { + apiBundle + apiendpoint.Endpoint[healthCheckGetRequest, statusResponse] +} + +func (*healthCheckGetEndpoint) Meta() *apiendpoint.EndpointMeta { + return &apiendpoint.EndpointMeta{ + Pattern: "GET /api/health-checks/{name}", + StatusCode: http.StatusOK, + } +} + +type healthCheckName string + +const ( + healthCheckNameComplete healthCheckName = "complete" + healthCheckNameMinimal healthCheckName = "minimal" +) + +type healthCheckGetRequest struct { + Name healthCheckName `json:"-"` // from ExtractRaw +} + +func (req *healthCheckGetRequest) ExtractRaw(r *http.Request) error { + req.Name = healthCheckName(r.PathValue("name")) + return nil +} + +func (a *healthCheckGetEndpoint) Execute(ctx context.Context, req *healthCheckGetRequest) (*statusResponse, error) { + switch req.Name { + case healthCheckNameComplete: + if _, err := a.dbPool.Exec(ctx, "SELECT 1"); err != nil { + return nil, apierror.WithInternalError( + apierror.NewServiceUnavailable("Unable to query database. Check logs for details."), + err, + ) + } + + case healthCheckNameMinimal: + // fall through to OK status response below + + default: + return nil, apierror.NewNotFound("Health check %q not found. Use either `complete` or `minimal`.", req.Name) + } + + return statusResponseOK, nil +} + type jobCancelEndpoint struct { apiBundle - apiendpoint.Endpoint[jobCancelRequest, jobCancelResponse] + apiendpoint.Endpoint[jobCancelRequest, statusResponse] } func (*jobCancelEndpoint) Meta() *apiendpoint.EndpointMeta { @@ -58,12 +112,8 @@ type jobCancelRequest struct { JobIDs []int64String `json:"ids"` } -type jobCancelResponse struct { - Status string `json:"status"` -} - -func (a *jobCancelEndpoint) Execute(ctx context.Context, req *jobCancelRequest) (*jobCancelResponse, error) { - return dbutil.WithTxV(ctx, a.dbPool, func(ctx context.Context, tx pgx.Tx) (*jobCancelResponse, error) { +func (a *jobCancelEndpoint) Execute(ctx context.Context, req *jobCancelRequest) (*statusResponse, error) { + return dbutil.WithTxV(ctx, a.dbPool, func(ctx context.Context, tx pgx.Tx) (*statusResponse, error) { updatedJobs := make(map[int64]*rivertype.JobRow) for _, jobID := range req.JobIDs { jobID := int64(jobID) @@ -78,7 +128,7 @@ func (a *jobCancelEndpoint) Execute(ctx context.Context, req *jobCancelRequest) } // TODO: return jobs in response, use in frontend instead of invalidating - return &jobCancelResponse{Status: "ok"}, nil + return statusResponseOK, nil }) } @@ -275,10 +325,6 @@ func (a *apiHandler) JobList(rw http.ResponseWriter, req *http.Request) { } } -func (a *apiHandler) Healthcheck(rw http.ResponseWriter, req *http.Request) { - a.writeResponse(req.Context(), rw, []byte("{\"status\": \"ok\"}")) -} - func (a *apiHandler) QueueGet(rw http.ResponseWriter, req *http.Request) { ctx, cancel := context.WithTimeout(req.Context(), 5*time.Second) defer cancel() diff --git a/api_handler_test.go b/api_handler_test.go index 61043128..9e55262c 100644 --- a/api_handler_test.go +++ b/api_handler_test.go @@ -44,7 +44,57 @@ func setupEndpoint[TEndpoint any](ctx context.Context, t *testing.T) (*TEndpoint } } -func TestAPIHandlerJobCancel(t *testing.T) { +func TestHandlerHealthCheckGetEndpoint(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + t.Run("CompleteSuccess", func(t *testing.T) { + t.Parallel() + + endpoint, _ := setupEndpoint[healthCheckGetEndpoint](ctx, t) + + resp, err := endpoint.Execute(ctx, &healthCheckGetRequest{Name: healthCheckNameComplete}) + require.NoError(t, err) + require.Equal(t, statusResponseOK, resp) + }) + + t.Run("CompleteDatabaseError", func(t *testing.T) { + t.Parallel() + + endpoint, bundle := setupEndpoint[healthCheckGetEndpoint](ctx, t) + + // Roll back prematurely so we get a database error. + require.NoError(t, bundle.tx.Rollback(ctx)) + + _, err := endpoint.Execute(ctx, &healthCheckGetRequest{Name: healthCheckNameComplete}) + requireAPIError(t, apierror.WithInternalError( + apierror.NewServiceUnavailable("Unable to query database. Check logs for details."), + pgx.ErrTxClosed, + ), err) + }) + + t.Run("Minimal", func(t *testing.T) { + t.Parallel() + + endpoint, _ := setupEndpoint[healthCheckGetEndpoint](ctx, t) + + resp, err := endpoint.Execute(ctx, &healthCheckGetRequest{Name: healthCheckNameMinimal}) + require.NoError(t, err) + require.Equal(t, statusResponseOK, resp) + }) + + t.Run("NotFound", func(t *testing.T) { + t.Parallel() + + endpoint, _ := setupEndpoint[healthCheckGetEndpoint](ctx, t) + + _, err := endpoint.Execute(ctx, &healthCheckGetRequest{Name: "other"}) + requireAPIError(t, apierror.NewNotFound("Health check %q not found. Use either `complete` or `minimal`.", "other"), err) + }) +} + +func TestJobCancelEndpoint(t *testing.T) { t.Parallel() ctx := context.Background() @@ -62,7 +112,7 @@ func TestAPIHandlerJobCancel(t *testing.T) { resp, err := endpoint.Execute(ctx, &jobCancelRequest{JobIDs: []int64String{int64String(insertRes1.Job.ID), int64String(insertRes2.Job.ID)}}) require.NoError(t, err) - require.Equal(t, &jobCancelResponse{Status: "ok"}, resp) + require.Equal(t, statusResponseOK, resp) updatedJob1, err := bundle.client.JobGetTx(ctx, bundle.tx, insertRes1.Job.ID) require.NoError(t, err) @@ -83,7 +133,7 @@ func TestAPIHandlerJobCancel(t *testing.T) { }) } -func TestAPIHandlerJobGet(t *testing.T) { +func TestJobGetEndpoint(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/handler.go b/handler.go index f1d260de..00a2c08e 100644 --- a/handler.go +++ b/handler.go @@ -88,6 +88,7 @@ func NewHandler(opts *HandlerOpts) (http.Handler, error) { prefix := opts.Prefix mux := http.NewServeMux() + apiendpoint.Mount(mux, opts.Logger, &healthCheckGetEndpoint{apiBundle: apiBundle}) mux.HandleFunc("GET /api/jobs", handler.JobList) apiendpoint.Mount(mux, opts.Logger, &jobCancelEndpoint{apiBundle: apiBundle}) mux.HandleFunc("POST /api/jobs/delete", handler.JobDelete) @@ -100,7 +101,6 @@ func NewHandler(opts *HandlerOpts) (http.Handler, error) { mux.HandleFunc("GET /api/workflows/{id}", handler.WorkflowGet) mux.HandleFunc("GET /api/states", handler.StatesAndCounts) mux.HandleFunc("/api", http.NotFound) - mux.HandleFunc("GET /healthcheck", handler.Healthcheck) mux.Handle("/", intercept404(fileServer, serveIndex)) if prefix != "/" { diff --git a/handler_test.go b/handler_test.go index ac49b7b8..6f855a99 100644 --- a/handler_test.go +++ b/handler_test.go @@ -88,6 +88,8 @@ func TestNewHandlerIntegration(t *testing.T) { // API calls // + makeAPICall(t, "HealthCheckGetComplete", http.MethodGet, makeURL("/api/health-checks/%s", healthCheckNameComplete), nil) + makeAPICall(t, "HealthCheckGetMinimal", http.MethodGet, makeURL("/api/health-checks/%s", healthCheckNameMinimal), nil) makeAPICall(t, "JobCancel", http.MethodPost, makeURL("/api/jobs/cancel"), mustMarshalJSON(t, &jobCancelRequest{JobIDs: []int64String{int64String(job.ID)}})) makeAPICall(t, "JobGet", http.MethodGet, makeURL("/api/jobs/%d", job.ID), nil) } diff --git a/internal/apiendpoint/api_endpoint.go b/internal/apiendpoint/api_endpoint.go index 52288e0c..9bbe4e3e 100644 --- a/internal/apiendpoint/api_endpoint.go +++ b/internal/apiendpoint/api_endpoint.go @@ -141,8 +141,17 @@ func executeAPIEndpoint[TReq any, TResp any](w http.ResponseWriter, r *http.Requ if err != nil { var apiErr apierror.Interface if errors.As(err, &apiErr) { + logAttrs := []any{ + slog.String("error", apiErr.Error()), + } + + if internalErr := apiErr.GetInternalError(); internalErr != nil { + logAttrs = append(logAttrs, slog.String("internal_error", internalErr.Error())) + } + // Logged at info level because API errors are normal. - logger.InfoContext(ctx, "API error response", slog.String("error", apiErr.Error())) + logger.InfoContext(ctx, "API error response", logAttrs...) + apiErr.Write(ctx, logger, w) return } diff --git a/internal/apierror/api_error.go b/internal/apierror/api_error.go index ac7bc257..ac2c9f5a 100644 --- a/internal/apierror/api_error.go +++ b/internal/apierror/api_error.go @@ -16,6 +16,11 @@ import ( // // APIErrorInterface should be used with errors.As instead of this struct. type APIError struct { + // InternalError is an additional error that might be associated with the + // API error. It's not returned in the API error response, but is logged in + // API endpoint execution to provide extra information for operators. + InternalError error `json:"-"` + // Message is a descriptive, human-friendly message indicating what went // wrong. Try to make error messages as actionable as possible to help the // caller easily fix what went wrong. @@ -26,7 +31,9 @@ type APIError struct { StatusCode int `json:"-"` } -func (e *APIError) Error() string { return e.Message } +func (e *APIError) Error() string { return e.Message } +func (e *APIError) GetInternalError() error { return e.InternalError } +func (e *APIError) SetInternalError(internalErr error) { e.InternalError = internalErr } // Write writes the API error to an HTTP response, writing to the given logger // in case of a problem. @@ -48,9 +55,18 @@ func (e *APIError) Write(ctx context.Context, logger *slog.Logger, w http.Respon // won't be usable as an errors.As target. type Interface interface { Error() string + GetInternalError() error + SetInternalError(internalErr error) Write(ctx context.Context, logger *slog.Logger, w http.ResponseWriter) } +// WithInternalError is a convenience function for assigning an internal error +// to the given API error and returning it. +func WithInternalError[TAPIError Interface](apiErr TAPIError, internalErr error) TAPIError { + apiErr.SetInternalError(internalErr) + return apiErr +} + // // BadRequest // diff --git a/internal/apierror/api_error_test.go b/internal/apierror/api_error_test.go index 060e8b8a..6a61cb2c 100644 --- a/internal/apierror/api_error_test.go +++ b/internal/apierror/api_error_test.go @@ -3,6 +3,7 @@ package apierror import ( "context" "encoding/json" + "errors" "net/http/httptest" "testing" @@ -11,6 +12,18 @@ import ( "github.com/riverqueue/riverui/internal/riverinternaltest" ) +func TestAPIError(t *testing.T) { + t.Parallel() + + var ( + anErr = errors.New("an error") + apiErr = NewBadRequest("Bad request.") + ) + + apiErr.SetInternalError(anErr) + require.Equal(t, anErr, apiErr.GetInternalError()) +} + func TestAPIErrorJSON(t *testing.T) { t.Parallel() @@ -40,6 +53,18 @@ func TestAPIErrorWrite(t *testing.T) { ) } +func TestWithInternalError(t *testing.T) { + t.Parallel() + + var ( + anErr = errors.New("an error") + apiErr = NewBadRequest("Bad request.") + ) + + apiErr = WithInternalError(apiErr, anErr) + require.Equal(t, anErr, apiErr.InternalError) +} + func mustMarshalJSON(t *testing.T, v any) []byte { t.Helper()