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

Add ActiveSupport::Notification to Channel::Base#perform_action #23723

Merged
merged 1 commit into from Mar 30, 2016

Conversation

Projects
None yet
4 participants
@mwear
Contributor

mwear commented Feb 16, 2016

It would be nice for ActionCable::Channel::Base#perform_action to have an ActiveSupport::Notifications instrumentation hook similar to ActionController#process_action
This PR adds an ActiveSupport::Notification hook to ActionCable Channel::Base#perform_action. The payload contains the action and the data passed to perform action.

@rails-bot

This comment has been minimized.

rails-bot commented Feb 16, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@maclover7

View changes

actioncable/lib/action_cable/channel/base.rb Outdated
dispatch_action(action, data)
else
logger.error "Unable to process #{action_signature(action, data)}"
ActiveSupport::Notifications.instrument("perform_action.action_cable", action: action, data: data) do

This comment has been minimized.

@maclover7

maclover7 Feb 17, 2016

Member

This should go inside the conditional, right above the dispatch method. Wouldn't we only want to send out notifications for actions that can be processed?

This comment has been minimized.

@mwear

mwear Feb 17, 2016

Contributor

It's updated. Thanks for pointing that out!

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Feb 17, 2016

Since you are here could you do the same in the transmit* calls? and move the logger.info calls to a LogSubscriber?

@mwear

This comment has been minimized.

Contributor

mwear commented Feb 17, 2016

Sure, I'd be happy to @rafaelfranca.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Feb 17, 2016

Thank you so much!

@rafaelfranca

View changes

actioncable/test/channel/base_test.rb Outdated
@@ -166,6 +166,21 @@ def rm_rf
end
end
test "notification for perform_action" do
events = []
ActiveSupport::Notifications.subscribe "perform_action.action_cable" do |*args|

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 17, 2016

Member

We need to unsubscribe in a ensure block.

@mwear mwear force-pushed the mwear:action_cable_notifications branch Feb 17, 2016

@mwear

This comment has been minimized.

Contributor

mwear commented Feb 18, 2016

It appears that the last commit did not trigger CI for some reason. The transmit* methods are instrumented and there is now a LogSubscriber for ActionCable::Channel. This is ready for feedback at your convenience.

@maclover7

This comment has been minimized.

Member

maclover7 commented Feb 18, 2016

Can you squash down your commits?

@mwear mwear force-pushed the mwear:action_cable_notifications branch 6 times, most recently Feb 18, 2016

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Feb 20, 2016

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Feb 23, 2016

Awesome work!

@mwear mwear force-pushed the mwear:action_cable_notifications branch 3 times, most recently Feb 25, 2016

@maclover7

View changes

actioncable/lib/action_cable/channel/base.rb Outdated
@@ -160,7 +161,10 @@ def perform_action(data)
action = extract_action(data)
if processable_action?(action)
dispatch_action(action, data)
payload = {channel_class: self.class.name, action: action, data: data}

This comment has been minimized.

@maclover7

maclover7 Feb 25, 2016

Member

Can you add spaces and after the brackets?

{ channel_class: self.class.name, action: action, data: data }

@maclover7

View changes

actioncable/lib/action_cable/channel/base.rb Outdated
@@ -191,8 +195,10 @@ def unsubscribed
# Transmit a hash of data to the subscriber. The hash will automatically be wrapped in a JSON envelope with
# the proper channel identifier marked as the recipient.
def transmit(data, via: nil)
logger.info "#{self.class.name} transmitting #{data.inspect.truncate(300)}".tap { |m| m << " (via #{via})" if via }
connection.transmit ActiveSupport::JSON.encode(identifier: @identifier, message: data)
payload = {channel_class: self.class.name, data: data, via: via}

This comment has been minimized.

@maclover7

maclover7 Feb 25, 2016

Member

same as above, regarding the brackets

@maclover7

View changes

actioncable/lib/action_cable/channel/log_subscriber.rb Outdated
module ActionCable
module Channel
class LogSubscriber < ActiveSupport::LogSubscriber

This comment has been minimized.

@maclover7

maclover7 Feb 25, 2016

Member

you can remove this line

@maclover7

This comment has been minimized.

Member

maclover7 commented Feb 25, 2016

A few small code styling comments, otherwise this looks really good! :)

@mwear mwear force-pushed the mwear:action_cable_notifications branch Feb 25, 2016

@mwear

This comment has been minimized.

Contributor

mwear commented Feb 25, 2016

Thanks for the feedback! I've updated the PR with your comments.

Add AS::Notifications and LogSubscriber to ActionCable::Channel
This commit adds ActiveSupport::Notifications instrumentation hooks
and a LogSuscriber to ActionCable::Channel::Base.

@mwear mwear force-pushed the mwear:action_cable_notifications branch to 09a1321 Mar 4, 2016

@mwear

This comment has been minimized.

Contributor

mwear commented Mar 24, 2016

@rafaelfranca I was wondering if this is still targeted for 5.0.0?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Mar 24, 2016

Yes it is. I found some problems with how Action Cable logging interact with tagged logs and how it would work with the LoggerSubscriber and I was going to address it before merging. I probably will do it tomorrow.

@mwear

This comment has been minimized.

Contributor

mwear commented Mar 24, 2016

Great, thanks for the update!

rafaelfranca added a commit that referenced this pull request Mar 30, 2016

Merge pull request #23723 from mwear/action_cable_notifications
Add ActiveSupport::Notification to Channel::Base#perform_action

@rafaelfranca rafaelfranca merged commit 09a1321 into rails:master Mar 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment