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

Background job custom error handlers #1212

Merged

Conversation

norbertnytko
Copy link
Contributor

@norbertnytko norbertnytko commented Oct 20, 2020

Goal
Allow passing custom error handler for background job gems. Useful to retry jobs for known transient errors without reporting errors to Datadog.

Example usage:

TRANSIENT_ERRORS = [SomeTransientError]
# This can be any callable object, provided `span` and `error` as arguments.
custom_error_handler = proc { |span, error| span.set_tag('error', true) unless TRANSIENT_ERRORS.include?(error.class) }

Datadog.configure do |c| 
  c.use :sidekiq, {
    error_handler: custom_error_handler
  }
end

@norbertnytko norbertnytko requested a review from a team October 20, 2020 08:04
@ericmustin
Copy link
Contributor

ericmustin commented Oct 20, 2020

@norbertnytko Thanks for the contribution. I've given this a quick pass, this looks like great work and very thorough. Would you mind describing your use case here a bit more? My understanding is, there are some background jobs that throw errors which you may not want marked as an error on the trace / trace related metrics / etc (span.status = 0 if <some_condition>? Or, you want to perform some arbitrary logic on them to determine if they're an error? Is this broadly correct? Are there other use cases here I am missing, if so would you mind explaining a bit so that we're aligned?

I think what you're proposing is reasonable, and recently we've seen a few use cases where users would like to pass in a custom error_handler for various integration, similar to what exists for some http clients currently. So I think this is worth having as a feature. If anything I think it may make sense to generalize this for all integrations, but I don't want to let perfect get in the way of good, so i'd be happy to see this accepted in it's current scope (i do still need to review more thoroughly) and set aside time to build on it.

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

  • One small nit is that we'll need to update the config instructions in GettingStarted.md to reflect the new options for each integration.

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

Nice work here. lgtm, approving. I'd like to have one of the other maintainers take a look here as well so going to wait to merge this in until next week when they return from PTO, if that's ok.

Some good next steps here for future work:

  • validation around the Proc that gets passed in.
    • we'd probably want to do this within the tracer.trace rescue block and log a debug or warn msg if the arg doesn't respond to call, but don't think this is blocking at the moment since we don't do vallidation around the tracer.trace on_error arg currently.
  • extending error_handler to other integrations beside just workers.

lib/ddtrace/contrib/sneakers/tracer.rb Show resolved Hide resolved
@marcotc marcotc added community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations labels Oct 27, 2020
Copy link
Member

@marcotc marcotc 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 again, @norbertnytko! Looks :shipit: to me.

@ericmustin ericmustin merged commit 944c0ba into DataDog:master Oct 28, 2020
@ericmustin ericmustin added this to the 0.43.0 milestone Oct 28, 2020
@norbertnytko norbertnytko deleted the norbertnytko/add-bg-workers-error-handler branch October 29, 2020 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants