Feat add dd and otel celery tasks #409
Conversation
Switch to pluggable design so that some monitoring functions can report to something other than New Relic. Still defaults to just New Relic, but new setting allows adding OpenTelemetry or Datadog, or removing New Relic. Initialization and configuration of OpenTelemetry is left as an exercise to the deployer, but https://github.com/mitodl/open-edx-plugins/tree/main/src/ol_openedx_otel_monitoring/ would be a likely candidate. Part of #389
- Add telemetry dependencies to test.in (including newrelic, even though that's also in base.in -- make it explicit) and run `make upgrade`. - Remove TestingBackend and test that the appropriate backend calls are being made. - Move `test_record_exception` from custom-monitoring to backends tests and add Datadog coverage - Add a similar test for fan-out of `set_custom_attribute`
# Conflicts: # CHANGELOG.rst # requirements/base.txt # requirements/dev.txt # requirements/doc.txt # requirements/quality.txt # requirements/test.txt
| @@ -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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| self.instrumentor().instrument() | ||
| else: | ||
| raise Exception( | ||
| "the worker_process_init celery signal must be provided for OpenTelemetry to monitor celery tasks." |
There was a problem hiding this comment.
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).
|
Based on Tim's comment, I went and looked into if we were getting what we needed from celery by default. To do that, I looked at the logs over the past month. Do the logs related to celery increase or decrease in volume when we enabled DD in edx-platform? |
|
@timmc-edx: FYI: I'm closing this PR. I assume at this point we do not need this. |
Description:
Adds an initialize_celery_monitoring util to allow celery tasks to be monitored by the Open Telemetry and Datadog providers. edx-platform PR to follow.
JIRA:
edx/edx-arch-experiments#584
Testing instructions:
edx_django_utils.monitoring.DataDogBackend]Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns:
List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.