feat: add error reporters and JSON log formatter#121
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes error reporting and adds structured log formatting: introduces Pgbus::ErrorReporter and Pgbus::LogFormatter, exposes configuration for Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker/Component
participant ErrRpt as Pgbus::ErrorReporter
participant Logger as Config.logger
participant Ext as ExternalReporter
Worker->>ErrRpt: ErrorReporter.report(error, ctx)
ErrRpt->>Logger: log_error(exception, ctx)
ErrRpt->>Ext: call_handler(reporter, exception, ctx, config)
Alt reporter raises
Ext-->>ErrRpt: raises
ErrRpt->>Logger: logger.error("reporter failed", ...)
End
ErrRpt-->>Worker: returns (swallows exceptions)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/pgbus/circuit_breaker.rb`:
- Line 95: The rescue path in circuit_breaker.rb calls ErrorReporter.report
(line with ErrorReporter.report(e, { action: "circuit_breaker_trip", queue:
queue_name })) which can itself raise and break fault tolerance; wrap that call
in its own begin/rescue that catches StandardError (or Exception as your policy)
and logs the failure via a local logger/STDERR without re-raising, or update
ErrorReporter.report to defensively rescue and swallow/report its internal
errors (e.g., add an internal begin/rescue around callback/logger usage inside
ErrorReporter.report); ensure the circuit breaker rescue path never propagates
exceptions from the reporter.
In `@lib/pgbus/log_formatter.rb`:
- Around line 46-48: Extract the duplicated tid logic into a single shared
helper and call it from both formatters: create a private module method (e.g.,
in a module TidHelper or the parent LogFormatter class) that implements the
current Thread.current[:pgbus_tid] ||= (Thread.current.object_id ^
::Process.pid).to_s(36) logic, remove the duplicate tid definitions from the
Text and JSON formatter classes, and have Text#tid and JSON#tid delegate to the
shared helper (or include the module) so the same unique identifier logic is
centralized and reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0737f647-4691-4e9a-8b5c-c3aa51de7846
📒 Files selected for processing (14)
lib/pgbus/active_job/executor.rblib/pgbus/circuit_breaker.rblib/pgbus/configuration.rblib/pgbus/error_reporter.rblib/pgbus/failed_event_recorder.rblib/pgbus/log_formatter.rblib/pgbus/outbox/poller.rblib/pgbus/process/dispatcher.rblib/pgbus/process/supervisor.rblib/pgbus/process/worker.rbspec/pgbus/configuration_error_reporters_spec.rbspec/pgbus/configuration_log_format_spec.rbspec/pgbus/error_reporter_spec.rbspec/pgbus/log_formatter_spec.rb
Add Pgbus::ErrorReporter module and config.error_reporters array so users
can route caught exceptions to APM services (Appsignal, Sentry, etc.)
instead of only logging them.
Configuration:
Pgbus.configure do |c|
c.error_reporters << ->(ex, ctx) { Appsignal.set_error(ex) }
end
Wired into all critical rescue sites: executor job failures, worker
fetch/process errors, dispatcher maintenance, circuit breaker trips,
supervisor fork failures, outbox publish errors, and failed event
recording. Non-critical paths (dashboard queries, stat recording,
debug-level defensive catches) intentionally left as log-only.
Inspired by Sidekiq's error_handlers pattern — each reporter receives
(exception, context_hash) or optionally (exception, context_hash, config).
Add Pgbus::LogFormatter module with Text and JSON formatters, inspired
by Sidekiq::Logger::Formatters. The JSON formatter outputs structured
log lines with separate fields for timestamp, pid, tid, severity,
message, component, and optional thread-local context.
Configuration:
Pgbus.configure do |c|
c.log_format = :json # switches logger formatter to JSON
end
The JSON formatter extracts [Pgbus] and [Pgbus::Web] prefixes from
messages into a "component" field, keeping the msg field clean. Thread-
local context (via LogFormatter.with_context) appears under "ctx".
Also adds LogFormatter::Text for consistent text output with pid/tid.
- Wrap ErrorReporter.report in outer rescue Exception to guarantee it never raises — callers sit inside rescue blocks where propagation would break fault-tolerance invariants - Extract duplicated tid helper to LogFormatter module-level method shared by both Text and JSON formatters - Add test proving report swallows logger failures
14cf3ee to
c180994
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/pgbus/active_job/executor.rb`:
- Around line 149-151: The context hash `ctx` in Executor#... currently contains
queue, job_class, msg_id, and read_ct but needs an explicit action key for
consistency; update the `ctx = { ... }` construction (the `ctx` variable created
near `queue_name`, `payload&.dig("job_class")`, `message.msg_id`,
`message.read_ct`) to include an `action` entry (e.g. action: "execute_job")
before calling `ErrorReporter.report(error, ctx)` so downstream
routing/filtering matches other reporter call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58e63167-6a10-4352-a5d1-9a6d82a2155e
📒 Files selected for processing (14)
lib/pgbus/active_job/executor.rblib/pgbus/circuit_breaker.rblib/pgbus/configuration.rblib/pgbus/error_reporter.rblib/pgbus/failed_event_recorder.rblib/pgbus/log_formatter.rblib/pgbus/outbox/poller.rblib/pgbus/process/dispatcher.rblib/pgbus/process/supervisor.rblib/pgbus/process/worker.rbspec/pgbus/configuration_error_reporters_spec.rbspec/pgbus/configuration_log_format_spec.rbspec/pgbus/error_reporter_spec.rbspec/pgbus/log_formatter_spec.rb
All other ErrorReporter.report call sites include an action: key.
Summary
Pgbus::ErrorReporter): Configurable array of callable error reporters (config.error_reporters) that receive(exception, context_hash)at every critical rescue site. Inspired by Sidekiq'serror_handlerspattern. Lets users route exceptions to APM services (Appsignal, Sentry, Honeybadger, etc.) instead of only logging them.Pgbus::LogFormatter::JSON): Structured JSON log output with separate fields forts,pid,tid,lvl,msg,component, and optionalctx. Extracts[Pgbus]/[Pgbus::Web]prefixes into a dedicatedcomponentfield somsgstays clean. Also addsPgbus::LogFormatter::Textfor consistent text output.Pgbus::LogFormatter.with_context): Thread-local context hash that appears in JSON output underctx, similar toSidekiq::Context.Error Reporter Configuration
JSON Logging Configuration
Wired Into
Error reporters are called at all critical rescue sites:
Non-critical paths (dashboard queries, stat recording, debug-level catches) remain log-only.
Test plan
bundle exec rspec— 1632 examples, 0 failures (excluding pre-existing system test failures)bundle exec rubocop— no offensesconfig.log_format = :jsonSummary by CodeRabbit
New Features
Tests