Merge pull request #149 from alphagov/improve_airbrake_hook

[#76800642] Rework the Airbrake hook

README.md

@@ -82,7 +82,7 @@ func init() {
 
   // Use the Airbrake hook to report errors that have Error severity or above to
   // an exception tracker. You can create custom hooks, see the Hooks section.
-  log.AddHook(&logrus_airbrake.AirbrakeHook{})
+  log.AddHook(airbrake.NewHook("https://example.com", "xyz", "development"))
 
   // Output to stderr instead of stdout, could also be a file.
   log.SetOutput(os.Stderr)
@@ -164,43 +164,8 @@ You can add hooks for logging levels. For example to send errors to an exception
 tracking service on `Error`, `Fatal` and `Panic`, info to StatsD or log to
 multiple places simultaneously, e.g. syslog.
 
-```go
-// Not the real implementation of the Airbrake hook. Just a simple sample.
-import (
-  log "github.com/Sirupsen/logrus"
-)
-
-func init() {
-  log.AddHook(new(AirbrakeHook))
-}
-
-type AirbrakeHook struct{}
-
-// `Fire()` takes the entry that the hook is fired for. `entry.Data[]` contains
-// the fields for the entry. See the Fields section of the README.
-func (hook *AirbrakeHook) Fire(entry *logrus.Entry) error {
-  err := airbrake.Notify(entry.Data["error"].(error))
-  if err != nil {
-    log.WithFields(log.Fields{
-      "source":   "airbrake",
-      "endpoint": airbrake.Endpoint,
-    }).Info("Failed to send error to Airbrake")
-  }
-
-  return nil
-}
-
-// `Levels()` returns a slice of `Levels` the hook is fired for.
-func (hook *AirbrakeHook) Levels() []log.Level {
-  return []log.Level{
-    log.ErrorLevel,
-    log.FatalLevel,
-    log.PanicLevel,
-  }
-}
-```
-
-Logrus comes with built-in hooks. Add those, or your custom hook, in `init`:
+Logrus comes with [built-in hooks](hooks/). Add those, or your custom hook, in
+`init`:
 
 ```go
 import (
@@ -211,7 +176,7 @@ import (
 )
 
 func init() {
-  log.AddHook(new(logrus_airbrake.AirbrakeHook))
+  log.AddHook(airbrake.NewHook("https://example.com", "xyz", "development"))
 
   hook, err := logrus_syslog.NewSyslogHook("udp", "localhost:514", syslog.LOG_INFO, "")
   if err != nil {

examples/hook/hook.go

@@ -3,21 +3,16 @@ package main
 import (
 	"github.com/Sirupsen/logrus"
 	"github.com/Sirupsen/logrus/hooks/airbrake"
-	"github.com/tobi/airbrake-go"
 )
 
 var log = logrus.New()
 
 func init() {
 	log.Formatter = new(logrus.TextFormatter) // default
-	log.Hooks.Add(new(logrus_airbrake.AirbrakeHook))
+	log.Hooks.Add(airbrake.NewHook("https://example.com", "xyz", "development"))
 }
 
 func main() {
-	airbrake.Endpoint = "https://exceptions.whatever.com/notifier_api/v2/notices.xml"
-	airbrake.ApiKey = "whatever"
-	airbrake.Environment = "production"
-
 	log.WithFields(logrus.Fields{
 		"animal": "walrus",
 		"size":   10,

hooks/airbrake/airbrake.go

@@ -1,51 +1,51 @@
-package logrus_airbrake
+package airbrake
 
 import (
+	"errors"
+	"fmt"
+
 	"github.com/Sirupsen/logrus"
 	"github.com/tobi/airbrake-go"
 )
 
 // AirbrakeHook to send exceptions to an exception-tracking service compatible
-// with the Airbrake API. You must set:
-// * airbrake.Endpoint
-// * airbrake.ApiKey
-// * airbrake.Environment
-//
-// Before using this hook, to send an error. Entries that trigger an Error,
-// Fatal or Panic should now include an "error" field to send to Airbrake.
-type AirbrakeHook struct{}
+// with the Airbrake API.
+type airbrakeHook struct {
+	APIKey      string
+	Endpoint    string
+	Environment string
+}
 
-func (hook *AirbrakeHook) Fire(entry *logrus.Entry) error {
-	if entry.Data["error"] == nil {
-		entry.Logger.WithFields(logrus.Fields{
-			"source":   "airbrake",
-			"endpoint": airbrake.Endpoint,
-		}).Warn("Exceptions sent to Airbrake must have an 'error' key with the error")
-		return nil
+func NewHook(endpoint, apiKey, env string) *airbrakeHook {
+	return &airbrakeHook{
+		APIKey:      apiKey,
+		Endpoint:    endpoint,
+		Environment: env,
 	}
+}
 
+func (hook *airbrakeHook) Fire(entry *logrus.Entry) error {
+	airbrake.ApiKey = hook.APIKey
+	airbrake.Endpoint = hook.Endpoint
+	airbrake.Environment = hook.Environment
+
+	var notifyErr error
 	err, ok := entry.Data["error"].(error)
-	if !ok {
-		entry.Logger.WithFields(logrus.Fields{
-			"source":   "airbrake",
-			"endpoint": airbrake.Endpoint,
-		}).Warn("Exceptions sent to Airbrake must have an `error` key of type `error`")
-		return nil
+	if ok {
+		notifyErr = err
+	} else {
+		notifyErr = errors.New(entry.Message)
 	}
 
-	airErr := airbrake.Notify(err)
+	airErr := airbrake.Notify(notifyErr)
 	if airErr != nil {
-		entry.Logger.WithFields(logrus.Fields{
-			"source":   "airbrake",
-			"endpoint": airbrake.Endpoint,
-			"error":    airErr,
-		}).Warn("Failed to send error to Airbrake")
+		return fmt.Errorf("Failed to send error to Airbrake: %s", airErr)
 	}
 
 	return nil
 }
 
-func (hook *AirbrakeHook) Levels() []logrus.Level {
+func (hook *airbrakeHook) Levels() []logrus.Level {
 	return []logrus.Level{
 		logrus.ErrorLevel,
 		logrus.FatalLevel,

hooks/airbrake/airbrake_test.go

@@ -1,27 +1,124 @@
-package logrus_airbrake
+package airbrake
 
 import (
 	"encoding/xml"
-	"errors"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 	"time"
 
 	"github.com/Sirupsen/logrus"
-	"github.com/tobi/airbrake-go"
 )
 
 type notice struct {
-	Error struct {
-		Message string `xml:"message"`
-	} `xml:"error"`
+	Error NoticeError `xml:"error"`
+}
+type NoticeError struct {
+	Class   string `xml:"class"`
+	Message string `xml:"message"`
 }
 
-func TestNoticeReceived(t *testing.T) {
-	msg := make(chan string, 1)
-	expectedMsg := "foo"
+type customErr struct {
+	msg string
+}
 
+func (e *customErr) Error() string {
+	return e.msg
+}
+
+const (
+	testAPIKey    = "abcxyz"
+	testEnv       = "development"
+	expectedClass = "*airbrake.customErr"
+	expectedMsg   = "foo"
+	unintendedMsg = "Airbrake will not see this string"
+)
+
+var (
+	noticeError = make(chan NoticeError, 1)
+)
+
+// TestLogEntryMessageReceived checks if invoking Logrus' log.Error
+// method causes an XML payload containing the log entry message is received
+// by a HTTP server emulating an Airbrake-compatible endpoint.
+func TestLogEntryMessageReceived(t *testing.T) {
+	log := logrus.New()
+	ts := startAirbrakeServer(t)
+	defer ts.Close()
+
+	hook := NewHook(ts.URL, testAPIKey, "production")
+	log.Hooks.Add(hook)
+
+	log.Error(expectedMsg)
+
+	select {
+	case received := <-noticeError:
+		if received.Message != expectedMsg {
+			t.Errorf("Unexpected message received: %s", received.Message)
+		}
+	case <-time.After(time.Second):
+		t.Error("Timed out; no notice received by Airbrake API")
+	}
+}
+
+// TestLogEntryMessageReceived confirms that, when passing an error type using
+// logrus.Fields, a HTTP server emulating an Airbrake endpoint receives the
+// error message returned by the Error() method on the error interface
+// rather than the logrus.Entry.Message string.
+func TestLogEntryWithErrorReceived(t *testing.T) {
+	log := logrus.New()
+	ts := startAirbrakeServer(t)
+	defer ts.Close()
+
+	hook := NewHook(ts.URL, testAPIKey, "production")
+	log.Hooks.Add(hook)
+
+	log.WithFields(logrus.Fields{
+		"error": &customErr{expectedMsg},
+	}).Error(unintendedMsg)
+
+	select {
+	case received := <-noticeError:
+		if received.Message != expectedMsg {
+			t.Errorf("Unexpected message received: %s", received.Message)
+		}
+		if received.Class != expectedClass {
+			t.Errorf("Unexpected error class: %s", received.Class)
+		}
+	case <-time.After(time.Second):
+		t.Error("Timed out; no notice received by Airbrake API")
+	}
+}
+
+// TestLogEntryWithNonErrorTypeNotReceived confirms that, when passing a
+// non-error type using logrus.Fields, a HTTP server emulating an Airbrake
+// endpoint receives the logrus.Entry.Message string.
+//
+// Only error types are supported when setting the 'error' field using
+// logrus.WithFields().
+func TestLogEntryWithNonErrorTypeNotReceived(t *testing.T) {
+	log := logrus.New()
+	ts := startAirbrakeServer(t)
+	defer ts.Close()
+
+	hook := NewHook(ts.URL, testAPIKey, "production")
+	log.Hooks.Add(hook)
+
+	log.WithFields(logrus.Fields{
+		"error": expectedMsg,
+	}).Error(unintendedMsg)
+
+	select {
+	case received := <-noticeError:
+		if received.Message != unintendedMsg {
+			t.Errorf("Unexpected message received: %s", received.Message)
+		}
+	case <-time.After(time.Second):
+		t.Error("Timed out; no notice received by Airbrake API")
+	}
+}
+
+func startAirbrakeServer(t *testing.T) *httptest.Server {
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		var notice notice
 		if err := xml.NewDecoder(r.Body).Decode(&notice); err != nil {
@@ -29,29 +126,8 @@ func TestNoticeReceived(t *testing.T) {
 		}
 		r.Body.Close()
 
-		msg <- notice.Error.Message
+		noticeError <- notice.Error
 	}))
-	defer ts.Close()
 
-	hook := &AirbrakeHook{}
-
-	airbrake.Environment = "production"
-	airbrake.Endpoint = ts.URL
-	airbrake.ApiKey = "foo"
-
-	log := logrus.New()
-	log.Hooks.Add(hook)
-
-	log.WithFields(logrus.Fields{
-		"error": errors.New(expectedMsg),
-	}).Error("Airbrake will not see this string")
-
-	select {
-	case received := <-msg:
-		if received != expectedMsg {
-			t.Errorf("Unexpected message received: %s", received)
-		}
-	case <-time.After(time.Second):
-		t.Error("Timed out; no notice received by Airbrake API")
-	}
+	return ts
 }