Add error callbacks that emit metrics (#53)

These are non-default because they require a metrics registry, but the
default handlers are now implemented by passing a nil registry to the
metric version.

Note that this contains an API break in the AsyncErrorCallback type, but
I don't expect there to be many (any?) custom implementations.

Author: bluekeyes

README.md

@@ -205,9 +205,11 @@ baseHandler, err := githubapp.NewDefaultCachingClientCreator(
 
 ## Metrics
 
-`go-githubapp` uses [rcrowley/go-metrics][] to provide metrics. GitHub clients
-emit the metrics below if configured with the `githubapp.ClientMetrics`
-middleware.
+`go-githubapp` uses [rcrowley/go-metrics][] to provide metrics. Metrics are
+optional and disabled by default.
+
+GitHub clients emit the following metrics when configured with the
+`githubapp.ClientMetrics` middleware:
 
 | metric name | type | definition |
 | ----------- | ---- | ---------- |
@@ -230,6 +232,13 @@ When using [asynchronous dispatch](#asynchronous-dispatch), the
 | `github.event.dropped` | `counter` | the number events dropped due to limited queue capacity |
 | `github.event.age` | `histogram` | the age (queue time) in milliseconds of events at processing time |
 
+The `MetricsErrorCallback` and `MetricsAsyncErrorCallback` error callbacks for
+the event dispatcher and asynchronous schedulers emit the following metrics:
+
+| metric name | type | definition |
+| ----------- | ---- | ---------- |
+| `github.handler.error[event:<type>]` | `counter` | the number of processing errors, tagged with the GitHub event type |
+
 Note that metrics need to be published in order to be useful. Several
 [publishing options][] are available or you can implement your own.
 

githubapp/dispatcher.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/google/go-github/v32/github"
 	"github.com/pkg/errors"
+	"github.com/rcrowley/go-metrics"
 	"github.com/rs/zerolog"
 )
 
@@ -201,24 +202,36 @@ func (d *eventDispatcher) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	d.onResponse(w, r, eventType, ok)
 }
 
-// DefaultErrorCallback logs errors and responds with a 500 status code.
+// DefaultErrorCallback logs errors and responds with an appropriate status code.
 func DefaultErrorCallback(w http.ResponseWriter, r *http.Request, err error) {
-	logger := zerolog.Ctx(r.Context())
+	defaultErrorCallback(w, r, err)
+}
 
-	var ve ValidationError
-	if errors.As(err, &ve) {
-		logger.Warn().Err(ve.Cause).Msgf("Received invalid webhook headers or payload")
-		http.Error(w, "Invalid webhook headers or payload", http.StatusBadRequest)
-		return
-	}
-	if errors.Is(err, ErrCapacityExceeded) {
-		logger.Warn().Msg("Dropping webhook event due to over-capacity scheduler")
-		http.Error(w, "No capacity available to processes this event", http.StatusServiceUnavailable)
-		return
-	}
+var defaultErrorCallback = MetricsErrorCallback(nil)
+
+// MetricsErrorCallback logs errors, increments an error counter, and responds
+// with an appropriate status code.
+func MetricsErrorCallback(reg metrics.Registry) ErrorCallback {
+	return func(w http.ResponseWriter, r *http.Request, err error) {
+		logger := zerolog.Ctx(r.Context())
 
-	logger.Error().Err(err).Msg("Unexpected error handling webhook")
-	http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+		var ve ValidationError
+		if errors.As(err, &ve) {
+			logger.Warn().Err(ve.Cause).Msgf("Received invalid webhook headers or payload")
+			http.Error(w, "Invalid webhook headers or payload", http.StatusBadRequest)
+			return
+		}
+		if errors.Is(err, ErrCapacityExceeded) {
+			logger.Warn().Msg("Dropping webhook event due to over-capacity scheduler")
+			http.Error(w, "No capacity available to processes this event", http.StatusServiceUnavailable)
+			return
+		}
+
+		logger.Error().Err(err).Msg("Unexpected error handling webhook")
+		errorCounter(reg, r.Header.Get("X-Github-Event")).Inc(1)
+
+		http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+	}
 }
 
 // DefaultResponseCallback responds with a 200 OK for handled events and a 202

githubapp/errors.go

@@ -0,0 +1,37 @@
+// Copyright 2020 Palantir Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package githubapp
+
+import (
+	"fmt"
+
+	"github.com/rcrowley/go-metrics"
+)
+
+const (
+	MetricsKeyHandlerError = "github.handler.error"
+)
+
+func errorCounter(r metrics.Registry, event string) metrics.Counter {
+	if r == nil {
+		return metrics.NilCounter{}
+	}
+
+	key := MetricsKeyHandlerError
+	if event != "" {
+		key = fmt.Sprintf("%s[event:%s]", key, event)
+	}
+	return metrics.GetOrRegisterCounter(key, r)
+}

githubapp/scheduler.go

@@ -59,11 +59,21 @@ func (d Dispatch) Execute(ctx context.Context) error {
 // AsyncErrorCallback is called by an asynchronous scheduler when an event
 // handler returns an error. The error from the handler is passed directly as
 // the final argument.
-type AsyncErrorCallback func(ctx context.Context, err error)
+type AsyncErrorCallback func(ctx context.Context, d Dispatch, err error)
 
 // DefaultAsyncErrorCallback logs errors.
-func DefaultAsyncErrorCallback(ctx context.Context, err error) {
-	zerolog.Ctx(ctx).Error().Err(err).Msg("Unexpected error handling webhook")
+func DefaultAsyncErrorCallback(ctx context.Context, d Dispatch, err error) {
+	defaultAsyncErrorCallback(ctx, d, err)
+}
+
+var defaultAsyncErrorCallback = MetricsAsyncErrorCallback(nil)
+
+// MetricsAsyncErrorCallback logs errors and increments an error counter.
+func MetricsAsyncErrorCallback(reg metrics.Registry) AsyncErrorCallback {
+	return func(ctx context.Context, d Dispatch, err error) {
+		zerolog.Ctx(ctx).Error().Err(err).Msg("Unexpected error handling webhook")
+		errorCounter(reg, d.EventType).Inc(1)
+	}
 }
 
 // ContextDeriver creates a new independent context from a request's context.
@@ -156,6 +166,7 @@ type scheduler struct {
 func (s *scheduler) safeExecute(ctx context.Context, d Dispatch) {
 	var err error
 	defer func() {
+		atomic.AddInt64(&s.activeWorkers, -1)
 		if r := recover(); r != nil {
 			if rerr, ok := r.(error); ok {
 				err = rerr
@@ -164,9 +175,8 @@ func (s *scheduler) safeExecute(ctx context.Context, d Dispatch) {
 			}
 		}
 		if err != nil && s.onError != nil {
-			s.onError(ctx, err)
+			s.onError(ctx, d, err)
 		}
-		atomic.AddInt64(&s.activeWorkers, -1)
 	}()
 
 	atomic.AddInt64(&s.activeWorkers, 1)

githubapp/scheduler_test.go

@@ -64,7 +64,7 @@ func TestAsyncScheduler(t *testing.T) {
 
 	t.Run("errorCallback", func(t *testing.T) {
 		errc := make(chan error, 1)
-		cb := func(ctx context.Context, err error) {
+		cb := func(ctx context.Context, d Dispatch, err error) {
 			errc <- err
 		}
 
@@ -115,7 +115,7 @@ func TestQueueAsyncScheduler(t *testing.T) {
 
 	t.Run("errorCallback", func(t *testing.T) {
 		errc := make(chan error, 1)
-		cb := func(ctx context.Context, err error) {
+		cb := func(ctx context.Context, d Dispatch, err error) {
 			errc <- err
 		}