Celery (re)instrumentation

[lrafeei]

This PR picks new instrumentation points for Celery support to allow for Custom Task classes.

[github-actions]

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Fixed	Errors	Warnings	Elapsed time
✅ ACTION	actionlint	6		0	0	0.73s
✅ MARKDOWN	markdownlint	7	0	0	0	1.1s
✅ MARKDOWN	markdown-link-check	7		0	0	26.29s
✅ PYTHON	ruff	922	5	0	0	0.83s
✅ PYTHON	ruff-format	922	6	0	0	0.29s
✅ YAML	prettier	13	0	0	0	1.33s
✅ YAML	v8r	13		0	0	4.2s
✅ YAML	yamllint	13		0	0	0.6s

See detailed report in MegaLinter reports

[codecov-commenter]

Codecov Report

❌ Patch coverage is 61.90476% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.31%. Comparing base (02d0071) to head (b2b19b3).

Files with missing lines	Patch %	Lines
newrelic/hooks/application_celery.py	61.29%	15 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1429      +/-   ##
==========================================
- Coverage   81.37%   81.31%   -0.07%     
==========================================
  Files         208      208              
  Lines       23566    23585      +19     
  Branches     3717     3719       +2     
==========================================
+ Hits        19176    19177       +1     
- Misses       3132     3150      +18     
  Partials     1258     1258              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TimPansino]

Nitpick, can we move these (and the tasks that use them) to the bottom of the file so the test app's functionality escalates in complexity rather than starting with the most confusing things?

[TimPansino]

I'm not sure I understand what this is doing, and the comment is insufficient.

From inspecting execution, task.__module__ = 'celery.app.builtins' and wrapped.__module__ = 'celery.app.trace'.

"Ensure the module is set" doesn't really clarify what it's being set to, and "for monkeypatching detection" is confusing. What is doing the detecting and why is that important?

I disabled this line and the tests pass 100%, so if there's some reason to be doing this we need a test for it.

[lrafeei]

Defining the TaskWrapper where the run() method just calls the existing __call__ method covers the case that the overriding of the __module__ attribute covers. I believe this was a way to optimize functionality.

Using the scenario where the __call__ method has not been overridden and the run() method has been defined:
Without overriding the __module__ attribute, the task tracer will simply use the run() method and bypass the __call__ function. Then, the TaskWrapper class will reroute that back to the __call__ method. When overriding the __module__ attribute, the task tracer will see that the __call__ method has been overridden (which it does by traversing the MRO), and use that.

[lrafeei]

That being said, whoops. That is supposed to be task.__module__ = wrap_task_call.__module__ (so wrapper function instead of wrapped function)

I figured it would be faster execution to keep that line in there, but I don't have strong feelings about removing it either.

[TimPansino]

I presume with removing these hooks we don't support some older versions of Celery outside our support window, but let's be sure to call that out in the release notes as I'm sure some customers out there use ancient versions of Celery.

[TimPansino]

The names of all these tests in this file have gotten so long they don't actually describe what they do. Can we shorten all of these and clarify what we're actually testing? Something like this maybe to trim out as many words as possible.

Suggested change

	def test_celery_task_distributed_tracing_inside_background_task(dt_enabled):
	def test_distributed_tracing_inside_txn_apply_async(dt_enabled):

[TimPansino]

This comment and the test don't make sense to me.

This test is for Celery's .apply() method which is immediate and local execution of the task no?

So the reason there's no distributed tracing headers to accept is because this isn't a distributed system, and Celery isn't handling the transaction?

We should validate that no DT headers are accepted for this then with forgone metrics by setting it to None like the above tests do when DT is disabled.

[TimPansino]

If all of that is true, why is the test after this seeing DT headers and accepting them? Isn't that also local exection? Where is it even getting these headers from to accept?

[lrafeei]

I've added a separate test for this, where a task is called within a transaction (background task) and insert_distributed_trace_headers()

Basically, delay/apply_async create worker processes and apply/__call__ use the main process to execute. In the case of a task being called in a function that is wrapped with the background_task decorator, any tasks called with delay/apply_async will create a worker process to execute the task and will no longer be in the context of the transaction. Conversely, any tasks called with apply/__call__ will execute in the main process and will be a function trace of the existing transaction.

[TimPansino]

Additional thought, can't you call tasks directly like functions? If that's not different execution-wise from calling .apply(), shouldn't we test that here as well the same as we test .apply()?

[lrafeei]

Good point, I meant to add a direct call test as well. The execution path is different from .apply() even though they both are executed on the main process.