From d96cee72fa69ef226c2d3c1594a829aa539cd902 Mon Sep 17 00:00:00 2001
From: Burke Libbey <burke@libbey.me>
Date: Thu, 19 Mar 2015 11:17:22 -0400
Subject: [PATCH] Code review changes

---
 README.md                     |  6 ++++
 hooks/bugsnag/bugsnag.go      | 65 +++++++++++++++++++++--------------
 hooks/bugsnag/bugsnag_test.go |  2 +-
 3 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/README.md b/README.md
index e755e7c..e3b5afa 100644
--- a/README.md
+++ b/README.md
@@ -74,6 +74,7 @@ import (
   "os"
   log "github.com/Sirupsen/logrus"
   "github.com/Sirupsen/logrus/hooks/airbrake"
+  "github.com/Sirupsen/logrus/hooks/bugsnag"
 )
 
 func init() {
@@ -84,6 +85,11 @@ func init() {
   // an exception tracker. You can create custom hooks, see the Hooks section.
   log.AddHook(&logrus_airbrake.AirbrakeHook{})
 
+  // Use the Bugsnag hook to report errors that have Error severity or above to
+  // an exception tracker. You can create custom hooks, see the Hooks section.
+  bugsnagHook, _ = logrus_bugsnag.NewBugsnagHook()
+  log.AddHook(bugsnagHook)
+
   // Output to stderr instead of stdout, could also be a file.
   log.SetOutput(os.Stderr)
 
diff --git a/hooks/bugsnag/bugsnag.go b/hooks/bugsnag/bugsnag.go
index a0cfe84..d20a0f5 100644
--- a/hooks/bugsnag/bugsnag.go
+++ b/hooks/bugsnag/bugsnag.go
@@ -1,42 +1,57 @@
 package logrus_bugsnag
 
 import (
+	"errors"
+
 	"github.com/Sirupsen/logrus"
 	"github.com/bugsnag/bugsnag-go"
 )
 
-// BugsnagHook sends exceptions to an exception-tracking service compatible
-// with the Bugsnag API. Before using this hook, you must call
-// bugsnag.Configure().
+type bugsnagHook struct{}
+
+// ErrBugsnagUnconfigured is returned if NewBugsnagHook is called before
+// bugsnag.Configure. Bugsnag must be configured before the hook.
+var ErrBugsnagUnconfigured = errors.New("bugsnag must be configured before installing this logrus hook")
+
+// ErrBugsnagSendFailed indicates that the hook failed to submit an error to
+// bugsnag. The error was successfully generated, but `bugsnag.Notify()`
+// failed.
+type ErrBugsnagSendFailed struct {
+	err error
+}
+
+func (e ErrBugsnagSendFailed) Error() string {
+	return "failed to send error to Bugsnag: " + e.err.Error()
+}
+
+// NewBugsnagHook initializes a logrus hook which sends exceptions to an
+// exception-tracking service compatible with the Bugsnag API. Before using
+// this hook, you must call bugsnag.Configure(). The returned object should be
+// registered with a log via `AddHook()`
 //
 // Entries that trigger an Error, Fatal or Panic should now include an "error"
-// field to send to Bugsnag
-type BugsnagHook struct{}
+// field to send to Bugsnag.
+func NewBugsnagHook() (*bugsnagHook, error) {
+	if bugsnag.Config.APIKey == "" {
+		return nil, ErrBugsnagUnconfigured
+	}
+	return &bugsnagHook{}, nil
+}
 
 // Fire forwards an error to Bugsnag. Given a logrus.Entry, it extracts the
-// implicitly-required "error" field and sends it off.
-func (hook *BugsnagHook) Fire(entry *logrus.Entry) error {
-	if entry.Data["error"] == nil {
-		entry.Logger.WithFields(logrus.Fields{
-			"source": "bugsnag",
-		}).Warn("Exceptions sent to Bugsnag must have an 'error' key with the error")
-		return nil
-	}
-
+// "error" field (or the Message if the error isn't present) and sends it off.
+func (hook *bugsnagHook) Fire(entry *logrus.Entry) error {
+	var notifyErr error
 	err, ok := entry.Data["error"].(error)
-	if !ok {
-		entry.Logger.WithFields(logrus.Fields{
-			"source": "bugsnag",
-		}).Warn("Exceptions sent to Bugsnag must have an `error` key of type `error`")
-		return nil
+	if ok {
+		notifyErr = err
+	} else {
+		notifyErr = errors.New(entry.Message)
 	}
 
-	bugsnagErr := bugsnag.Notify(err)
+	bugsnagErr := bugsnag.Notify(notifyErr)
 	if bugsnagErr != nil {
-		entry.Logger.WithFields(logrus.Fields{
-			"source": "bugsnag",
-			"error":  bugsnagErr,
-		}).Warn("Failed to send error to Bugsnag")
+		return ErrBugsnagSendFailed{bugsnagErr}
 	}
 
 	return nil
@@ -44,7 +59,7 @@ func (hook *BugsnagHook) Fire(entry *logrus.Entry) error {
 
 // Levels enumerates the log levels on which the error should be forwarded to
 // bugsnag: everything at or above the "Error" level.
-func (hook *BugsnagHook) Levels() []logrus.Level {
+func (hook *bugsnagHook) Levels() []logrus.Level {
 	return []logrus.Level{
 		logrus.ErrorLevel,
 		logrus.FatalLevel,
diff --git a/hooks/bugsnag/bugsnag_test.go b/hooks/bugsnag/bugsnag_test.go
index 7db5136..e9ea298 100644
--- a/hooks/bugsnag/bugsnag_test.go
+++ b/hooks/bugsnag/bugsnag_test.go
@@ -37,7 +37,7 @@ func TestNoticeReceived(t *testing.T) {
 	}))
 	defer ts.Close()
 
-	hook := &BugsnagHook{}
+	hook := &bugsnagHook{}
 
 	bugsnag.Configure(bugsnag.Configuration{
 		Endpoint:     ts.URL,