Skip to content

Set user to the current scope via sidekiq middleware#1469

Merged
st0012 merged 1 commit intogetsentry:masterfrom
shouichi:set-user-via-middleware
Jun 25, 2021
Merged

Set user to the current scope via sidekiq middleware#1469
st0012 merged 1 commit intogetsentry:masterfrom
shouichi:set-user-via-middleware

Conversation

@shouichi
Copy link
Copy Markdown
Contributor

@shouichi shouichi commented Jun 11, 2021

Close #1468.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 11, 2021 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            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
shouichi commented Jun 11, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@shouichi shouichi left a comment

Choose a reason for hiding this comment

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

@st0012 Could you help to finish this PR?

@shouichi shouichi marked this pull request as draft June 11, 2021 03:30
st0012
st0012 reviewed Jun 11, 2021
View reviewed changes
@shouichi shouichi force-pushed the set-user-via-middleware branch from 56e42c9 to 22366eb Compare June 11, 2021 08:45
shouichi
shouichi commented Jun 11, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@shouichi shouichi left a comment

Choose a reason for hiding this comment

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

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

st0012
st0012 reviewed Jun 11, 2021
View reviewed changes
@st0012 st0012 added this to the 4.6.0 milestone Jun 11, 2021
@shouichi shouichi force-pushed the set-user-via-middleware branch from 22366eb to e12dcc6 Compare June 14, 2021 03:06
shouichi
shouichi commented Jun 14, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@shouichi shouichi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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.

@shouichi shouichi force-pushed the set-user-via-middleware branch from e12dcc6 to c903d43 Compare June 18, 2021 02:07
@shouichi shouichi marked this pull request as ready for review June 18, 2021 02:09
@shouichi shouichi requested a review from st0012 June 18, 2021 02:09
st0012
st0012 reviewed Jun 18, 2021
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)
Copy link
Copy Markdown
Contributor

@st0012 st0012 Jun 18, 2021

Choose a reason for hiding this comment

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

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 🙂

Copy link
Copy Markdown
Contributor Author

@shouichi shouichi Jun 22, 2021

Choose a reason for hiding this comment

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

Fixed!

Copy link
Copy Markdown
Contributor

@st0012 st0012 Jun 24, 2021

Choose a reason for hiding this comment

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

actually it still fails with Sidekiq 5

Copy link
Copy Markdown
Contributor Author

@shouichi shouichi Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ran tests with sidekiq 5 and confirmed it works.

@shouichi shouichi force-pushed the set-user-via-middleware branch from c903d43 to 70bf1b6 Compare June 22, 2021 05:40
@st0012 st0012 modified the milestone: 4.6.0 Jun 24, 2021
@shouichi shouichi force-pushed the set-user-via-middleware branch from 70bf1b6 to f674c58 Compare June 25, 2021 03:35
shouichi
shouichi commented Jun 25, 2021
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
Copy link
Copy Markdown
Contributor Author

@shouichi shouichi Jun 25, 2021

Choose a reason for hiding this comment

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

@st0012 this behavior is strange...

Copy link
Copy Markdown
Contributor

@st0012 st0012 Jun 25, 2021

Choose a reason for hiding this comment

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

I'll check this in a follow up PR.

st0012
st0012 approved these changes Jun 25, 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 contribution 👍
I'll add another PR to refactor the test cases and add changelog entry for this.

@st0012 st0012 merged commit 79f0453 into getsentry:master Jun 25, 2021
@shouichi shouichi deleted the set-user-via-middleware branch June 25, 2021 04:07
@shouichi
Copy link
Copy Markdown
Contributor Author

shouichi commented Jun 25, 2021

Thanks a lot for the help 👍

@st0012 st0012 mentioned this pull request Jun 25, 2021
Merged
st0012 added a commit that referenced this pull request Jun 25, 2021
@st0012
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

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

4.6.0

Development

Successfully merging this pull request may close these issues.

Automatically set user via sidekiq middleware

3 participants

@shouichi @codecov-commenter @st0012