Skip to content

Extract wrapped Sidekiq job class names in both Sidekiq integrations#1238

Merged
st0012 merged 2 commits intogetsentry:masterfrom
agrobbin:sidekiq-transaction-name-wrapped-job-class
Jan 29, 2021
Merged

Extract wrapped Sidekiq job class names in both Sidekiq integrations#1238
st0012 merged 2 commits intogetsentry:masterfrom
agrobbin:sidekiq-transaction-name-wrapped-job-class

Conversation

@agrobbin
Copy link
Copy Markdown
Contributor

@agrobbin agrobbin commented Jan 26, 2021

The Active Job wrapped job class name logic was only happening in the lower-level ErrorHandler, and was actually being ignored since transaction_name was already set via SentryContextMiddleware.

Now, the logic to properly extract the transaction_name has been refactored into 1 Util class that is used by SentryContextMiddleware and ErrorHandler.

Separately, this removes the now-unused CleanupMiddleware, after its replacement in #1177.

Fixes #1237.

@agrobbin
remove now-unused Sidekiq CleanupMiddleware
eb1388a 
This was replaced in getsentry#1177, but the middleware wasn't actually removed.
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 26, 2021 •
edited
Loading

Codecov Report

Merging #1238 (c74d656) into master (ff865eb) will decrease coverage by 26.25%.
The diff coverage is 94.11%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1238       +/-   ##
===========================================
- Coverage   97.92%   71.66%   -26.26%     
===========================================
  Files         193       44      -149     
  Lines        8280     1546     -6734     
===========================================
- Hits         8108     1108     -7000     
- Misses        172      438      +266
Impacted Files Coverage Δ
sentry-sidekiq/lib/sentry/sidekiq/util.rb 91.66% <91.66%> (ø)
sentry-sidekiq/lib/sentry/sidekiq/error_handler.rb 100.00% <100.00%> (+5.00%) ⬆️
...iq/lib/sentry/sidekiq/sentry_context_middleware.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transaction.rb 23.52% <0.00%> (-76.48%) ⬇️
sentry-ruby/lib/sentry/transport/http_transport.rb 31.42% <0.00%> (-68.58%) ⬇️
sentry-ruby/lib/sentry/rack/capture_exceptions.rb 26.31% <0.00%> (-68.43%) ⬇️
sentry-ruby/lib/sentry/interfaces/request.rb 28.57% <0.00%> (-66.17%) ⬇️
sentry-ruby/lib/sentry/span.rb 35.48% <0.00%> (-64.52%) ⬇️
sentry-ruby/lib/sentry/utils/real_ip.rb 43.33% <0.00%> (-56.67%) ⬇️
sentry-ruby/lib/sentry/transport.rb 43.75% <0.00%> (-54.17%) ⬇️
... and 172 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 ff865eb...c74d656. Read the comment docs.

@agrobbin agrobbin force-pushed the sidekiq-transaction-name-wrapped-job-class branch from c74d656 to 841e6ab Compare January 26, 2021 19:24
@agrobbin
Copy link
Copy Markdown
Contributor Author

agrobbin commented Jan 26, 2021

I'm not sure why project-level code coverage dropped so significantly, @st0012 let me know if I have something weird going on here that is causing that!

@st0012 st0012 added this to the sentry-sidekiq-4.1.3 milestone Jan 27, 2021
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Jan 27, 2021

@agrobbin thanks for the PR 👍 it looks good but I'll do some testing to see why I was not able to spot the issue before merging this, which will probably be this Friday 🙂

st0012
st0012 reviewed Jan 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for reporting the issue & the PR 😄

I can confirm this fixes the issue
截圖 2021-01-29 下午6 25 05

just left a few questions + small suggestions. I think we can merge it today 👍

@agrobbin agrobbin force-pushed the sidekiq-transaction-name-wrapped-job-class branch from 841e6ab to 6cd7bec Compare January 29, 2021 13:23
@agrobbin
extract wrapped Sidekiq job class names in both Sidekiq integrations
e372134 
The Active Job `wrapped` job class name logic was only happening in the lower-level `ErrorHandler`, and was actually being ignored since `transaction_name` was already set via `SentryContextMiddleware`.

Now, the logic to properly extract the `transaction_name` has been refactored into `ContextFilter`.
@agrobbin agrobbin force-pushed the sidekiq-transaction-name-wrapped-job-class branch from 6cd7bec to e372134 Compare January 29, 2021 13:27
@agrobbin
Copy link
Copy Markdown
Contributor Author

agrobbin commented Jan 29, 2021

@st0012 just pushed an update here, let me know if there's anything else I can do!

st0012
st0012 approved these changes Jan 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the great work 👍

@st0012 st0012 merged commit e9a2f62 into getsentry:master Jan 29, 2021
@agrobbin
Copy link
Copy Markdown
Contributor Author

agrobbin commented Jan 29, 2021

Thanks, @st0012! Looking forward to getting this released and into production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@st0012 st0012 st0012 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

sentry-sidekiq

Projects

None yet

Milestone

sentry-sidekiq-4.1.4

Development

Successfully merging this pull request may close these issues.

Active Job + Sidekiq transaction name has reverted back to JobWrapper

4 participants

@agrobbin @codecov-io @st0012 @sl0thentr0py