Add instrumentation for Celery by mauriciovasquezbernal · Pull Request #648 · open-telemetry/opentelemetry-python

mauriciovasquezbernal · 2020-05-04T21:52:56Z

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.

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

ocelotl

Looks mostly good, some comments and documentation seem to be out of this project context.

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

…quezbernal/opentelemetry-python into mauricio/instrumentor-celery

ocelotl

👍

thedrow · 2020-05-14T10:18:47Z

Hi Celery core contributor here.
Can you please request me to review this so I won't forget?

ocelotl · 2020-05-14T14:44:12Z

Hi Celery core contributor here.
Can you please request me to review this so I won't forget?

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 🤷‍♂️.

toumorokoshi

A few minor comments, but looks good!

I may try to address the comments as I know Mauricio has been moved to other priorities.

toumorokoshi · 2020-05-28T03:31:47Z

+    =src
+packages=find_namespace:
+install_requires =
+    opentelemetry-api == 0.7.dev0


this will probably have to be bumped before we merge.

toumorokoshi · 2020-05-28T03:31:51Z

+
+[options.extras_require]
+test =
+    opentelemetry-test == 0.7.dev0


this as well.

toumorokoshi · 2020-05-28T03:36:11Z

+            return
+
+        ex = kwargs.get("einfo")
+        if ex is None:


shouldn't we set the span status to unknown, even if the exception is None? this function is only called on task failure.

toumorokoshi · 2020-05-28T03:37:02Z

+        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."


might be good to call out what is actually being looked for (the "sender" and "request" keys). Might help someone debug.

toumorokoshi · 2020-05-28T03:38:15Z

+
+        attribute_name = "celery.{}".format(key)
+
+        if key == "id":


this block of code feels like it could be simplified (e.g. using a map for known constants, or a lookup).

toumorokoshi · 2020-05-28T03:41:38Z

+packages=find_namespace:
+install_requires =
+    opentelemetry-api == 0.7.dev0
+    celery ~= 4.0


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.

Celery 5.x and above won't support this instrumentation extension.

toumorokoshi · 2020-05-28T03:42:01Z

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+__version__ = "0.7.dev0"


should be 0.8.dev0

toumorokoshi · 2020-05-29T05:40:03Z

@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.

thedrow · 2020-05-29T06:42:44Z

+            return
+
+        # TODO: When the span could be SERVER?
+        span = self._tracer.start_span(task.name, kind=trace.SpanKind.CONSUMER)


Also what happens if the task is run eagerly?

thedrow

What happens when tasks are revoked or rejected?
How do we trace canvases?

majorgreys · 2020-06-02T13:01:04Z

@thedrow Thanks for the input!

Adding a traced function for the task_revoked and task_rejected should be straightforward and so makes sense to add to this PR if we can.

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?

thedrow · 2020-06-02T13:29:56Z

Isn't it possible to have a span for the canvas itself and then for each task?
If so, it makes sense in my opinion.

majorgreys · 2020-06-05T18:15:03Z

@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 celery_app fixture though to have it call celery_worker after the task decorator had been applied to functions local to each test.

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 task_revoked signal. Am I doing something wrong or perhaps there's a workaround I should know about?

auvipy · 2020-06-06T01:26:04Z

+    License :: OSI Approved :: Apache Software License
+    Programming Language :: Python
+    Programming Language :: Python :: 3
+    Programming Language :: Python :: 3.4


3.4 can be dropped

toumorokoshi · 2020-06-13T04:14:55Z

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>

mauriciovasquezbernal added ext instrumentation Related to the instrumentation of third party libraries or frameworks labels May 4, 2020

mauriciovasquezbernal requested a review from a team May 4, 2020 21:52

mauriciovasquezbernal added 4 commits May 4, 2020 17:22

Add Celery Instrumentation

7094ec4

Port Celery instrumentation from DataDog donation: https://github.com/open-telemetry/opentelemetry-python-contrib/tree/5aee3ce32e418e1ce2ae46e303d1ca0630f3f836/reference/ddtrace/contrib/celery

add missing instrument in docstring

8b0f2e5

Update with some semantic conventions

df5bf48

add test for instrumentation info

5ae5626

ocelotl suggested changes May 6, 2020

View reviewed changes

mauriciovasquezbernal and others added 11 commits May 6, 2020 13:55

don't import whole celery

001527e

make variables name consistent

eb05bae

handle feedback

0b590c6

remove unuseful comment

d8aead4

use unittest.fail

c2b7561

Merge branch 'master' into mauricio/instrumentor-celery

b0e0720

use instrumentation instead of integration

a571304

Apply suggestions from code review

3d94aed

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

Merge branch 'mauricio/instrumentor-celery' of github.com:mauriciovas…

606595e

…quezbernal/opentelemetry-python into mauricio/instrumentor-celery

Merge branch 'master' into mauricio/instrumentor-celery

db71469

fix lint

ba6db4e

ocelotl approved these changes May 7, 2020

View reviewed changes

codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label May 8, 2020

ocelotl assigned thedrow and ocelotl and unassigned ocelotl May 14, 2020

ocelotl self-requested a review May 14, 2020 14:44

ocelotl approved these changes May 27, 2020

View reviewed changes

toumorokoshi suggested changes May 28, 2020

View reviewed changes

thedrow reviewed May 29, 2020

View reviewed changes

thedrow suggested changes May 29, 2020

View reviewed changes

majorgreys added a commit to DataDog/opentelemetry-python that referenced this pull request Jun 4, 2020

address feedback from PR open-telemetry#648 and fix async tests

9dff127

majorgreys mentioned this pull request Jun 4, 2020

Add instrumentation for Celery #780

Merged

auvipy reviewed Jun 6, 2020

View reviewed changes

toumorokoshi closed this Jun 13, 2020

Conversation

mauriciovasquezbernal commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

thedrow commented May 14, 2020

Uh oh!

ocelotl commented May 14, 2020

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toumorokoshi commented May 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thedrow left a comment

Choose a reason for hiding this comment

Uh oh!

majorgreys commented Jun 2, 2020

Uh oh!

thedrow commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

majorgreys commented Jun 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toumorokoshi commented Jun 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mauriciovasquezbernal commented May 4, 2020 •

edited

Loading

thedrow commented Jun 2, 2020 •

edited

Loading