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: record the original exception info when job can be retried #29

Merged
merged 1 commit into from
May 6, 2022

Conversation

yiichou
Copy link
Contributor

@yiichou yiichou commented May 4, 2022

If a failed job is able to be retried, Sidekiq will

  1. catch the original exception
  2. append the exception message to the job and push it into retry queue
  3. raise a new exception Sidekiq::JobRetry::Skip

when Sidekiq::LogstashJobLogger tries to parse the failed job by #log_job_exception,
the received exception is Sidekiq::JobRetry::Skip and
the recorded error_message, error and error_backtrace are wrong.

@yiichou
Copy link
Contributor Author

yiichou commented May 4, 2022

#28

@yiichou
Copy link
Contributor Author

yiichou commented May 4, 2022

One more question:

Sidekiq uses error_class to record the exception class name,

but sidekiq-logstash uses payload['error'] = exc.class

Should it be better to use error_class uniformly?

If a failed job is able to be retried, Sidekiq will

1. catch the original exception
2. append the exception message to the job and push it into retry queue
3. raise a new exception `Sidekiq::JobRetry::Skip`

when `Sidekiq::LogstashJobLogger` tries to parse the failed job by #log_job_exception,
the received exception is `Sidekiq::JobRetry::Skip` and
the recorded `error_message`, `error` and `error_backtrace` are wrong.
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Owner

@iMacTia iMacTia 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 for fixing this @yiichou.
This must have changed at some point because I also noticed this in a recent project I'm working on, but I didn't have time to update this gem yet, thank you so much 🙏 !

@iMacTia
Copy link
Owner

iMacTia commented May 6, 2022

@yiichou on your point above:

One more question:
Sidekiq uses error_class to record the exception class name,
but sidekiq-logstash uses payload['error'] = exc.class
Should it be better to use error_class uniformly?

Although I understand the will to follow the convention set by Sidekiq, I'm afraid this would be a braking change.
Better not to add it to this PR, we should take this offline into a separate issue and eventually consider it for a future v3.0 release of the gem

@iMacTia iMacTia merged commit f59fa08 into iMacTia:master May 6, 2022
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.

2 participants