-
Notifications
You must be signed in to change notification settings - Fork 609
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
Add instrumentation for Celery #648
Add instrumentation for Celery #648
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, some comments and documentation seem to be out of this project context.
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
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
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-docker-tests/tests/pymysql/test_pymysql_functional.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
…quezbernal/opentelemetry-python into mauricio/instrumentor-celery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Hi Celery core contributor here. |
I tried to add you as a reviewer but your username does not show up in the drop-down list. I have assigned this to you instead 🤷♂️. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments, but looks good!
I may try to address the comments as I know Mauricio has been moved to other priorities.
=src | ||
packages=find_namespace: | ||
install_requires = | ||
opentelemetry-api == 0.7.dev0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will probably have to be bumped before we merge.
|
||
[options.extras_require] | ||
test = | ||
opentelemetry-test == 0.7.dev0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this as well.
return | ||
|
||
ex = kwargs.get("einfo") | ||
if ex is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we set the span status to unknown, even if the exception is None? this function is only called on task failure.
context = kwargs.get("request") | ||
if task is None or context is None: | ||
logger.debug( | ||
"Unable to extract the Task or the Context. This version of Celery may not be supported." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be good to call out what is actually being looked for (the "sender" and "request" keys). Might help someone debug.
|
||
attribute_name = "celery.{}".format(key) | ||
|
||
if key == "id": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this block of code feels like it could be simplified (e.g. using a map for known constants, or a lookup).
packages=find_namespace: | ||
install_requires = | ||
opentelemetry-api == 0.7.dev0 | ||
celery ~= 4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we hard pin the celery version range like this? granted it's good to know what version is supported, but I have a feeling newer versions could be supported without a lot of issue, and this may block people from upgrading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Celery 5.x and above won't support this instrumentation extension.
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
__version__ = "0.7.dev0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be 0.8.dev0
@thedrow would you mind giving this a review? We're making a push to merge in older PRs, and we had users who are blocked on celery instrumentation in opentelemetry. We'll probably merge in a couple days unless there's issues on your side. And we can always address improvements in a followup PR. |
return | ||
|
||
# TODO: When the span could be SERVER? | ||
span = self._tracer.start_span(task.name, kind=trace.SpanKind.CONSUMER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what happens if the task is run eagerly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when tasks are revoked or rejected?
How do we trace canvases?
@thedrow Thanks for the input! Adding a traced function for the I'm less sure about the addition of canvas tracing as I have no experience with them. Is the expected behavior that tasks that are in a workflow should show up as part of the same trace? |
Isn't it possible to have a span for the canvas itself and then for each task? |
@thedrow I have opened a new PR to carry on this work in #780. I would appreciate your feedback on it, in particular the testing, which I changed to use the pytest fixtures from Celery. This allowed me to test async execution. I decided to patch the I've also identified the requests you made as follow-up features. I have code for adding revoked and rejected handlers but I wasn't able to get them working. I opened celery/celery#6158 for the issue I found with testing the |
License :: OSI Approved :: Apache Software License | ||
Programming Language :: Python | ||
Programming Language :: Python :: 3 | ||
Programming Language :: Python :: 3.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.4 can be dropped
Closing in favor of #780 |
* feat: stackdriver trace exporter * Update packages/opentelemetry-exporter-stackdriver-trace/README.md Co-Authored-By: Mayur Kale <mayurkale@google.com> * chore: undo changes to basic tracer example * chore: remove dependent type from sdt export * chore: make properties readonly * chore: remove throw * chore: document unused types * chore: remove unused type * fix: lint * feat: infer google application credentials * chore: move stackdriver trace to its own example * chore: missing service name is actually fine * chore: add link to stackdriver trace example * test: add tests for transformer * test: add tests for stackdriver trace export * test: speed up tests * chore: update agent label version * chore: update test * fix: lint * fix: lint * chore: update example * chore: remove service name * chore: fix tests * chore: remove serviceName from readme * chore: fix lint * chore: review comments * chore: review comments * chore: add screenshot of stackdriver trace Co-authored-by: Mayur Kale <mayurkale@google.com>
Fixes #629
Note: Currently this integration doesn't satisfy all the requirements in the semantic conventions for a messaging system: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/messaging.md. In df5bf48 I updated it to include some of the attributes, but I haven't been able to gather the other ones. I'd propose to review this PR as it is and improve the semantic convention compliance in a follow up PR.