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

Register SentryContextClientMiddleware on sidekiq workers #1774

Merged
merged 1 commit into from
Apr 9, 2022
Merged

Register SentryContextClientMiddleware on sidekiq workers #1774

merged 1 commit into from
Apr 9, 2022

Conversation

jeromepl
Copy link
Contributor

This configuration was missing in order to support the scenario where a sidekiq job itself pushes new jobs.

This meant that although the Sentry user was propagated from the client to the sidekiq server when running a job, it was not propagated again to any new jobs launched from this worker.

This is what is recommend as per sidekiq's documentation

@st0012 st0012 added this to In progress in 5.x via automation Apr 9, 2022
@st0012 st0012 added the bug fix label Apr 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2022

Codecov Report

Merging #1774 (38c55ab) into master (e99578b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1774      +/-   ##
==========================================
+ Coverage   98.38%   98.41%   +0.03%     
==========================================
  Files         144      145       +1     
  Lines        8462     8562     +100     
==========================================
+ Hits         8325     8426     +101     
+ Misses        137      136       -1     
Impacted Files Coverage Δ
sentry-sidekiq/lib/sentry-sidekiq.rb 76.00% <100.00%> (+2.08%) ⬆️
sentry-sidekiq/spec/sentry/sidekiq_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/breadcrumb.rb 96.29% <0.00%> (-3.71%) ⬇️
sentry-ruby/lib/sentry/event.rb 98.78% <0.00%> (-0.13%) ⬇️
sentry-ruby/lib/sentry/transport.rb 99.08% <0.00%> (-0.03%) ⬇️
sentry-ruby/lib/sentry/client.rb 100.00% <0.00%> (ø)
sentry-ruby/lib/sentry/transaction_event.rb 100.00% <0.00%> (ø)
sentry-ruby/spec/sentry/session_flusher_spec.rb 100.00% <0.00%> (ø)
sentry-ruby/spec/sentry/background_worker_spec.rb 100.00% <0.00%> (ø)
sentry-ruby/lib/sentry/transport/http_transport.rb 100.00% <0.00%> (ø)
... and 8 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 e99578b...38c55ab. Read the comment docs.

Copy link
Collaborator

@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.

Thank you!

@st0012 st0012 added this to the 5.3.0 milestone Apr 9, 2022
@st0012 st0012 merged commit 72396ce into getsentry:master Apr 9, 2022
5.x automation moved this from In progress to Done Apr 9, 2022
sl0thentr0py added a commit that referenced this pull request Sep 28, 2023
…s and retries [#2118](#2118)

#1774 added
`SentryContextClientMiddleware` to the server middleware chain too.

Sidekiq's scheduler pushes to the client on the server again for
schedules and retries which causes our trace propagation to be broken
for this case.
https://github.com/sidekiq/sidekiq/blob/aadc77a172f1490fb141c7936d2801ca3af925ef/lib/sidekiq/scheduled.rb#L39

Prioritize taking the trace propagation headers from the job whenever
they exist.
sl0thentr0py added a commit that referenced this pull request Sep 28, 2023
…s and retries [#2118](#2118) (#2118)

#1774 added
`SentryContextClientMiddleware` to the server middleware chain too.

Sidekiq's scheduler pushes to the client on the server again for
schedules and retries which causes our trace propagation to be broken
for this case.
https://github.com/sidekiq/sidekiq/blob/aadc77a172f1490fb141c7936d2801ca3af925ef/lib/sidekiq/scheduled.rb#L39

Prioritize taking the trace propagation headers from the job whenever
they exist.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5.x
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants