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

[celery] add retry reason metadata to spans #630

Merged
merged 5 commits into from
Oct 10, 2018

Conversation

Kyle-Verhoog
Copy link
Member

Background

The method of retrying a task in celery raises retry exceptions. These appear to be getting caught and mishandled as errors in the tracer.

Overview

This PR aims to gracefully handle celery retry exceptions.

Current Status

Some investigation has been done and there is a signal API provided to trace retries. However I have been unable to replicate the retry exception being marked as an error in the tracer.

@palazzem
Copy link

palazzem commented Oct 4, 2018

Related to #603

eq_(span.get_tag('celery.action'), 'run')
eq_(span.get_tag('celery.state'), 'RETRY')

# TODO: these should be failing
Copy link
Member

Choose a reason for hiding this comment

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

The reason these aren't failing is because you are explicitly raising a Retry exception.

This is something that Celery will catch and handle on it's own.

The other case we need to be able to support is when you set a retry policy for the task and another type of exception is raised.

@self.app.task(autotry_for(Exception, ))
def fn_exception():
    raise Exception('oh no, what happened?!')

@brettlangdon
Copy link
Member

brettlangdon commented Oct 8, 2018

I think the cases we need to handle are:

# Success \0/
@app.task
def fn_success():
    pass


# This should be reported as an error
@app.task
def fn_exception():
    raise Exception()


# This should be reported as success (the desired action of retrying occurred)
# We should add metadata to the span to show that it will be retried
# We need to make sure we handle when the max number of
#   retries have occurred (this should then be an error)
@app.task
def fn_retry():
    raise Retry()


# Same as above
@app.task(bind=True)
def fn_retry(self):
    raise self.retry()


# This feels like it should fall under the same as the previous,
#   mark as success unless max retires have occurred
@app.task(autoretry_for=(Exception, ))
def fn_retry_exception():
    raise Exception()

@brettlangdon brettlangdon changed the base branch from master to 0.15-dev October 9, 2018 15:49
@brettlangdon brettlangdon changed the title [celery] Address celery retries treated as errors [celery] add retry reason metadata to spans Oct 9, 2018
@brettlangdon
Copy link
Member

@Kyle-Verhoog stealing this PR from you :)

Changed the base to 0.15-dev.

This PR now is focused on adding celery task retry reason as metadata to the span.


# Add retry reason metadata to span
# DEV: Use `str(reason)` instead of `reason.message` in case we get something that isn't an `Exception`
span.set_tag(c.TASK_RETRY_REASON_KEY, str(reason))
Copy link
Member

Choose a reason for hiding this comment

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

We don't want this added as an exception, retrying isn't necessarily an error.

We could potentially add other metadata here, like the type of exception that was raised, and traceback info, but reason alone should be good to get started?

@Kyle-Verhoog
Copy link
Member Author

I can't officially review the PR because I created it, but this looks good to me 👍

@labbati labbati self-requested a review October 10, 2018 14:35
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@brettlangdon brettlangdon merged commit 573c0aa into 0.15-dev Oct 10, 2018
@Kyle-Verhoog Kyle-Verhoog deleted the kyle-verhoog/celery-retry branch October 17, 2018 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants