-
-
Notifications
You must be signed in to change notification settings - Fork 495
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
Use ActiveSupport Lazy Load Hook to Apply ActiveJob Extension #1494
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1494 +/- ##
==========================================
+ Coverage 98.21% 98.77% +0.56%
==========================================
Files 218 123 -95
Lines 10553 6715 -3838
==========================================
- Hits 10365 6633 -3732
+ Misses 188 82 -106 Continue to review full report at Codecov.
|
@jdelStrother thanks for spotting the issue and opening this PR. I can confirm that it fixes the original issue by manually testing it 👍 |
@jdelStrother I've added an isolated test for ActiveJob patching in #1497. can you modify it and make it fail without your fix? |
As soon as ActiveJob::Base is referenced, it applies Rails.application.config.active_job, and any further changes to that config are ignored. By loading ActiveJob::Base before eager_load, it prevents activejob being configured in, eg, config/initializers/new_framework_defaults.rb
Sure, I've pushed an update with a test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdelStrother thanks for the fix 😄
@jdelStrother btw I also updated the PR description and changelog entry to make the change more clear to users. |
The loading fix in getsentry/sentry-ruby#1494 could possibly help with the Sentry error we are seeing https://sentry.io/organizations/resolve-to-save-lives/issues/2527976309/?project=1217715
The loading fix in getsentry/sentry-ruby#1494 could possibly help with the Sentry error we are seeing https://sentry.io/organizations/resolve-to-save-lives/issues/2527976309/?project=1217715
Similar to the update for ActionCableExtensions in getsentry#1218 and getsentry#1494
…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>
Since the 4.5.1 release, setting
Rails.application.config.active_job.foo = bar
in an initializers file (eg config/initializers/foo.rb) would be ignored.As soon as ActiveJob::Base is referenced, it applies Rails.application.config.active_job, and any further changes to that config are ignored. By loading ActiveJob::Base before eager_load, it prevents ActiveJob being configured in, eg, config/initializers/new_framework_defaults.rb
How about using
ActiveSupport.on_load(:active_job)
instead?This tweaks the behaviour in #1464 which was introduced to fix #1249, would be good to confirm that the original issue poster isn't affected by this new fix.
I tried to add a test but found it kind of tricky. ActiveJob::Base is already loaded by the time we call
make_basic_app
in specs, so it seems impossible to hook in early enough to test the failure case.cc @CroneKorkN, @st0012