Capture exceptions in Action Cable connections and channels#1295
Capture exceptions in Action Cable connections and channels#1295agrobbin wants to merge 1 commit intogetsentry:masterfrom
Conversation
st0012
left a comment
There was a problem hiding this comment.
thanks for the PR 👍
I left some questions and it looks like there are some duplications?
| module Subscriptions | ||
| def self.included(base) | ||
| base.class_eval do | ||
| set_callback :subscribe, :around, ->(_, block) { sentry_capture(:subscribed, &block) }, prepend: true |
There was a problem hiding this comment.
why don't we use rescue_from but around callbacks?
There was a problem hiding this comment.
I'm definitely open to another approach here, @st0012! I went down the around route to allow channels/connections to set any additional context and/or capture their own exceptions, similar to the Sentry::Rack::CaptureExceptions and Sentry::Sidekiq::SentryContextMiddlware middlewares.
There was a problem hiding this comment.
I see, let's keep it this way then. and perhaps adding a test case for this scenario?
sentry-rails/lib/sentry/rails/action_cable/exception_reporter.rb
Outdated
Show resolved
Hide resolved
5777598 to
b572778
Compare
|
@st0012 I have no idea why I somehow ended up with so much duplication in this PR! I must have made a mistake when copy/pasting from the app of ours that is doing this. Anyways, I've just pushed up a new version that eliminates that. |
There are 5 types of hooks that Action Cable provides: * `Connection#connect` * `Connection#disconnect` * `Channel#subscribed` * `Channel#unsubscribed` * `Channel` actions Now, any exceptions raised within those hooks are captured by Sentry, and reported as `ActionCable/[...]` transactions. Additional context is included depending on the hook the exception was raised within. A note/quirk: the Rack env that's included in the scope is from the `Connection`, and therefore has a URL of the cable `mount_path` (usually `/cable`) as well as the headers from that initial connection request. Additionally, there is not currently a really clean way to hook in and set `user_context`. I don't know if that is a blocker for this integration, but wanted to make sure it was noted.
b572778 to
7b3abed
Compare
st0012
left a comment
There was a problem hiding this comment.
I've tested it locally and it looks great 👍
After adding more test cases this will be ready to merge 😄
(I may defer the merge a bit to ship other patch versions first, this will go out in 4.3.0)
| module Subscriptions | ||
| def self.included(base) | ||
| base.class_eval do | ||
| set_callback :subscribe, :around, ->(_, block) { sentry_capture(:subscribed, &block) }, prepend: true |
There was a problem hiding this comment.
I see, let's keep it this way then. and perhaps adding a test case for this scenario?
| end | ||
|
|
||
| describe ChatChannel do | ||
| include ActionCable::Channel::TestCase::Behavior |
There was a problem hiding this comment.
can we use channel spec here instead of mixing with minitest?
| it "captures errors during the subscribe" do | ||
| expect { subscribe room_id: 42 }.to raise_error('foo') | ||
|
|
||
| event = transport.events.last.to_json_compatible |
There was a problem hiding this comment.
can we also verify that the SDK only captures 1 event? like
expect(transport.events.count).to eq(1)| it "captures errors during the action" do | ||
| expect { perform :appear, foo: 'bar' }.to raise_error('foo') | ||
|
|
||
| event = transport.events.last.to_json_compatible |
There was a problem hiding this comment.
expect(transport.events.count).to eq(1)| end | ||
|
|
||
| class ChatChannel < ::ActionCable::Channel::Base | ||
| def subscribed |
There was a problem hiding this comment.
can we also test unsubscribe?
| Sentry.with_scope do |scope| | ||
| scope.set_rack_env(env) | ||
| scope.set_extras(action_cable: extra_context) if extra_context | ||
| scope.set_transaction_name("#{ACTION_CABLE_NAME}/#{transaction_name}") |
There was a problem hiding this comment.
since we already prefix it transaction_name with the channel's name, like AppearanceChannel#subscribed, I don't think the additional ActionCable namespace is necessary.
|
@agrobbin I'll release v4.3.0 next week. so maybe we can push this over the line by the end of this week? |
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
Hey @st0012, I think this got closed as stale but it was supposed to be merged, right? |
|
@louim I don't have a strong opinion on adding/rejecting this feature. on adding integration-specific features we usually make decisions based on community's reaction. and at the moment we haven't seen a strong demand on ActionCable integration. given that this PR wasn't complete (very close though), I didn't merge it nor have a plan to finish it. but if you or other users want to give the integration another try, I'll be happy to work with you 🙂 |
|
@st0012 @louim my sincere apologies on the slow-walking of this PR. I unfortunately have not had the time to come back to it and resolve the outstanding items. My work has drifted away from Action Cable over the last few months, making this less valuable for me, so it's gotten pushed down my priority list. I will try to get back to it in the next week or 2, but @louim if you want to take it the rest of the way, I'm totally on board with that! |
getsentry#1295 getsentry#1295 Co-authored-by: Stan Lo <stan001212@gmail.com>
getsentry#1295 Co-authored-by: Stan Lo <stan001212@gmail.com>
…tion of #1295) (#1638) * capture exceptions in Action Cable connections and channels There are 5 types of hooks that Action Cable provides: * `Connection#connect` * `Connection#disconnect` * `Channel#subscribed` * `Channel#unsubscribed` * `Channel` actions Now, any exceptions raised within those hooks are captured by Sentry, and reported as `ActionCable/[...]` transactions. Additional context is included depending on the hook the exception was raised within. A note/quirk: the Rack env that's included in the scope is from the `Connection`, and therefore has a URL of the cable `mount_path` (usually `/cable`) as well as the headers from that initial connection request. Additionally, there is not currently a really clean way to hook in and set `user_context`. I don't know if that is a blocker for this integration, but wanted to make sure it was noted. * Only extend ActionCable when it's defined Similar to the update for ActionCableExtensions in #1218 and #1494 * test(ActionCable): Assert only 1 event captured: #1295 #1295 * Remove namespace 'ActionCable/*' #1295 * Test ActionCable #unsubscribed #1295 * test(ActionCable): prefer rspec-rails over mini-test #1295 (comment) * test(ActionCable): cleanup duplicated set_callback * docs(CHANGELOG): update #1295 to #1638 Co-authored-by: Alex Robbin <agrobbin@gmail.com>
Hi again @st0012! Sorry it took me a couple weeks to get back to this. I've reissued this now that 4.2 has already been released.
I mentioned this in #470 (comment) (and #470 (comment)), and now that 4.x is reasonably stable, thought it worth getting this PR out there.
There are 5 types of hooks that Action Cable provides:
Connection#connectConnection#disconnectChannel#subscribedChannel#unsubscribedChannelactionsNow, any exceptions raised within those hooks are captured by Sentry, and reported as
ActionCable/[...]transactions. Additional context is included depending on the hook the exception was raised within.A note/quirk: the Rack env that's included in the scope is from the
Connection, and therefore has a URL of the cablemount_path(usually/cable) as well as the headers from that initial connection request.Additionally, there is not currently a really clean way to hook in and set
user_context. I don't know if that is a blocker for this integration, but wanted to make sure it was noted.Finally, this is limited to Rails 6+, as the hooks used (
action_cable_connectionandaction_cable_channel) weren't introduced until then (and neither were the test helpers).Closes #470.