From 874698954b8d02620020a1e6375ff36e1934dcf3 Mon Sep 17 00:00:00 2001 From: Luke Hansford Date: Mon, 2 Mar 2026 17:25:07 +0800 Subject: [PATCH] Add support for Sentry error reporting **Disclaimer**: This commit was mostly generated using Claude Code. I have reviewed all code changes myself, but would appreciate a review with consideration that some of code is generated. This adds support for reporting errors to Sentry alongside Bugsnag. --- AGENTS.md | 124 ++++++++++++++++++++++++++++++++++++++++++++++++ go.mod | 1 + go.sum | 2 + logging.go | 98 ++++++++++++++++++++++++++++++++++---- logging_test.go | 112 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 327 insertions(+), 10 deletions(-) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..f5c63e6 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,124 @@ +# AGENTS.md — logging-go + +## Overview + +Shared logging library for Fishbrain's Go services. Single-package Go module (`package logging`) that wraps [logrus](https://github.com/sirupsen/logrus) with Bugsnag error reporting, Sentry error reporting, Datadog trace correlation, and NSQ log-level bridging. + +**Module path**: `github.com/fishbrain/logging-go` + +## Commands + +| Task | Command | +|-------|------------------| +| Build | `go build ./...` | +| Test | `go test ./...` | + +There is no linter, formatter, or Makefile configured. CI (`go.yml`) runs `go build -v .` only — no test step in CI. + +## Project Structure + +``` +logging.go # All library code — types, logger init, entry helpers, Bugsnag/Sentry hooks +logging_test.go # All tests +go.mod / go.sum # Module definition (Go 1.24+, toolchain 1.26) +.tool-versions # asdf version pinning (go 1.26.0) +``` + +This is a **single-file library** — everything lives in `logging.go` and `logging_test.go`. No subdirectories, no `cmd/`, no `internal/`. + +## Architecture & Key Types + +### Global singleton + +`Init(LoggingConfig)` initializes the package-level `Log *Logger` variable. It is guarded by a nil check (not a `sync.Once`), so it only runs once. `TestMain` calls `Init(LoggingConfig{})` to set up the singleton before tests run. + +### Type hierarchy + +- **`Logger`** — wraps `*logrus.Logger`. Provides `WithField`, `WithError`, `WithDDTrace`, `NewEntry`, and `NSQLogger`. +- **`Entry`** — wraps `*logrus.Entry`. Provides domain-specific field helpers (`WithUser`, `WithEvent`, `WithChannel`, `WithDuration`, etc.) that return `*Entry` for chaining. +- **`NSQLogger`** — adaptor that implements `Output(int, string) error` so it can be passed to `nsq.SetLogger`. +- **`bugsnagHook`** — logrus hook that fires on Error/Fatal/Panic levels, forwarding to Bugsnag with metadata. +- **`sentryHook`** — logrus hook that fires on Error/Fatal/Panic levels, forwarding to Sentry with metadata and extra fields. + +### Initialization flow + +``` +Init(config) → + 1. bugsnag.Configure(...) — sets up Bugsnag client + 2. bugsnag.OnBeforeNotify(...) — unwraps *fmt.wrapError to get real error class + 3. sentry.Init(...) — sets up Sentry client (if SentryDSN is set and environment matches ErrorNotifyReleaseStages) + 4. Log = new(true, withSentry, config) — creates Logger with Bugsnag and optionally Sentry hooks attached +``` + +## Key Dependencies + +| Dependency | Purpose | +|---|---| +| `github.com/sirupsen/logrus` | Structured logging (JSON formatter) | +| `github.com/bugsnag/bugsnag-go/v2` | Error reporting to Bugsnag | +| `github.com/DataDog/dd-trace-go/v2` | Datadog APM trace/span ID injection | +| `github.com/getsentry/sentry-go` | Error reporting to Sentry | +| `github.com/nsqio/go-nsq` | NSQ message queue log-level bridging | +| `github.com/stretchr/testify` | Test assertions | + +## Code Patterns & Conventions + +### Fluent entry builder + +All `With*` methods return `*Entry` to support chaining: + +```go +Log.WithDDTrace(ctx).WithUser(userID).WithDuration(d).Info("processed request") +``` + +When adding new field helpers, follow this pattern: method on `*Entry`, return `*Entry`, delegate to `e.WithField(...)`. + +### Error wrapping + +Errors passed to `WithError` are wrapped with `bugsnag_errors.New(err, 1)` to capture stack traces. The `1` parameter controls stack frame skipping. The standalone `Errorf` and `ErrorWithStacktrace` functions also use this pattern. + +### JSON log output + +Logrus is configured with `JSONFormatter` and custom field mapping: +- `msg` → `message` +- `func` → `logger.method_name` +- `file` → `logger.name` +- `error` key → `error.message` +- Timestamp format: `RFC3339Nano` + +### Log levels + +The `LogLevel` config string must be uppercase: `"ERROR"`, `"WARNING"`, `"INFO"`, `"DEBUG"`. Unknown values default to `InfoLevel`. + +### NSQ log bridging + +`Logger.NSQLogger()` returns an `(NSQLogger, nsq.LogLevel)` tuple for plugging into `nsq.SetLogger`. The `NSQLogger.Output` method parses the 3-character prefix from NSQ log messages to route them to the correct logrus level. + +## Testing + +- **Framework**: stdlib `testing` + `testify/assert` +- **Setup**: `TestMain` initializes the global `Log` singleton via `Init(LoggingConfig{})` +- **Log capture**: Tests use `os.Pipe()` to capture log output by swapping `Log.Out`, then assert on the captured string content +- **Concurrency test**: `TestConcurrentUseOfEntry` verifies entries are safe for concurrent use across goroutines +- **Table-driven tests**: `TestGetLogrusLogLevel` uses a table-driven approach with a package-level test data slice +- **Sentry hook tests**: `TestSentryHookFire`, `TestSentryHookLevels`, `TestNewWithSentry`, and `TestNewWithoutSentry` cover the Sentry hook and its integration into the logger +- **Release-stage gating tests**: `TestShouldNotify` verifies the `shouldNotify` helper used for conditional Sentry/Bugsnag activation + +## Gotchas + +1. **No CI test step**: The GitHub Actions workflow builds but does not run tests. Running `go test ./...` locally is essential before pushing. +2. **Singleton guard is not sync.Once**: `Init` uses `if nil == Log` — safe for single-goroutine init, but not for concurrent callers. In practice this is fine since `Init` is called once at service startup. +3. **`ioutil.ReadAll` in tests**: Tests use the deprecated `io/ioutil` package. New code should use `io.ReadAll` instead. +4. **Bugsnag error unwrapping limit**: The `OnBeforeNotify` handler unwraps `*fmt.wrapError` chains up to 11 levels deep, then logs and stops. +5. **`logrus.ErrorKey` is mutated globally**: `new()` sets `logrus.ErrorKey = "error.message"` as a side effect — this affects all logrus loggers in the process, not just this one. +6. **Reversed nil check style**: The codebase uses Yoda conditions (`nil == Log`) in the `Init` function. +7. **`BugsnagNotifyReleaseStages` renamed**: The config field was renamed to `ErrorNotifyReleaseStages` and is now shared between Bugsnag and Sentry for release-stage gating. +8. **Sentry is conditional**: Sentry is only initialized when `SentryDSN` is non-empty and the current `Environment` is in `ErrorNotifyReleaseStages`. If `sentry.Init` fails, it logs to stderr and proceeds without the Sentry hook. + +## Releasing + +Create a GitHub Release. The module is imported by other Fishbrain Go services via its module path. Versioning follows Go module semantics (semver tags). + +## Ownership + +Owned by `@fishbrain/platform-team` (see `CODEOWNERS`). Dependency updates managed by Renovate (see `renovate.json`). diff --git a/go.mod b/go.mod index 5bd0326..7f49691 100644 --- a/go.mod +++ b/go.mod @@ -40,6 +40,7 @@ require ( github.com/dustin/go-humanize v1.0.1 // indirect github.com/eapache/queue/v2 v2.0.0-20230407133247-75960ed334e4 // indirect github.com/ebitengine/purego v0.8.4 // indirect + github.com/getsentry/sentry-go v0.43.0 // indirect github.com/go-logr/logr v1.4.3 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-ole/go-ole v1.3.0 // indirect diff --git a/go.sum b/go.sum index 3cd2220..28f93d4 100644 --- a/go.sum +++ b/go.sum @@ -122,6 +122,8 @@ github.com/ebitengine/purego v0.8.3 h1:K+0AjQp63JEZTEMZiwsI9g0+hAMNohwUOtY0RPGex github.com/ebitengine/purego v0.8.3/go.mod h1:iIjxzd6CiRiOG0UyXP+V1+jWqUXVjPKLAI0mRfJZTmQ= github.com/ebitengine/purego v0.8.4 h1:CF7LEKg5FFOsASUj0+QwaXf8Ht6TlFxg09+S9wz0omw= github.com/ebitengine/purego v0.8.4/go.mod h1:iIjxzd6CiRiOG0UyXP+V1+jWqUXVjPKLAI0mRfJZTmQ= +github.com/getsentry/sentry-go v0.43.0 h1:XbXLpFicpo8HmBDaInk7dum18G9KSLcjZiyUKS+hLW4= +github.com/getsentry/sentry-go v0.43.0/go.mod h1:XDotiNZbgf5U8bPDUAfvcFmOnMQQceESxyKaObSssW0= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY= github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= diff --git a/logging.go b/logging.go index 83a6d84..d4d665a 100644 --- a/logging.go +++ b/logging.go @@ -13,6 +13,7 @@ import ( "github.com/DataDog/dd-trace-go/v2/ddtrace/tracer" "github.com/bugsnag/bugsnag-go/v2" bugsnag_errors "github.com/bugsnag/bugsnag-go/v2/errors" + "github.com/getsentry/sentry-go" nsq "github.com/nsqio/go-nsq" "github.com/sirupsen/logrus" ) @@ -26,12 +27,13 @@ var ( ) type LoggingConfig struct { - LogLevel string - Environment string - AppVersion string - BugsnagAPIKey string - BugsnagNotifyReleaseStages []string - BugsnagProjectPackages []string + LogLevel string + Environment string + AppVersion string + BugsnagAPIKey string + ErrorNotifyReleaseStages []string + BugsnagProjectPackages []string + SentryDSN string } type Logger struct { @@ -40,6 +42,8 @@ type Logger struct { type bugsnagHook struct{} +type sentryHook struct{} + func (l Logger) getNSQLogLevel() nsq.LogLevel { switch l.Level { case logrus.DebugLevel: @@ -279,7 +283,53 @@ func (b *bugsnagHook) Levels() []logrus.Level { } } -func new(withBugsnag bool, config LoggingConfig) *Logger { +func (s *sentryHook) Fire(entry *logrus.Entry) error { + var notifyErr error + switch err := entry.Data[logrus.ErrorKey].(type) { + case *bugsnag_errors.Error: + notifyErr = err + case error: + if entry.Message != "" { + notifyErr = fmt.Errorf("%s: %w", entry.Message, err) + } else { + notifyErr = err + } + default: + notifyErr = fmt.Errorf("%s", entry.Message) + } + + event := sentry.NewEvent() + event.Level = sentry.LevelError + if entry.Level == logrus.FatalLevel { + event.Level = sentry.LevelFatal + } + event.Message = notifyErr.Error() + event.Exception = []sentry.Exception{{ + Type: reflect.TypeOf(notifyErr).String(), + Value: notifyErr.Error(), + }} + + extra := make(map[string]interface{}) + for key, val := range entry.Data { + if key != logrus.ErrorKey { + extra[key] = val + } + } + event.Extra = extra + + sentry.CaptureEvent(event) + return nil +} + +func (s *sentryHook) Levels() []logrus.Level { + return []logrus.Level{ + logrus.ErrorLevel, + logrus.FatalLevel, + logrus.PanicLevel, + } +} + +func new(withBugsnag bool, withSentry bool, config LoggingConfig) *Logger { log := logrus.New() logrus.ErrorKey = "error.message" log.Formatter = &logrus.JSONFormatter{ @@ -296,18 +346,31 @@ func new(withBugsnag bool, config LoggingConfig) *Logger { log.Hooks.Add(&bugsnagHook{}) } + if withSentry { + log.Hooks.Add(&sentryHook{}) + } + return &Logger{log} } +func shouldNotify(releaseStages []string, environment string) bool { + for _, stage := range releaseStages { + if stage == environment { + return true + } + } + return false +} + func Init(config LoggingConfig) { if nil == Log { bugsnag.Configure(bugsnag.Configuration{ APIKey: config.BugsnagAPIKey, ReleaseStage: config.Environment, AppVersion: config.AppVersion, - NotifyReleaseStages: config.BugsnagNotifyReleaseStages, + NotifyReleaseStages: config.ErrorNotifyReleaseStages, ProjectPackages: config.BugsnagProjectPackages, - Logger: stdlog.New(new(false, config).Writer(), "bugsnag: ", 0), + Logger: stdlog.New(new(false, false, config).Writer(), "bugsnag: ", 0), }) bugsnag.OnBeforeNotify( func(event *bugsnag.Event, config *bugsnag.Configuration) error { @@ -334,6 +397,21 @@ func Init(config LoggingConfig) { event.ErrorClass = errClass return nil }) - Log = new(true, config) + + withSentry := false + if config.SentryDSN != "" && shouldNotify(config.ErrorNotifyReleaseStages, config.Environment) { + err := sentry.Init(sentry.ClientOptions{ + Dsn: config.SentryDSN, + Environment: config.Environment, + Release: config.AppVersion, + }) + if err != nil { + stdlog.Printf("sentry.Init: %s", err) + } else { + withSentry = true + } + } + + Log = new(true, withSentry, config) } } diff --git a/logging_test.go b/logging_test.go index a590e13..129678e 100644 --- a/logging_test.go +++ b/logging_test.go @@ -1,6 +1,7 @@ package logging import ( + "fmt" "io/ioutil" "os" "strings" @@ -131,6 +132,117 @@ func TestWithStringFieldIgnoreEmpty(t *testing.T) { }) } +func TestSentryHookFire(t *testing.T) { + hook := &sentryHook{} + + t.Run("fires on error entry with error", func(t *testing.T) { + entry := &logrus.Entry{ + Level: logrus.ErrorLevel, + Message: "something went wrong", + Data: logrus.Fields{ + logrus.ErrorKey: fmt.Errorf("test error"), + }, + } + err := hook.Fire(entry) + assert.NoError(t, err) + }) + + t.Run("fires on error entry without error key", func(t *testing.T) { + entry := &logrus.Entry{ + Level: logrus.ErrorLevel, + Message: "something went wrong", + Data: logrus.Fields{}, + } + err := hook.Fire(entry) + assert.NoError(t, err) + }) + + t.Run("fires on fatal entry", func(t *testing.T) { + entry := &logrus.Entry{ + Level: logrus.FatalLevel, + Message: "fatal problem", + Data: logrus.Fields{ + logrus.ErrorKey: fmt.Errorf("fatal error"), + }, + } + err := hook.Fire(entry) + assert.NoError(t, err) + }) + + t.Run("includes metadata in extra", func(t *testing.T) { + entry := &logrus.Entry{ + Level: logrus.ErrorLevel, + Message: "error with metadata", + Data: logrus.Fields{ + logrus.ErrorKey: fmt.Errorf("test error"), + "user_id": 123, + "request_id": "abc-123", + }, + } + err := hook.Fire(entry) + assert.NoError(t, err) + }) +} + +func TestSentryHookLevels(t *testing.T) { + hook := &sentryHook{} + levels := hook.Levels() + assert.Contains(t, levels, logrus.ErrorLevel) + assert.Contains(t, levels, logrus.FatalLevel) + assert.Contains(t, levels, logrus.PanicLevel) + assert.NotContains(t, levels, logrus.WarnLevel) + assert.NotContains(t, levels, logrus.InfoLevel) + assert.NotContains(t, levels, logrus.DebugLevel) +} + +func TestShouldNotify(t *testing.T) { + t.Run("returns true when release stages is empty", func(t *testing.T) { + assert.True(t, shouldNotify([]string{}, "production")) + }) + + t.Run("returns true when environment matches", func(t *testing.T) { + assert.True(t, shouldNotify([]string{"production", "staging"}, "production")) + }) + + t.Run("returns false when environment does not match", func(t *testing.T) { + assert.False(t, shouldNotify([]string{"production", "staging"}, "development")) + }) + + t.Run("returns true when nil release stages", func(t *testing.T) { + assert.True(t, shouldNotify(nil, "production")) + }) +} + +func TestNewWithSentry(t *testing.T) { + logger := new(false, true, LoggingConfig{LogLevel: "INFO"}) + assert.NotNil(t, logger) + + hasSentryHook := false + for _, hooks := range logger.Hooks { + for _, hook := range hooks { + if _, ok := hook.(*sentryHook); ok { + hasSentryHook = true + } + } + } + assert.True(t, hasSentryHook, "logger should have sentry hook") +} + +func TestNewWithoutSentry(t *testing.T) { + logger := new(false, false, LoggingConfig{LogLevel: "INFO"}) + assert.NotNil(t, logger) + + hasSentryHook := false + for _, hooks := range logger.Hooks { + for _, hook := range hooks { + if _, ok := hook.(*sentryHook); ok { + hasSentryHook = true + } + } + } + assert.False(t, hasSentryHook, "logger should not have sentry hook") +} + func TestNSQLogger(t *testing.T) { Log.Level = logrus.DebugLevel _, level := Log.NSQLogger()