Skip to content

Only extend ActiveJob when it's defined#1218

Merged
st0012 merged 1 commit intomasterfrom
fix-#1210
Jan 20, 2021
Merged

Only extend ActiveJob when it's defined#1218
st0012 merged 1 commit intomasterfrom
fix-#1210

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@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
Copy Markdown

codecov-io commented Jan 20, 2021

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.79%. Comparing base (38b3891) to head (3ae1365).
⚠️ Report is 1112 commits behind head on master.

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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
Copy Markdown

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor Author

st0012 commented Jan 21, 2021

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

@croaky
Copy link
Copy Markdown

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