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

Feat add dd and otel celery tasks #409

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ Change Log

.. There should always be an "Unreleased" section for changes pending release.


[5.14.0] - 2024-03-28
---------------------
Added
~~~~~
* Initial support for sending monitoring data from Celery to OpenTelemetry collector or Datadog agent.

[5.13.0] - 2024-04-30
---------------------
Added
Expand Down
2 changes: 1 addition & 1 deletion edx_django_utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
EdX utilities for Django Application development..
"""

__version__ = "5.13.0"
__version__ = "5.14.0"

default_app_config = (
"edx_django_utils.apps.EdxDjangoUtilsConfig"
Expand Down
4 changes: 4 additions & 0 deletions edx_django_utils/monitoring/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ Feature support matrix for built-in telemetry backends:
- ✅
- ❌
- ❌
* - Instrument celery tasks directly (``intialize_celery_task``)
- ❌
- ✅
- ✅

Using Custom Attributes
-----------------------
Expand Down
1 change: 1 addition & 0 deletions edx_django_utils/monitoring/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
accumulate,
background_task,
increment,
initialize_celery_monitoring,
record_exception,
set_custom_attribute,
set_custom_attributes_for_course_key
Expand Down
34 changes: 32 additions & 2 deletions edx_django_utils/monitoring/internal/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ def record_exception(self):
Record the exception that is currently being handled.
"""

@abstractmethod
def initialize_celery_monitoring(self, *args, **kwargs):
"""
Instrument celery to be monitored by the monitoring service.

Optional kwargs:
worker_process_init - required for open telemetry to integrate to the clery signal.
Import from from celery.signals.
"""


class NewRelicBackend(TelemetryBackend):
"""
Expand Down Expand Up @@ -77,14 +87,17 @@ def record_exception(self):
# https://docs.newrelic.com/docs/apm/agents/python-agent/python-agent-api/recordexception-python-agent-api/
newrelic.agent.record_exception()

def initialize_celery_monitoring(self, *args, **kwargs):
pass


class OpenTelemetryBackend(TelemetryBackend):
"""
Send telemetry via OpenTelemetry.

Requirements to use:

- Install `opentelemetry-api` Python package
- Install `opentelemetry-api` and `opentelemetry-instrumentation-celery` Python packages.
- Configure and initialize OpenTelemetry

API reference: https://opentelemetry-python.readthedocs.io/en/latest/
Expand All @@ -93,7 +106,9 @@ class OpenTelemetryBackend(TelemetryBackend):
def __init__(self):
# If import fails, the backend won't be used.
from opentelemetry import trace
from opentelemetry.instrumentation.celery import CeleryInstrumentor
self.otel_trace = trace
self.instrumentor = CeleryInstrumentor

def set_attribute(self, key, value):
# Sets the value on the current span, not necessarily the root
Expand All @@ -103,6 +118,17 @@ def set_attribute(self, key, value):
def record_exception(self):
self.otel_trace.get_current_span().record_exception(sys.exc_info()[1])

def initialize_celery_monitoring(self, *args, **kwargs):
worker_process_init = kwargs.get('worker_process_init', None)
if worker_process_init is not None:
@worker_process_init.connect(weak=False)
def init_celery_tracing(*args, **kwargs):
self.instrumentor().instrument()
else:
raise Exception(
"the worker_process_init celery signal must be provided for OpenTelemetry to monitor celery tasks."
Copy link
Contributor

Choose a reason for hiding this comment

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

If OpenTelemetry needs it, then it will have to be passed to all the backends, because there will only be one call site for the initialize_celery_monitoring API call we expose. However, it looks like it doesn't need to be passed in at all—this is just a signal, so we could from celery.signals import worker_process_init.

For that matter, it might make the most sense to instead say that the caller should do that, like so:

@celery.signals.worker_process_init.connect(weak=False)
def init_celery_tracing(*args, **kwargs):
    edx_django_utils.monitoring.initialize_celery_monitoring()

We could also just leave the OpenTelemetry side of this unimplemented for now, since we won't be able to give it the testing it deserves (in production).

)


