-
Notifications
You must be signed in to change notification settings - Fork 22k
Structured event subscribers #55690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Structured event subscribers #55690
Changes from all commits
b8eb11e
d6cb07b
1a25283
163f7b7
ef36303
a486b99
333d51b
6f26cc8
e729f86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
# frozen_string_literal: true | ||
|
||
require "active_support/structured_event_subscriber" | ||
|
||
module ActionMailer | ||
class StructuredEventSubscriber < ActiveSupport::StructuredEventSubscriber # :nodoc: | ||
# An email was delivered. | ||
def deliver(event) | ||
if (exception = event.payload[:exception_object]) | ||
emit_debug_event("action_mailer.delivery_error", | ||
message_id: event.payload[:message_id], | ||
exception_class: exception.class.name, | ||
exception_message: exception.message, | ||
mail: event.payload[:mail], | ||
) | ||
elsif event.payload[:perform_deliveries] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why have separate events for what was the same notficiation? Are we going to publicly document these events, and their payloads (as stable) as their AS::Notification counterparts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a new guide next to the AS instrumentation one "Active Support Structured Events" could make sense. But once we document them, making changes are more difficult, so good to get feedback on the events being added here first. Maybe they could stay experimental and undocumented for now? The other thing, now we have essentially two places to maintain the same basic "event" from Rails. So if someone wants to add a new field to the payload to How we have CI to tell us if a new config flag is missing from the configuring guide, I always thought it'd be nice to have for notifications as well, that might help. There is some benefit to keeping the event names and payloads consistent between the two, but I also can appreciate the chance to make them better before people are depending on them. I don't have a strong opinion on this type of change, just wanted to call it out as a question. Keep up the good work! :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing events in two places is unfortunate, but because notification events can have non-serializable data, and structured events should be serializable, there's likely going to be a translation layer needed for all of these regardless. In terms of the original discussion about the multiple IFs, I will refactor these instances to a singular event being emitted with variable payload content to mirror related notification events. Thanks again for all your feedback! |
||
emit_debug_event("action_mailer.delivered", | ||
message_id: event.payload[:message_id], | ||
duration: event.duration.round(1), | ||
mail: event.payload[:mail], | ||
) | ||
else | ||
emit_debug_event("action_mailer.delivery_skipped", | ||
message_id: event.payload[:message_id], | ||
mail: event.payload[:mail], | ||
) | ||
end | ||
end | ||
debug_only :deliver | ||
|
||
# An email was generated. | ||
def process(event) | ||
emit_debug_event("action_mailer.processed", | ||
mailer: event.payload[:mailer], | ||
action: event.payload[:action], | ||
duration: event.duration.round(1), | ||
) | ||
end | ||
debug_only :process | ||
end | ||
end | ||
|
||
ActionMailer::StructuredEventSubscriber.attach_to :action_mailer |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
# frozen_string_literal: true | ||
|
||
require "abstract_unit" | ||
require "active_support/testing/event_reporter_assertions" | ||
require "mailers/base_mailer" | ||
require "action_mailer/structured_event_subscriber" | ||
|
||
module ActionMailer | ||
class StructuredEventSubscriberTest < ActionMailer::TestCase | ||
include ActiveSupport::Testing::EventReporterAssertions | ||
|
||
class BogusDelivery | ||
def initialize(*) | ||
end | ||
|
||
def deliver!(mail) | ||
raise "failed" | ||
end | ||
end | ||
|
||
def run(*) | ||
ActiveSupport.event_reporter.with_debug do | ||
super | ||
end | ||
end | ||
|
||
def test_deliver_is_notified | ||
event = assert_event_reported("action_mailer.delivered", payload: { message_id: "123@abc", mail: /.*/ }) do | ||
BaseMailer.welcome(message_id: "123@abc").deliver_now | ||
end | ||
|
||
assert event[:payload][:duration] > 0 | ||
ensure | ||
BaseMailer.deliveries.clear | ||
end | ||
|
||
def test_deliver_message_when_perform_deliveries_is_false | ||
assert_event_reported("action_mailer.delivery_skipped", payload: { message_id: "123@abc", mail: /.*/ }) do | ||
BaseMailer.welcome_without_deliveries(message_id: "123@abc").deliver_now | ||
end | ||
ensure | ||
BaseMailer.deliveries.clear | ||
end | ||
|
||
def test_deliver_message_when_exception_happened | ||
previous_delivery_method = BaseMailer.delivery_method | ||
BaseMailer.delivery_method = BogusDelivery | ||
payload = { message_id: "123@abc", mail: /.*/, exception_class: "RuntimeError", exception_message: "failed" } | ||
|
||
assert_event_reported("action_mailer.delivery_error", payload:) do | ||
assert_raises(RuntimeError) { BaseMailer.welcome(message_id: "123@abc").deliver_now } | ||
end | ||
ensure | ||
BaseMailer.delivery_method = previous_delivery_method | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
# frozen_string_literal: true | ||
|
||
module ActionController | ||
class StructuredEventSubscriber < ActiveSupport::StructuredEventSubscriber # :nodoc: | ||
INTERNAL_PARAMS = %w(controller action format _method only_path) | ||
|
||
def start_processing(event) | ||
payload = event.payload | ||
params = {} | ||
payload[:params].each_pair do |k, v| | ||
params[k] = v unless INTERNAL_PARAMS.include?(k) | ||
end | ||
format = payload[:format] | ||
format = format.to_s.upcase if format.is_a?(Symbol) | ||
format = "*/*" if format.nil? | ||
|
||
emit_event("action_controller.request_started", | ||
controller: payload[:controller], | ||
action: payload[:action], | ||
format:, | ||
params:, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If anyone is wondering, params for this event and others in this file are pre-filtered by Action Pack. They show up like this: |
||
) | ||
end | ||
|
||
def process_action(event) | ||
payload = event.payload | ||
status = payload[:status] | ||
|
||
if status.nil? && (exception_class_name = payload[:exception]&.first) | ||
status = ActionDispatch::ExceptionWrapper.status_code_for_exception(exception_class_name) | ||
end | ||
|
||
emit_event("action_controller.request_completed", { | ||
controller: payload[:controller], | ||
action: payload[:action], | ||
status: status, | ||
duration_ms: event.duration.round(2), | ||
gc_time_ms: event.gc_time.round(1), | ||
}.compact) | ||
end | ||
|
||
def halted_callback(event) | ||
emit_event("action_controller.callback_halted", filter: event.payload[:filter]) | ||
end | ||
|
||
def rescue_from_callback(event) | ||
exception = event.payload[:exception] | ||
emit_event("action_controller.rescue_from_handled", | ||
exception_class: exception.class.name, | ||
exception_message: exception.message, | ||
exception_backtrace: exception.backtrace&.first&.delete_prefix("#{Rails.root}/") | ||
) | ||
end | ||
|
||
def send_file(event) | ||
emit_event("action_controller.file_sent", path: event.payload[:path], duration_ms: event.duration.round(1)) | ||
end | ||
|
||
def redirect_to(event) | ||
emit_event("action_controller.redirected", location: event.payload[:location]) | ||
end | ||
|
||
def send_data(event) | ||
emit_event("action_controller.data_sent", filename: event.payload[:filename], duration_ms: event.duration.round(1)) | ||
end | ||
|
||
def unpermitted_parameters(event) | ||
unpermitted_keys = event.payload[:keys] | ||
context = event.payload[:context] | ||
|
||
params = {} | ||
context[:params].each_pair do |k, v| | ||
params[k] = v unless INTERNAL_PARAMS.include?(k) | ||
end | ||
|
||
emit_debug_event("action_controller.unpermitted_parameters", | ||
controller: context[:controller], | ||
action: context[:action], | ||
unpermitted_keys:, | ||
params: | ||
) | ||
end | ||
debug_only :unpermitted_parameters | ||
|
||
%w(write_fragment read_fragment exist_fragment? expire_fragment).each do |method| | ||
class_eval <<-METHOD, __FILE__, __LINE__ + 1 | ||
# frozen_string_literal: true | ||
def #{method}(event) | ||
return unless ActionController::Base.enable_fragment_cache_logging | ||
key = ActiveSupport::Cache.expand_cache_key(event.payload[:key] || event.payload[:path]) | ||
emit_event("action_controller.fragment_cache", | ||
method: "#{method}", | ||
key: key, | ||
duration_ms: event.duration.round(1) | ||
) | ||
end | ||
METHOD | ||
end | ||
end | ||
end | ||
|
||
ActionController::StructuredEventSubscriber.attach_to :action_controller |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# frozen_string_literal: true | ||
|
||
module ActionDispatch | ||
class StructuredEventSubscriber < ActiveSupport::StructuredEventSubscriber # :nodoc: | ||
def redirect(event) | ||
payload = event.payload | ||
status = payload[:status] | ||
|
||
emit_event("action_dispatch.redirect", { | ||
location: payload[:location], | ||
status: status, | ||
status_name: Rack::Utils::HTTP_STATUS_CODES[status], | ||
duration_ms: event.duration.round(2) | ||
}) | ||
end | ||
end | ||
end | ||
|
||
ActionDispatch::StructuredEventSubscriber.attach_to :action_dispatch |
Uh oh!
There was an error while loading. Please reload this page.