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

Fix Sidekiq metadata: Ensures metadata is attached to notifications #495

Merged
merged 9 commits into from
Nov 22, 2018

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Nov 13, 2018

Goal

Fix the issue where the Sidekiq integration is clearing request data before it can be attached to a notify call.

Copy link
Contributor

@tobyhs tobyhs left a comment

Choose a reason for hiding this comment

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

lib changes look fine, but would it make sense to have tests in spec/integrations/sidekiq_spec.rb that checks whether Sidekiq request data is cleared or not depending on various conditions after calling Bugsnag::Sidekiq#call?

tobyhs
tobyhs previously approved these changes Nov 14, 2018
Copy link
Contributor

@tobyhs tobyhs left a comment

Choose a reason for hiding this comment

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

possible suggestion, otherwise lgtm

spec/integrations/sidekiq_spec.rb Outdated Show resolved Hide resolved
features/sidekiq.feature Outdated Show resolved Hide resolved
features/fixtures/sidekiq/Dockerfile Outdated Show resolved Hide resolved
lib/bugsnag/integrations/sidekiq.rb Outdated Show resolved Hide resolved
features/fixtures/sidekiq/Dockerfile Outdated Show resolved Hide resolved
features/fixtures/sidekiq/app/Gemfile Show resolved Hide resolved
@Cawllec
Copy link
Contributor Author

Cawllec commented Nov 20, 2018

Just need the codeclimate allowable complexity to be updated here, I don't have access to that functionality.

@snmaynard
Copy link
Contributor

@kattrali can you make the change?

@Cawllec Cawllec merged commit 81851b5 into master Nov 22, 2018
@Cawllec Cawllec deleted the cawllec/fix-sidekiq-metadata branch November 22, 2018 08:31
@Cawllec Cawllec mentioned this pull request Nov 30, 2018
@javierjulio
Copy link
Contributor

I see this includes a metadata tab for errors that occur within Sidekiq, but only includes msg and queue. Is there a way for Bugsnag to report the worker args when any error occurs within Sidekiq?

@Cawllec
Copy link
Contributor Author

Cawllec commented Dec 13, 2018

Hey @javierjulio, the arguments for the job the worker was running with should be present in the msg => args section of the sidekiq metadata.
If that isn't the case, or you aren't seeing that reported, please let us know.

@javierjulio
Copy link
Contributor

@Cawllec no everything is good. 👏🏻 Thanks so much for fixing this! ❤️ I confirmed the fix in development by replicating various errors from production and am now seeing the Sidekiq data in all those cases where we weren't before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants