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

Only extend ActiveJob when it's defined #1218

Merged
merged 1 commit into from
Jan 20, 2021
Merged

Only extend ActiveJob when it's defined #1218

merged 1 commit into from
Jan 20, 2021

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Jan 20, 2021

It's somewhat common for Rails users to exclude ActiveJob by manually
requiring every other components when they don't need it. So we should
detect if the ActiveJob constant is defined before extending it.

Fixes #1210

@st0012 st0012 added this to the 4.1.5 milestone Jan 20, 2021
@st0012 st0012 self-assigned this Jan 20, 2021
@codecov-io
Copy link

codecov-io commented Jan 20, 2021

Codecov Report

Merging #1218 (3ae1365) into master (38b3891) will increase coverage by 0.86%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
+ Coverage   97.92%   98.79%   +0.86%     
==========================================
  Files         193       28     -165     
  Lines        8249      746    -7503     
==========================================
- Hits         8078      737    -7341     
+ Misses        171        9     -162     
Impacted Files Coverage Δ
sentry-rails/spec/support/test_rails_app/app.rb 100.00% <ø> (ø)
sentry-rails/lib/sentry/rails/railtie.rb 100.00% <100.00%> (ø)
sentry-rails/spec/sentry/rails/activejob_spec.rb 98.00% <100.00%> (+0.04%) ⬆️
sentry-rails/lib/sentry/rails/active_job.rb 96.66% <0.00%> (-3.34%) ⬇️
sentry-raven/lib/raven/configuration.rb
sentry-raven/lib/raven/context.rb
...raven/spec/raven/breadcrumbs/sentry_logger_spec.rb
sentry-raven/lib/raven/processor/http_headers.rb
sentry-raven/spec/raven/integrations/rails_spec.rb
...c/raven/integrations/sidekiq/error_handler_spec.rb
... and 159 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38b3891...3ae1365. Read the comment docs.

It's somewhat common for Rails users to exclude ActiveJob by manually
requiring every other components when they don't need it. So we should
detect if the ActiveJob constant is defined before extending it.
@st0012 st0012 merged commit 25ab172 into master Jan 20, 2021
@st0012 st0012 deleted the fix-#1210 branch January 20, 2021 06:25
@croaky
Copy link

croaky commented Jan 21, 2021

Thanks for this! I bumped into it while migrating from sentry-raven to sentry-ruby today.

Do you think you could release a patch version of this to rubygems.org?

This is working perfectly locally:

gem "sentry-rails", github: "getsentry/sentry-ruby", glob: "sentry-rails/*.gemspec"
gem "sentry-ruby", github: "getsentry/sentry-ruby", glob: "sentry-ruby/*.gemspec"

But the Sentry transactions aren't being reported from Heroku. Wondering if it's Git-related due to some logs but, still investigating:

fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)

@st0012
Copy link
Collaborator Author

st0012 commented Jan 21, 2021

I'll release sentry-rails 4.1.5 this week. but I think this should work

gem "sentry-rails", github: "getsentry/sentry-ruby"
gem "sentry-ruby", github: "getsentry/sentry-ruby"

@st0012
Copy link
Collaborator Author

st0012 commented Jan 21, 2021

@croaky well it's been released in version 4.1.5 now 🙂

@croaky
Copy link

croaky commented Jan 21, 2021

Thanks @st0012!

choznerol added a commit to choznerol/sentry-ruby that referenced this pull request Dec 9, 2021
similar to the way we treat ActiveJobExtensions in getsentry#1218
choznerol added a commit to choznerol/sentry-ruby that referenced this pull request Dec 9, 2021
similar to the way we treat ActiveJobExtensions in getsentry#1218
choznerol pushed a commit to choznerol/sentry-ruby that referenced this pull request Dec 9, 2021
getsentry@3ae1365

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.
choznerol added a commit to choznerol/sentry-ruby that referenced this pull request Dec 9, 2021
Similar to the update for ActiveJobExtensions in getsentry#1218
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uninitialized constant ActiveJob
3 participants