Skip to content

Migrate to ActiveSupport::Notifications - #16

Open
skatkov wants to merge 12 commits into
mainfrom
use-notifications
Open

Migrate to ActiveSupport::Notifications#16
skatkov wants to merge 12 commits into
mainfrom
use-notifications

Conversation

@skatkov

@skatkov skatkov commented Apr 30, 2026

Copy link
Copy Markdown
Owner

fixes #15

Refactors Webhukhs observability to emit a single ActiveSupport::Notifications event, webhukhs.event, instead of logging/reporting errors directly from the gem.

Event Payload Schema

{
  operation: :receive | :process,
  outcome: :duplicate | :unknown_handler | :inactive_handler | :error |
           :discarded | :skipped | :started | :completed | :validation_failed,
  severity: :info | :warn | :error,
  error: Exception,          # only present for error events
  service_id: String,        # when available
  handler_class: String,     # when available
  handler_event_id: String,  # when available
  webhook_id: Integer | String # when available
}

Features:

  • Initializer got updated with a proposed listener.
  • README adjusted, to ensure that people will initialize gem correctly
  • Configuration.error_context dropped

Using Rails.logger and Rails.errors directly is a tricky thing to manage. Would rather not try.

Rails.errors is only from Rails >= 7, so if we don't depend on it we can support older Rails apps easily.
Comment thread lib/webhukhs.rb
Comment thread lib/webhukhs/templates/webhukhs.rb Outdated
Comment thread lib/webhukhs/templates/webhukhs.rb Outdated
Comment thread test/webhukhs_test.rb Outdated
skatkov added 7 commits April 30, 2026 23:16
- Added Webhukhs::BaseHandler#to_s.
- Replaced handler.class.name usage in receive event payloads with handler.to_s.
- Updated initializer and README examples to check payload[:severity] == :error.
- Removed the redundant payload.fetch(:severity, :error) in the Rails.error example.
- Removed the reviewed error_context absence test.
@skatkov

skatkov commented Apr 30, 2026

Copy link
Copy Markdown
Owner Author

This is a breaking change. So before merging this... maybe @julik has a take on this? :-)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors Webhukhs observability to emit a single ActiveSupport::Notifications event (webhukhs.event) with structured payloads, replacing direct logging/error-reporting from within the gem and updating docs/tests accordingly.

Changes:

  • Introduces Webhukhs.instrument and replaces logging / Rails.error.report calls with structured webhukhs.event instrumentation.
  • Updates test suite helpers and assertions to validate emitted notification payloads instead of captured logs/error reports.
  • Updates documentation/changelog and adjusts CI/lint configuration related to docs and mutation testing.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
lib/webhukhs.rb Adds Webhukhs.instrument and requires active_support/notifications.
lib/webhukhs/controllers/receive_webhooks_controller.rb Emits webhukhs.event for receive-path errors instead of direct error reporting.
lib/webhukhs/jobs/processing_job.rb Emits processing lifecycle/error/discard events instead of logging/reporting.
lib/webhukhs/base_handler.rb Emits duplicate-delivery event; adjusts persisted handler name usage.
lib/webhukhs/models/received_webhook.rb Clarifies ensure_id comment regarding UUID assignment.
lib/webhukhs/templates/webhukhs.rb Adds notification subscriber example (currently active code in template).
README.md Updates install/setup steps and documents notification-based observability.
CHANGELOG.md Documents the notifications migration and removal of config.error_context.
test/test_helper.rb Replaces log-capture helper with captured_webhukhs_events notification capture.
test/webhukhs_test.rb Updates controller/processing assertions to validate notification payloads.
test/processing_job_test.rb Replaces log/error-report assertions with notification assertions for job behavior.
test/base_handler_test.rb Updates duplicate-delivery test to assert notification payload.
.yard-lint.yml Disables Documentation/UndocumentedObjects and normalizes quoting.
.github/workflows/mutant.yml Limits mutation tests workflow to pull_request events only.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .yard-lint.yml
Comment on lines 34 to +36
Documentation/UndocumentedObjects:
Description: 'Checks for classes, modules, and methods without documentation.'
Enabled: true
Description: "Checks for classes, modules, and methods without documentation."
Enabled: false

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mainly interested in yard tags for methods, so I can convert those into types. Object description is not that important.

Comment thread .github/workflows/mutant.yml
Comment thread test/webhukhs_test.rb
Comment thread lib/webhukhs/templates/webhukhs.rb
Comment thread lib/webhukhs/base_handler.rb
Comment thread lib/webhukhs/controllers/receive_webhooks_controller.rb Outdated
skatkov and others added 2 commits May 1, 2026 00:44
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@skatkov
skatkov force-pushed the use-notifications branch from 11d0446 to 183ca89 Compare April 30, 2026 22:58
arguments: job.arguments.map(&:inspect)
}, severity: :error)
discard_on ActiveJob::DeserializationError, InvalidWebhookArgument do |_job, error|
Webhukhs.instrument(operation: :process, outcome: :discarded, severity: :error, error: error)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably include "job" related attributes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get rid of dependency on Rails.logger and Rails.error emit ActiveSupport::Notifications events

2 participants