Skip to content
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 support for Action Cable #257

Merged
merged 1 commit into from
Apr 13, 2019
Merged

Conversation

xlts
Copy link
Contributor

@xlts xlts commented Aug 28, 2018

Refactor log subscribers & create a new one to support logging of Action Cable events

Monkey-patch Action Cable server class to silence default Action Cable logger

Monkey-patch Action Cable Channel and Connection classes to support logging subscribe, unsubscribe, connect & disconnect events

Update log formatters to support Action Cable log data

@xlts
Copy link
Contributor Author

xlts commented Aug 28, 2018

Two important remarks here:

  1. I decided to silence the default Action Cable logger (defined in ActionCable::Server::Base) completely - the framework code is littered with multiple logger calls in plenty of classes. Monkey-patching it would be utterly painful and could leave the code extremely hard to maintain in case of a change in Action Cable itself. As a consequence, calling logger e.g. in user-defined Connection or Channel classes has no effect - a Rails.logger has to be used instead. Should this solution be accepted, we'd probably need to include a post-install message informing the developer about this state of things.

  2. Obviously Action Cable doesn't include a notion of a "controller", however for simplicity I decided to use this name to indicate both Channel or Connection classes in formatters. So if e.g. a pong action defined in MyChannel < ActionCable::Channel::Base is called, the Graylog log message would include the following: MyChannel#pong and the key of MyChannel value in Lograge data hash is still called controller.

Refactor log subscribers & create a new one to support logging of Action Cable events

Monkey-patch Action Cable server class to silence default Action Cable logger

Monkey-patch Action Cable Channel and Connection classes to support logging subscribe, unsubscribe, connect & disconnect events

Update log formatters to support Action Cable log data
@benlovell
Copy link
Collaborator

Thanks. This looks good at first glance - I'll give it a proper review over the weekend.

@xlts
Copy link
Contributor Author

xlts commented Jan 22, 2019

Thx @benlovell. Let me know if anything needs explaining

@nfedyashev
Copy link

@benlovell is there anything missing?

@xlts thanks for your work btw!

@xlts
Copy link
Contributor Author

xlts commented Apr 13, 2019

Thanks @nfedyashev! @benlovell are there chances of having this reviewed anytime soon?

@benlovell benlovell merged commit 2347f63 into roidrage:master Apr 13, 2019
@benlovell
Copy link
Collaborator

Thanks for this. Sorry for the delay! I’ll cut a release today or tomorrow. 💖

@xlts
Copy link
Contributor Author

xlts commented Apr 15, 2019

Cool! In the next few days I'll try to work on updates to Readme so that we have this documented & explained properly.

@benlovell
Copy link
Collaborator

benlovell commented Apr 15, 2019 via email

@jwhitcraft
Copy link

jwhitcraft commented Jun 21, 2019

Because of the Monkey-patch it breaks AnyCable see #294

@palkan
Copy link
Contributor

palkan commented Aug 28, 2019

Because of the Monkey-patch it breaks AnyCable see #294

Not only AnyCable is affected, Action Cable's functionality is also changed, compare these two lines, for example:
lograge


rails https://github.com/rails/rails/blob/a11e63f39907f828986f1832329c1b0530ca2aa3/actioncable/lib/action_cable/connection/base.rb#L178

@xlts @benlovell Please, consider replacing this MP with the one not-related on the internal implementation (e.g., using Module#prepend) and/or propose the required changes to Rails (I would be glad to help with making it merged).

@jwhitcraft A quick-fix should be smth like:

Lograge.class_eval do
  def self.attach_to_action_cable; end
end

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.

5 participants