class DatadogBackend(TelemetryBackend):
"""
Expand All @@ -118,8 +144,9 @@ class DatadogBackend(TelemetryBackend):
# pylint: disable=import-outside-toplevel
def __init__(self):
# If import fails, the backend won't be used.
from ddtrace import tracer
from ddtrace import patch, tracer
self.dd_tracer = tracer
self.patch = patch

def set_attribute(self, key, value):
if root_span := self.dd_tracer.current_root_span():
Expand All @@ -129,6 +156,9 @@ def record_exception(self):
if span := self.dd_tracer.current_span():
span.set_traceback()

def initialize_celery_monitoring(self, *args, **kwargs):
self.patch(celery=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like ddtrace patches Celery by default: https://github.com/DataDog/dd-trace-py/blob/01fbf9127180794392cc7dbe16acd610087dacf6/ddtrace/_monkey.py#L33 -- were you seeing otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just going by these docs: https://ddtrace.readthedocs.io/en/stable/integrations.html#celery.
This was in the ticket I had created: edx/edx-arch-experiments#584
Maybe it was inaccurate?

Separately: In those docs I see that trace headers are off by default. We should find out if that is what we want from DD? Maybe long running tasks would kill our traces? Not sure. @connorhaugh: Can you task/track this?



# We're using an lru_cache instead of assigning the result to a variable on
# module load. With the default settings (pointing to a TelemetryBackend
Expand Down
11 changes: 9 additions & 2 deletions edx_django_utils/monitoring/internal/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
We try to keep track of our custom monitoring at:
https://openedx.atlassian.net/wiki/spaces/PERF/pages/54362736/Custom+Attributes+in+New+Relic

At this time, the custom monitoring will only be reported to New Relic.

"""
from .backends import configured_backends
from .middleware import CachedCustomMonitoringMiddleware
Expand Down Expand Up @@ -110,3 +108,12 @@ def noop_decorator(func):
return newrelic.agent.background_task(*args, **kwargs)
else:
return noop_decorator


def initialize_celery_monitoring(*args, **kwargs):
"""
Set monitoring custom attribute.
This is not cached.
"""
for backend in configured_backends():
backend.initialize_celery_monitoring(*args, **kwargs)
20 changes: 18 additions & 2 deletions edx_django_utils/monitoring/tests/test_backends.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
"""
Tests for TelemetryBackend and implementations.
"""
from unittest.mock import patch
from unittest.mock import Mock, patch

import ddt
import pytest
from django.test import TestCase, override_settings

from edx_django_utils.monitoring import record_exception, set_custom_attribute
from edx_django_utils.monitoring import initialize_celery_monitoring, record_exception, set_custom_attribute
from edx_django_utils.monitoring.internal.backends import configured_backends


Expand Down Expand Up @@ -151,3 +151,19 @@ def test_record_exception(
mock_nr_record_exception.assert_called_once()
mock_otel_record_exception.assert_called_once()
mock_dd_span.assert_called_once()

# Record exception on current span, not root span.
@patch('ddtrace.patch')
def test_initialize_celery_monitoring(
self, mock_dd_patch
):
celery_signal_decorator_mock = Mock()
with override_settings(OPENEDX_TELEMETRY=[
'edx_django_utils.monitoring.NewRelicBackend',
'edx_django_utils.monitoring.OpenTelemetryBackend',
'edx_django_utils.monitoring.DatadogBackend',
]):
initialize_celery_monitoring(worker_process_init=celery_signal_decorator_mock)
mock_dd_patch.assert_called_once()
# celery hook should be defined, but not called.
self.assertFalse(celery_signal_decorator_mock.called)
8 changes: 4 additions & 4 deletions requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,19 @@ django-crum==0.7.9
# via -r requirements/base.in
django-waffle==4.1.0
# via -r requirements/base.in
newrelic==9.8.0
newrelic==9.9.0
# via -r requirements/base.in
pbr==6.0.0
# via stevedore
psutil==5.9.8
# via -r requirements/base.in
pycparser==2.21
pycparser==2.22
# via cffi
pynacl==1.5.0
# via -r requirements/base.in
sqlparse==0.4.4
sqlparse==0.5.0
# via django
stevedore==5.2.0
# via -r requirements/base.in
typing-extensions==4.10.0
typing-extensions==4.11.0
# via asgiref
10 changes: 5 additions & 5 deletions requirements/ci.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,27 @@ colorama==0.4.6
# via tox
distlib==0.3.8
# via virtualenv
filelock==3.13.3
filelock==3.14.0
# via
# tox
# virtualenv
packaging==24.0
# via
# pyproject-api
# tox
platformdirs==4.2.0
platformdirs==4.2.1
# via
# tox
# virtualenv
pluggy==1.4.0
pluggy==1.5.0
# via tox
pyproject-api==1.6.1
# via tox
tomli==2.0.1
# via
# pyproject-api
# tox
tox==4.14.2
tox==4.15.0
# via -r requirements/ci.in
virtualenv==20.25.1
virtualenv==20.26.1
# via tox
Loading
Loading