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.

Sign up for GitHub

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

Jump to bottom

Set user to the current scope via sidekiq middleware #1469

Merged

st0012 merged 1 commit into getsentry:master from shouichi:set-user-via-middleware

Jun 25, 2021

Contributor

shouichi commented Jun 11, 2021

Close #1468.

codecov-commenter commented Jun 11, 2021 •

edited

Codecov Report

Merging #1469 (56e42c9) into master (f992b27) will increase coverage by 0.49%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##           master    #1469      +/-   ##
==========================================
+ Coverage   98.24%   98.74%   +0.49%     
==========================================
  Files         214      120      -94     
  Lines       10156     6350    -3806     
==========================================
- Hits         9978     6270    -3708     
+ Misses        178       80      -98

Impacted Files	Coverage Δ
sentry-sidekiq/lib/sentry-sidekiq.rb	`72.72% <50.00%> (-27.28%)`	⬇️
...iq/lib/sentry/sidekiq/sentry_context_middleware.rb	`100.00% <100.00%> (ø)`
...c/sentry/sidekiq/sentry_context_middleware_spec.rb	`100.00% <100.00%> (ø)`
sentry-sidekiq/spec/sentry/sidekiq_spec.rb	`100.00% <100.00%> (ø)`
sentry-rails/app/jobs/sentry/send_event_job.rb	`66.66% <0.00%> (-11.12%)`	⬇️
...-sidekiq/spec/sentry/sidekiq/error_handler_spec.rb	`100.00% <0.00%> (ø)`
sentry-raven/spec/raven/processors/cookies_spec.rb
sentry-raven/spec/raven/transports/http_spec.rb
sentry-raven/lib/raven/configuration.rb
... and 93 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 f992b27...56e42c9. Read the comment docs.

shouichi commented

View reviewed changes

Contributor Author

shouichi left a comment

@st0012 Could you help to finish this PR?

sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb Show resolved Hide resolved

sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb Outdated Show resolved Hide resolved

shouichi marked this pull request as draft

June 11, 2021 03:30

st0012 reviewed

View reviewed changes

sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb Outdated Show resolved Hide resolved

sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb Show resolved Hide resolved

sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb Outdated Show resolved Hide resolved

shouichi force-pushed the set-user-via-middleware branch from 56e42c9 to 22366eb Compare

June 11, 2021 08:45

shouichi commented

View reviewed changes

Contributor Author

shouichi left a comment

@st0012 Hi, I rewrote the test in an E2E way. Could you take a look?

sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb Outdated Show resolved Hide resolved

sentry-sidekiq/spec/spec_helper.rb Outdated Show resolved Hide resolved

st0012 reviewed

View reviewed changes

sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb Outdated Show resolved Hide resolved

sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb Outdated Show resolved Hide resolved

sentry-sidekiq/lib/sentry/sidekiq/sentry_context_middleware.rb Outdated Show resolved Hide resolved

sentry-sidekiq/spec/spec_helper.rb Outdated Show resolved Hide resolved

st0012 added this to the 4.6.0 milestone

st0012 added enhancement sentry-sidekiq labels

st0012 added this to In progress in 4.x via automation

shouichi force-pushed the set-user-via-middleware branch from 22366eb to e12dcc6 Compare

June 14, 2021 03:06

shouichi commented

View reviewed changes

Contributor Author

shouichi left a comment •

edited

I believe it's almost done but there's one outstanding issue. The notable changes made from the last review are

Use traces_sample_rate = 1.0 "hack".
Use HappyWroker/SadWoker from spec helper.

sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb Show resolved Hide resolved

shouichi force-pushed the set-user-via-middleware branch from e12dcc6 to c903d43 Compare

June 18, 2021 02:07

shouichi marked this pull request as ready for review

June 18, 2021 02:09

shouichi requested a review from st0012

June 18, 2021 02:09

st0012 reviewed

View reviewed changes

sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb Outdated

+                  queue = random_empty_queue
+                  options = { fetch: Sidekiq::BasicFetch.new(queues: [queue.name]) }
+                  processor = Sidekiq::Processor.new(nil, options)

Collaborator

st0012 Jun 18, 2021

I think the way you initialize sidekiq processor is not compatible with sidekiq v5.2 (which runs in Ruby 2.4 test suite).

instead of initializing the processor directly, can we initialize a manager and get the processor from it? like

sentry-ruby/sentry-sidekiq/spec/sentry/sidekiq_spec.rb

Lines 15 to 19 in 6d2fc60

    
           let(:processor) do 
        
             opts = { :queues => ['default'] } 
        
             manager = Sidekiq::Manager.new(opts) 
        
             manager.workers.first 
        
           end

I will merge this PR once the test is fixed 🙂

Contributor Author

shouichi Jun 22, 2021

Fixed!

Collaborator

st0012 Jun 24, 2021

actually it still fails with Sidekiq 5

Contributor Author

shouichi Jun 25, 2021 •

edited

Ran tests with sidekiq 5 and confirmed it works.

shouichi force-pushed the set-user-via-middleware branch from c903d43 to 70bf1b6 Compare

June 22, 2021 05:40

st0012 modified the milestone: 4.6.0


          Set user to the current scope via sidekiq middleware

f674c58

shouichi force-pushed the set-user-via-middleware branch from 70bf1b6 to f674c58 Compare

June 25, 2021 03:35

shouichi commented

View reviewed changes

sentry-sidekiq/spec/sentry/sidekiq/sentry_context_middleware_spec.rb


		client.push('queue' => queue.name, 'class' => SadWorker, 'args' => [])

		# XXX: In ruby 2.4, two events are pushed. In other versions, only one

Contributor Author

shouichi Jun 25, 2021

@st0012 this behavior is strange...

Collaborator

st0012 Jun 25, 2021

I'll check this in a follow up PR.

st0012 approved these changes

View reviewed changes

Collaborator

st0012 left a comment

thanks for the contribution 👍
I'll add another PR to refactor the test cases and add changelog entry for this.

4.x automation moved this from In progress to Reviewer approved

st0012 merged commit 79f0453 into getsentry:master

4.x automation moved this from Reviewer approved to Done

shouichi deleted the set-user-via-middleware branch

June 25, 2021 04:07

Contributor Author

shouichi commented Jun 25, 2021

Thanks a lot for the help 👍

st0012 mentioned this pull request

Follow up #1469 #1483

Merged

st0012 added a commit that referenced this pull request


          Follow up #1469 (#1483)

d82b463

* Refactor tests

* Update changelog

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