Add instrumentation for Celery #780
Conversation
The executemany test fails in some conditions because the argument used is a sequence and not a sequence of sequences as it should be: https://dev.mysql.com/doc/connector-python/en/connector-python-api-mysqlcursor-executemany.html https://pymysql.readthedocs.io/en/latest/modules/cursors.html#pymysql.cursors.Cursor.executemany https://www.psycopg.org/docs/cursor.html#cursor.executemany
Port Celery instrumentation from DataDog donation: https://github.com/open-telemetry/opentelemetry-python-contrib/tree/5aee3ce32e418e1ce2ae46e303d1ca0630f3f836/reference/ddtrace/contrib/celery
Co-authored-by: Diego Hurtado <[email protected]>
…quezbernal/opentelemetry-python into mauricio/instrumentor-celery
ext/opentelemetry-ext-celery/src/opentelemetry/ext/celery/version.py
Outdated
Show resolved
Hide resolved
| # pylint: disable=unused-argument | ||
| def _instrument(self, **kwargs): |
There was a problem hiding this comment.
Using pylint disable in this place disables the check for all next lines I think, docs:
http://pylint.pycqa.org/en/latest/user_guide/message-control.html
If this is used only because of kwargs then wouldn't it be reasonable to add args and kwargs to pylintrc as ignored? It seems that it can be common case. WDYT?
# Argument names that match this expression will be ignored. Default to name
# with leading underscore.
ignored-argument-names=_.*|^ignored_|^unused_|^kwargs|^args|^mock_.+
| task_id = retrieve_task_id(kwargs) | ||
| if task is None or task_id is None: | ||
| logger.debug( | ||
| "Unable to extract the Task and the task_id. This version of Celery may not be supported." |
There was a problem hiding this comment.
Seeing this log message, as I user I would wonder where can I find information about supported Celery versions. Is it possible to add some link / information about places where can I look for those?
There was a problem hiding this comment.
Is it possible to be conclusive about the Celery versions that are not supported? Letting the user know that this version may not be supported is not that useful.
There was a problem hiding this comment.
This check is repeated several times, can it be placed in its own function?
There was a problem hiding this comment.
This check is repeated several times, can it be placed in its own function?
+1
There was a problem hiding this comment.
I've pulled these out as much as I could into utility functions, though the signatures for the signals we handle differ slightly.
|
|
||
| [options.extras_require] | ||
| test = | ||
| pytest |
There was a problem hiding this comment.
Should this have a version specified?
There was a problem hiding this comment.
We are using latest elsewhere in the code
ext/opentelemetry-ext-celery/src/opentelemetry/ext/celery/__init__.py
Outdated
Show resolved
Hide resolved
| task_id = retrieve_task_id(kwargs) | ||
| if task is None or task_id is None: | ||
| logger.debug( | ||
| "Unable to extract the Task and the task_id. This version of Celery may not be supported." |
There was a problem hiding this comment.
Is it possible to be conclusive about the Celery versions that are not supported? Letting the user know that this version may not be supported is not that useful.
| task_id = retrieve_task_id(kwargs) | ||
| if task is None or task_id is None: | ||
| logger.debug( | ||
| "Unable to extract the Task and the task_id. This version of Celery may not be supported." |
There was a problem hiding this comment.
This check is repeated several times, can it be placed in its own function?
ext/opentelemetry-ext-celery/src/opentelemetry/ext/celery/utils.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-celery/src/opentelemetry/ext/celery/utils.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-celery/src/opentelemetry/ext/celery/utils.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-celery/src/opentelemetry/ext/celery/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-celery/src/opentelemetry/ext/celery/__init__.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Diego Hurtado <[email protected]>
|
majorgreys
force-pushed
the
majorgreys/instrumentor-celery
branch
from
a6dc985 to
21cac5b
Compare
majorgreys
force-pushed
the
majorgreys/instrumentor-celery
branch
from
21cac5b to
3eae958
Compare
…pentelemetry-python into majorgreys/instrumentor-celery
|
@toumorokoshi can you confirm all your comments have been addressed and approve if they have. Would be good to get this one in |
Resolves #629 and supersedes #648.
This is largely a migration of the Datadog-donated integration of Celery though I have made an effort here to add tests for asynchronous tasks. In order to do so, I have used the pytest fixtures provided by Celery. At times I've had to add workarounds for known issues in celery.
As a follow-up on this PR, we should address the requests for better handling revoked and rejected tasks #648 (review) and canvases #648 (comment).