critical: 0.12.1 breaks synchronous celery calls (argument mismatch)

[eedwards-sk]

we just upgraded from 0.11.0 to 0.12.1 and we're now getting errors when calling a task without 'delay'

stack trace:

TypeError: process_subscription_order() takes at most 2 arguments (3 given)
  File "flask/app.py", line 1475, in full_dispatch_request
    rv = self.dispatch_request()
  File "flask/app.py", line 1461, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "flask_restful/__init__.py", line 477, in wrapper
    resp = resource(*args, **kwargs)
  File "flask/views.py", line 84, in view
    return self.dispatch_request(*args, **kwargs)
  File "flask_restful/__init__.py", line 587, in dispatch_request
    resp = meth(*args, **kwargs)
  File "core/authorization/decorators.py", line 135, in wrapper
    return method(*args, **kwargs)
  File "core/authorization/decorators.py", line 147, in decorated
    return func(*args, **kwargs)
  File "core/resources/foh.py", line 217, in put
    process_subscription_order(order.id, 'task')
  File "celery/local.py", line 188, in __call__
    return self._get_current_object()(*a, **kw)
  File "celery/app/task.py", line 420, in __call__
    return self.run(*args, **kwargs)
  File "ddtrace/contrib/celery/util.py", line 51, in wrapper
    return decorated(pin, wrapped, instance, args, kwargs)
  File "ddtrace/contrib/celery/task.py", line 107, in _task_run
    return func(*args, **kwargs)

example of task definition:

@celery.task(bind=False)
def process_subscription_order(order_id, actions=None):
    _process_order_synchronous(order_id, actions=actions, is_subscription_order=True)

calling it this way works:

process_subscription_order.delay(order_id)

but as per above, when we call it without the .delay(), it fails at the tracer level, and the task never executes:

process_subscription_order(order_id)

we're instrumenting by running the tracer:

ddtrace-run celery worker -A core.celery_worker:celery -Q default --concurrency=1

we upgraded to 0.12.1 so we could remove the need to manually configure the celery tracer in code

[palazzem]

Thanks @eedwards-sk for the follow-up! We're checking this issue (and others that have been reported) so that the Celery integration can work as expected. I'll reach you out once I have reproduced the problem.

[nickwilliams-eventbrite]

Having the same problem over here. Our celery tasks are failing for the same reason (with the same error) when called directly instead of delayed.

[nickwilliams-eventbrite]

Of note, if I back up to 0.12.0, the problem goes away, but there are a lot of candidate changes to Celery support in the span from 0.12.0 to 0.12.1. Unfortunately, backing up to 0.12.0 puts us up against the Python 3-incompatibility bug we reported a few weeks ago. 😞

[palazzem]

Thanks @nickwilliams-eventbrite for the feedback! we're giving very high priority to Celery integration.

[nickwilliams-eventbrite]

@palazzem, any update on this yet? You said you're giving very high priority to it, but it has been 30 days without a fix and we are stuck pinned on 0.12.0, which puts us at the mercy of another bug (Python 3 compatibility, which was fixed in 0.12.1) to make this bug go away.

[palazzem]

@nickwilliams-eventbrite sorry for this huge delay, but the priority is still high and we're working actively on it. Unfortunately, it's taking time because we need more testing for the new integration, especially because we don't want to ship something that may break again instrumentation. We evaluated a new way to instrument Celery because the current approach doesn't play well with old style tasks, in some cases even with decorators (this issue) and with the ddtrace-run script (see #493).

Because we did a release that solved some problems but raised new ones, we'd like to test it properly and see how we can keep the current compatibility matrix (Celery 3.1+) so the new release will be entirely backward compatible.

I will be sure to push the new integration so that you can test it next week. This will help us to confirm all these changes make sense.

Anyway, thanks for raising these concerns. This helps us a lot to improve our communication with developers and contributors. I'll be sure to publish status updates on a weekly / bi-weekly basis, so that it's clear what's going on our side.

[palazzem]

I've prepared the PR as discussed before. You can find the PR attached here: #530

It's still a WIP that is under test, but if you're interested to give it a try, your feedback would be great. It introduces some changes you can check in the PR description. I strongly advise to not use it on production until we have more positive feedbacks.

It doesn't address the issue with the ddtrace-run script that will be available in another PR, so a call to patch(celery=True) is required.

cc @nickwilliams-eventbrite @eedwards-sk @mgu

[mgu]

I will try as soon as I can

[palazzem]

Thank you very much! any feedback here or in the PR will be very helpful. If we have positive feedbacks, it will land in our 0.13.0 release.

[mgu]

I've tested the branch on #530 .
My feedbacks :

• Cannot run ddtrace-run celery worker attempts to execute celery with python #493 is still not fixed because https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/bootstrap/sitecustomize.py#L73 is still present
• synchronous celery calls will work for @eedwards-sk but he has to use .apply() instead of a direct call if he wants traces (as stated in your PR description, pre/post run signals are not fired with direct calls)
• it seems to work really well with a real app w/ celery 3.1 (not tested in production :D and no time to test it with celery 4.x) ❤️

[palazzem]

Thanks @mgu for the feedback! To answer your points:

• Cannot run ddtrace-run celery worker attempts to execute celery with python #493 should be fixed with [celery] Patch via post-import hooks #534 that removes the block you mentioned. This PR doesn't address the issue but you can test kyle-verhoog/celery-hook branch that includes also the Celery PR (saw you did already in the other thread. Here only for reference)
• correct, unfortunately signals are not handled for synchronous Celery calls unless you use apply(). This is a tradeoff we think to keep because Celery signals are way more safer than any kind of monkey-patch strategy (and well supported for future releases!)
• that's great! we'll made some changes based on some reviews and be ready to ship it