feat: replace unmaintained rollbar and gobrake with modern versions#15
feat: replace unmaintained rollbar and gobrake with modern versions#15
Conversation
- Replace stvp/rollbar with rollbar/rollbar-go v1.4.8
- Use SetToken/SetEnvironment instead of direct variable assignment
- Use ErrorWithStackSkipWithExtras (auto-captures stack) instead of
manual stack conversion via ErrorWithStack + custom Frame types
- Remove convToRollbar() — no longer needed
- Replace gopkg.in/airbrake/gobrake.v2 with airbrake/gobrake/v5
- Use NewNotifierWithOptions instead of NewNotifier
Public API is fully backward compatible — no signature changes.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpgrades error-notification clients and related indirect deps: Rollbar and Airbrake libraries updated and notifier implementation adapted to their new APIs; new tests exercise initialization and notification paths. (46 words) Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the notifier integrations by replacing unmaintained/old Rollbar and Airbrake SDKs with their current maintained equivalents, while keeping the public notifier API stable.
Changes:
- Replace
stvp/rollbarwithrollbar/rollbar-goand update initialization + notification calls to the new API. - Replace
gopkg.in/airbrake/gobrake.v2withgithub.com/airbrake/gobrake/v5and update notifier initialization accordingly. - Remove the custom Rollbar stack conversion helper and rely on the new Rollbar SDK’s stack capture.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| notifier/notifier.go | Updates Rollbar/Airbrake SDK usage (init + notify paths) and removes Rollbar stack conversion helper. |
| go.mod | Switches direct dependencies to rollbar/rollbar-go and airbrake/gobrake/v5; updates indirect requirements after tidy. |
| go.sum | Reflects updated dependency graph from the Rollbar/Airbrake upgrades and go mod tidy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix import ordering (goimports-compatible) - Add tests for InitRollbar, InitAirbrake, SetEnvironment - Avoid rollbar-go global client race by testing notify paths with rollbar disabled (rollbar-go has internal async transport race)
|
Addressing review comments in 53b8547:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
notifier/notifier.go (1)
111-114: Seed Airbrake's init-time environment from the stored package env.
gobrake/v5exposesNotifierOptions.Environment, so wiring the already stored env intoNewNotifierWithOptionswould makeSetEnvironmentcalls that happen beforeInitAirbraketake effect without requiring callers to reapply them. (pkg.go.dev)♻️ Suggested follow-up
func InitAirbrake(projectID int64, projectKey string) { airbrake = gobrake.NewNotifierWithOptions(&gobrake.NotifierOptions{ ProjectId: projectID, ProjectKey: projectKey, + Environment: sentryEnvironment, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier.go` around lines 111 - 114, The Airbrake notifier is created without seeding the environment, so SetEnvironment calls made before InitAirbrake don't take effect; update the gobrake.NewNotifierWithOptions call that constructs airbrake to populate NotifierOptions.Environment using the stored env variable (the same env you currently persist and later set in SetEnvironment), i.e. add Environment: <storedEnv> to the NotifierOptions passed to NewNotifierWithOptions in InitAirbrake so early environment state is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notifier/notifier.go`:
- Around line 121-122: The Rollbar integration currently loses ErrorExt stack
frames; update InitRollbar() (or the Rollbar call sites that call
rollbar.ErrorWithStackSkipWithExtras) to convert ErrorExt's
StackFrame()/Callers() into Rollbar-compatible stack data and inject it into the
extras map (similar to convToGoBrake()), or alternatively implement a Stack()
[]runtime.Frame method on ErrorExt or call rollbar.SetStackTracer() with a
tracer that reads ErrorExt. Locate usages of ErrorWithStackSkipWithExtras and
ensure when err is an ErrorExt you extract err.StackFrame()/err.Callers(),
transform each frame into Rollbar's expected structure, and add them to the
extras argument so Rollbar receives the pre-captured frames.
---
Nitpick comments:
In `@notifier/notifier.go`:
- Around line 111-114: The Airbrake notifier is created without seeding the
environment, so SetEnvironment calls made before InitAirbrake don't take effect;
update the gobrake.NewNotifierWithOptions call that constructs airbrake to
populate NotifierOptions.Environment using the stored env variable (the same env
you currently persist and later set in SetEnvironment), i.e. add Environment:
<storedEnv> to the NotifierOptions passed to NewNotifierWithOptions in
InitAirbrake so early environment state is honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a71039da-b8fe-4191-b432-957782a49134
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modnotifier/notifier.go
- Register rollbar.SetStackTracer() in InitRollbar() to convert ErrorExt.Callers() to []runtime.Frame — rollbar now uses pre-captured stacks instead of calling runtime.Callers() (matches Airbrake/Sentry) - Seed Environment in InitAirbrake via NotifierOptions so SetEnvironment() calls before InitAirbrake() are honored - Rename TestRollbarNotifyOnPanicDoesNotPanic to TestNotifyOnPanicRecoverable - Add TestRollbarStackTracerExtractsErrorExtFrames - Add TestInitAirbrakeSeededEnvironment
There was a problem hiding this comment.
♻️ Duplicate comments (1)
notifier/notifier.go (1)
120-123:⚠️ Potential issue | 🟠 MajorRestore
ErrorExtstack propagation for Rollbar.The migration removed
convToRollbar()but doesn’t replace it withrollbar.SetStackTracer(). rollbar-go only reuses embedded stacks from errors implementing itsStackerinterface; otherwise its docs say it captures the stack where the error is reported. This package’s error API exposesCallers()/StackFrame()instead, so the Rollbar sends at Line 370 and Line 441 now fall back to notifier-site frames unless the concrete error types also implementStack() []runtime.Frame. (raw.githubusercontent.com)🔧 Suggested fix
func InitRollbar(token, env string) { rollbar.SetToken(token) rollbar.SetEnvironment(env) + rollbar.SetStackTracer(func(err error) ([]runtime.Frame, bool) { + if e, ok := err.(errors.ErrorExt); ok { + callers := runtime.CallersFrames(e.Callers()) + frames := make([]runtime.Frame, 0, len(e.Callers())) + for { + frame, more := callers.Next() + frames = append(frames, frame) + if !more { + break + } + } + return frames, true + } + return rollbar.DefaultStackTracer(err) + }) rollbarInited = true }Run this to verify there still isn’t a Rollbar-compatible stack adapter elsewhere in the repo:
#!/bin/bash set -euo pipefail echo "== rollbar stack tracer registrations ==" rg -n --type=go 'rollbar\.SetStackTracer\(' echo echo "== rollbar-go Stacker implementations ==" rg -nP --type=go 'func\s+\([^)]+\)\s+Stack\(\)\s+\[\]runtime\.Frame' echo echo "== Rollbar call sites using ErrorWithStackSkipWithExtras ==" rg -n --type=go 'ErrorWithStackSkipWithExtras\(' notifier/notifier.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier.go` around lines 120 - 123, InitRollbar currently only sets token/env and lost the Rollbar stack propagation; add a rollbar.SetStackTracer registration inside InitRollbar that provides an adapter converting this package's error type (e.g., ErrorExt or any type exposing Callers()/StackFrame()) to a rollbar.Stacker so Rollbar reuses embedded stacks instead of capturing the report-site stack; implement the adapter as a function referenced from InitRollbar (via rollbar.SetStackTracer) that inspects the error, extracts frames via Callers()/StackFrame() or ErrorExt methods, and returns a rollbar.Stacker-compatible value so existing Rollbar calls (the ones at the notifier's call sites) get the original error stack.
🧹 Nitpick comments (1)
notifier/notifier_rollbar_airbrake_test.go (1)
9-17: Assert the migrated Rollbar state, not just local flags / no-panics.These tests still pass if Line 121 or Line 473 stops calling Rollbar’s setters, because they only inspect
rollbarInitedor “does not panic”. rollbar-go exposesToken()andEnvironment()accessors on the managed client, so these paths can be checked directly. (pkg.go.dev)🧪 Suggested assertions
import ( "testing" "github.com/go-coldbrew/errors" + rollbar "github.com/rollbar/rollbar-go" ) func TestInitRollbar(t *testing.T) { rollbarInited = false - t.Cleanup(func() { rollbarInited = false }) + oldToken, oldEnv := rollbar.Token(), rollbar.Environment() + t.Cleanup(func() { + rollbarInited = false + rollbar.SetToken(oldToken) + rollbar.SetEnvironment(oldEnv) + }) InitRollbar("test-token", "test") if !rollbarInited { t.Error("expected rollbarInited to be true after InitRollbar") } + if got := rollbar.Token(); got != "test-token" { + t.Fatalf("expected rollbar token to be set, got %q", got) + } + if got := rollbar.Environment(); got != "test" { + t.Fatalf("expected rollbar environment to be set, got %q", got) + } } func TestSetEnvironmentWithAirbrake(t *testing.T) { old := airbrake - t.Cleanup(func() { airbrake = old }) + oldEnv := rollbar.Environment() + oldSentryEnv := sentryEnvironment + t.Cleanup(func() { + airbrake = old + rollbar.SetEnvironment(oldEnv) + sentryEnvironment = oldSentryEnv + }) InitAirbrake(12345, "test-key") - // Should not panic SetEnvironment("production") + if got := rollbar.Environment(); got != "production" { + t.Fatalf("expected rollbar environment to be updated, got %q", got) + } + if sentryEnvironment != "production" { + t.Fatalf("expected sentryEnvironment to be updated, got %q", sentryEnvironment) + } }Also applies to: 54-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier_rollbar_airbrake_test.go` around lines 9 - 17, Replace the brittle local-flag assertion in TestInitRollbar with assertions against the rollbar-go managed client: after calling InitRollbar("test-token", "test") call the rollbar package accessors (rollbar.Token() and rollbar.Environment()) and assert they equal "test-token" and "test" respectively, and keep resetting global state in t.Cleanup; apply the same change to the similar test at lines 54-60 so both validate the migrated Rollbar state rather than just rollbarInited or lack of panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@notifier/notifier.go`:
- Around line 120-123: InitRollbar currently only sets token/env and lost the
Rollbar stack propagation; add a rollbar.SetStackTracer registration inside
InitRollbar that provides an adapter converting this package's error type (e.g.,
ErrorExt or any type exposing Callers()/StackFrame()) to a rollbar.Stacker so
Rollbar reuses embedded stacks instead of capturing the report-site stack;
implement the adapter as a function referenced from InitRollbar (via
rollbar.SetStackTracer) that inspects the error, extracts frames via
Callers()/StackFrame() or ErrorExt methods, and returns a
rollbar.Stacker-compatible value so existing Rollbar calls (the ones at the
notifier's call sites) get the original error stack.
---
Nitpick comments:
In `@notifier/notifier_rollbar_airbrake_test.go`:
- Around line 9-17: Replace the brittle local-flag assertion in TestInitRollbar
with assertions against the rollbar-go managed client: after calling
InitRollbar("test-token", "test") call the rollbar package accessors
(rollbar.Token() and rollbar.Environment()) and assert they equal "test-token"
and "test" respectively, and keep resetting global state in t.Cleanup; apply the
same change to the similar test at lines 54-60 so both validate the migrated
Rollbar state rather than just rollbarInited or lack of panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76bf0d1e-9bb7-4bf4-b5ce-59a0be6a2fab
📒 Files selected for processing (2)
notifier/notifier.gonotifier/notifier_rollbar_airbrake_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Return (nil, false) when filtered frames slice is empty so rollbar falls back to its built-in runtime.Callers() capture - Reset rollbar global state (token, environment) in test cleanup to prevent state leaking between tests
Summary
stvp/rollbar(unmaintained) withrollbar/rollbar-gov1.4.8 (official, actively maintained)gopkg.in/airbrake/gobrake.v2(3 major versions behind) withairbrake/gobrake/v5convToRollbar()— new rollbar SDK captures stacks internally viaruntime.Callersgetsentry/raven-godependency removed bygo mod tidyPublic API is fully backward compatible — all exported function signatures unchanged.
Changes
stvp/rollbarrollbar/rollbar-gov1.4.8SetToken/SetEnvironmentinstead of direct var assignmentrollbar.ErrorWithStack+Fieldrollbar.ErrorWithStackSkipWithExtras+map[string]interface{}gobrake.NewNotifier(id, key)gobrake.NewNotifierWithOptions(&NotifierOptions{...})Test plan
make buildpassesmake testpasses (go test -race ./...)make lintpasses (golangci-lint 0 issues + govulncheck 0 vulnerabilities)go docrollbar.ERR/rollbar.CRITconstants have same string valuesSummary by CodeRabbit