Use ActiveSupport Lazy Load Hook to Apply ActiveJob Extension#1494
Merged
st0012 merged 1 commit intogetsentry:masterfrom Jul 8, 2021
Merged
Use ActiveSupport Lazy Load Hook to Apply ActiveJob Extension#1494st0012 merged 1 commit intogetsentry:masterfrom
st0012 merged 1 commit intogetsentry:masterfrom
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.
|
Contributor
|
@jdelStrother thanks for spotting the issue and opening this PR. I can confirm that it fixes the original issue by manually testing it 👍 |
Contributor
|
@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
Contributor
Author
|
Sure, I've pushed an update with a test. |
st0012
approved these changes
Jul 8, 2021
Contributor
st0012
left a comment
There was a problem hiding this comment.
@jdelStrother thanks for the fix 😄
Contributor
|
@jdelStrother btw I also updated the PR description and changelog entry to make the change more clear to users. |
rsanheim
added a commit
to simpledotorg/simple-server
that referenced
this pull request
Jul 29, 2021
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
rsanheim
added a commit
to simpledotorg/simple-server
that referenced
this pull request
Jul 30, 2021
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
choznerol
added a commit
to choznerol/sentry-ruby
that referenced
this pull request
Dec 9, 2021
Similar to the update for ActionCableExtensions in getsentry#1218 and getsentry#1494
st0012
pushed a commit
that referenced
this pull request
Dec 12, 2021
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since the 4.5.1 release, setting
Rails.application.config.active_job.foo = barin 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_appin specs, so it seems impossible to hook in early enough to test the failure case.cc @CroneKorkN, @st0012