Merge pull request #53 from scribd/laynax/SERF-1828/fix-sentry

[SERF-1828] Send sentry event via hook only of error is provided

Author: laynax

README.md

@@ -542,14 +542,15 @@ production:
   dsn: "https://<key>@sentry.io/<project>"
 ```
 
-The tracking can be enabled from the `Builder` with the `SetTracking`
-function:
+The tracking can be enabled from the `Logger Builder` with the `SetTracking`
+function. To trigger tracking, use `logger` with `WithError(err)` as follows:
 
 ```go
 package main
 
 import (
 	"log"
+	"errors"
 
 	sdklogger   "github.com/scribd/go-sdk/pkg/logger"
 	sdktracking "github.com/scribd/go-sdk/pkg/tracking"
@@ -573,6 +574,9 @@ func main() {
 	if Logger, err = sdklogger.NewBuilder(loggerConfig).SetTracking(trackingConfig).Build(); err != nil {
 		log.Fatalf("Failed to load SDK logger: %s", err.Error())
 	}
+
+	err = errors.New("new error")
+	logger.WithError(err).Errorf("trying sentry error functionality")
 ```
 
 A logger build with a valid tracking configuration will automatically

pkg/logger/logger.go

@@ -53,4 +53,6 @@ type Logger interface {
 	// Note that it doesn't log until you call Debug, Print, Info,
 	// Warn, Fatal or Panic on the Entry it returns.
 	WithFields(keyValues Fields) Logger
+	// WithError sets an error field value with logurs.Errorkey key.
+	WithError(err error) Logger
 }

pkg/logger/logrus.go

@@ -151,6 +151,13 @@ func (l *logrusLogEntry) WithFields(fields Fields) Logger {
 	}
 }
 
+// WithError sets an error field on logrus logger.
+func (l *logrusLogEntry) WithError(err error) Logger {
+	return &logrusLogEntry{
+		entry: l.entry.WithError(err),
+	}
+}
+
 // SetTracking configures and enables the error reporting.
 func (l *logrusLogEntry) setTracking(trackingConfig *tracking.Config) error {
 	hook, err := tracking.NewSentryHook(trackingConfig)

pkg/tracking/sentry.go

@@ -1,6 +1,8 @@
 package tracking
 
 import (
+	"fmt"
+
 	"github.com/getsentry/sentry-go"
 	"github.com/sirupsen/logrus"
 )
@@ -22,7 +24,6 @@ var (
 // It's used for sending errors and messages to Sentry on specific
 // log levels. It wraps a default Sentry client.
 type Hook struct {
-	client      *sentry.Client
 	levels      []logrus.Level
 	tags        map[string]string
 	release     string
@@ -38,21 +39,23 @@ func (hook *Hook) Levels() []logrus.Level {
 // Fire uses the configured Sentry client to report the given Logrus Entry as
 // Sentry Event.
 func (hook *Hook) Fire(entry *logrus.Entry) error {
-	exceptions := []sentry.Exception{}
-
-	if err, ok := entry.Data[logrus.ErrorKey].(error); ok && err != nil {
-		stacktrace := sentry.ExtractStacktrace(err)
-		if stacktrace == nil {
-			stacktrace = sentry.NewStacktrace()
-		}
-		exceptions = append(exceptions, sentry.Exception{
-			Type:       entry.Message,
-			Value:      err.Error(),
-			Stacktrace: stacktrace,
-		})
+	entryError, ok := entry.Data[logrus.ErrorKey].(error)
+	if !ok || entryError == nil {
+		return nil
+	}
+
+	stacktrace := sentry.ExtractStacktrace(entryError)
+	if stacktrace == nil {
+		stacktrace = sentry.NewStacktrace()
 	}
 
-	event := sentry.Event{
+	exceptions := []sentry.Exception{{
+		Type:       entry.Message,
+		Value:      entryError.Error(),
+		Stacktrace: stacktrace,
+	}}
+
+	event := &sentry.Event{
 		Level:       levelsMap[entry.Level],
 		Message:     entry.Message,
 		Extra:       map[string]interface{}(entry.Data),
@@ -62,8 +65,7 @@ func (hook *Hook) Fire(entry *logrus.Entry) error {
 		Exception:   exceptions,
 	}
 
-	hub := sentry.CurrentHub()
-	hook.client.CaptureEvent(&event, nil, hub.Scope())
+	sentry.CaptureEvent(event)
 
 	return nil
 }
@@ -92,15 +94,10 @@ func (hook *Hook) SetEnvironment(environment string) {
 }
 
 // NewSentryHook creates a hook to be added to an instance of logger
-// and initializes the Sentry client.
+// and initializes Sentry in package level. SentryHook will be triggered
+// on Panic, Fatal and Error levels.
 func NewSentryHook(config *Config) (*Hook, error) {
-	levels := []logrus.Level{
-		logrus.PanicLevel,
-		logrus.FatalLevel,
-		logrus.ErrorLevel,
-	}
-
-	client, err := sentry.NewClient(sentry.ClientOptions{
+	if err := sentry.Init(sentry.ClientOptions{
 		// The DSN to use. If the DSN is not set, the client is effectively disabled.
 		Dsn: config.SentryDSN,
 		// In debug mode, the debug information is printed to stdout to help you understand what
@@ -116,20 +113,18 @@ func NewSentryHook(config *Config) (*Hook, error) {
 		Release: config.release,
 		// The environment to be sent with events.
 		Environment: config.environment,
-	})
-	if err != nil {
-		return nil, err
+	}); err != nil {
+		return nil, fmt.Errorf("initializing sentry. err: %w", err)
 	}
 
-	hook := Hook{
-		client: client,
-		levels: levels,
-		tags:   map[string]string{},
-	}
-
-	if len(hook.levels) == 0 {
-		hook.levels = logrus.AllLevels
+	levels := []logrus.Level{
+		logrus.PanicLevel,
+		logrus.FatalLevel,
+		logrus.ErrorLevel,
 	}
 
-	return &hook, nil
+	return &Hook{
+		levels: levels,
+		tags:   map[string]string{},
+	}, nil
 }

pkg/tracking/sentry_test.go

@@ -1,13 +1,37 @@
 package tracking
 
 import (
+	"errors"
+	"io/ioutil"
 	"testing"
 
+	"github.com/getsentry/sentry-go"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 )
 
-const testSentryDSN = "https://[email protected]/0000000"
+const (
+	testSentryDSNValid   = "https://[email protected]/0000000"
+	testSentryDSNInvalid = "invalidSentryDSN"
+)
+
+func TestNewSentryHook(t *testing.T) {
+	hook, err := NewSentryHook(&Config{})
+	assert.NoError(t, err)
+
+	logger := newMockLogger(hook)
+
+	assert.Empty(t, sentry.LastEventID(),
+		"eventID must be empty without calling sentry hook levels")
+
+	logger.Errorf("sample message")
+	assert.Empty(t, sentry.LastEventID(),
+		"eventID must be empty without calling WithError")
+
+	logger.WithError(errors.New("sample error")).Errorf("sample message")
+	assert.NotEmpty(t, sentry.LastEventID(),
+		"last eventID must be set as expected")
+}
 
 func TestSentryHookLevels(t *testing.T) {
 	config := Config{}
@@ -17,19 +41,19 @@ func TestSentryHookLevels(t *testing.T) {
 }
 
 func TestSentryHookDsn(t *testing.T) {
-	config := Config{SentryDSN: testSentryDSN}
+	config := Config{SentryDSN: testSentryDSNValid}
 	_, err := NewSentryHook(&config)
 	assert.NoError(t, err)
 }
 
 func TestSentryHookErrorOnInvalidDsn(t *testing.T) {
-	config := Config{SentryDSN: "invalidSentryDSN"}
+	config := Config{SentryDSN: testSentryDSNInvalid}
 	_, err := NewSentryHook(&config)
 	assert.Error(t, err)
 }
 
 func TestSentryHookManualTag(t *testing.T) {
-	config := Config{SentryDSN: testSentryDSN}
+	config := Config{SentryDSN: testSentryDSNValid}
 	hook, err := NewSentryHook(&config)
 	assert.NoError(t, err)
 
@@ -46,3 +70,12 @@ func TestSentryHookManualTag(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, value, hook.tags[key])
 }
+
+func newMockLogger(hook *Hook) *logrus.Logger {
+	logger := logrus.New()
+
+	logger.SetOutput(ioutil.Discard)
+	logger.Hooks.Add(hook)
+
+	return logger
+}