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

Allow capture of error class when job fails #33

Closed

Conversation

niborg
Copy link
Contributor

@niborg niborg commented Feb 8, 2024

This gem captures failed jobs with the jobs_failed_total metric, but does not provide a way to add a label for the error class, which can be useful. This commit allows developers to optionally include a label with the error class on the jobs_failed_total metric.

This gem captures failed jobs with the jobs_failed_total metric, but does not provide a way to add a label for the error class, which can
be useful. This commit allows developers to optionally include a
label with the error class on the jobs_failed_total metric.
@niborg
Copy link
Contributor Author

niborg commented Feb 8, 2024

Woops meant to make this on my fork!

@niborg niborg closed this Feb 8, 2024
@Envek
Copy link
Member

Envek commented Feb 9, 2024

But why not here? 😄

@niborg
Copy link
Contributor Author

niborg commented Feb 9, 2024

Soon!

Copy link
Member

@Envek Envek left a comment

Choose a reason for hiding this comment

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

I believe implementation could be simpler and less flexible.

Also you need to specify new tag key at metric registration time.

Comment on lines +26 to +33
if (config_value = Yabeda::Sidekiq.config.label_for_error_class_on_sidekiq_jobs_failed)
label = if config_value.is_a?(Symbol) || config_value.is_a?(String)
config_value
else
DEFAULT_JOB_FAILED_ERROR_LABEL_KEY
end
sidekiq_jobs_failed_labels[label] = e.class.name
end
Copy link
Member

Choose a reason for hiding this comment

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

Why is that flexibility needed? Wouldn't just error tag be enough?

Suggested change
if (config_value = Yabeda::Sidekiq.config.label_for_error_class_on_sidekiq_jobs_failed)
label = if config_value.is_a?(Symbol) || config_value.is_a?(String)
config_value
else
DEFAULT_JOB_FAILED_ERROR_LABEL_KEY
end
sidekiq_jobs_failed_labels[label] = e.class.name
end
if Yabeda::Sidekiq.config.label_for_error_class_on_sidekiq_jobs_failed
sidekiq_jobs_failed_labels[:error] = e.class.name
end

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.

None yet

2 participants