Do not keep task id reference indefinitely in Celery instrumenation#3690
Do not keep task id reference indefinitely in Celery instrumenation#3690rbagd wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
rbagd
changed the title
| task_runtime_estimated = (default_timer() - start_time) * 1000 | ||
|
|
||
| metrics = self.get_metrics() | ||
| self.assertEqual(CeleryInstrumentor().task_id_to_start_time, {}) |
There was a problem hiding this comment.
Should another test be added where the task_id does not exist and the pop operation returns None instead?
There was a problem hiding this comment.
Would that test matter actually? We're not testing return value of the pop here in any case. I may not see your point. Can you maybe explain the case you have in mind?
| self.task_id_to_start_time.get(task_id), | ||
| attributes=metric_attributes, | ||
| ) | ||
| self.task_id_to_start_time.pop(task_id, None) |
There was a problem hiding this comment.
What is that we are leaking? On line 355 we are setting this a time object so don't expect to keep alive something on the celery side?
There was a problem hiding this comment.
task_id_to_start_time is a dictionary initialised with CeleryInstrumentor. Each time a task finishes the dictionary is augmented with a record of time spent for that task id. In the current state the dictionary just keeps growing even though the task finished long time ago (task identifiers are normally unique). There are no references to any other Python objects that prevent GC, it's just this relatively small issue.
Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
|
This PR has been closed due to inactivity. Please reopen if you would like to continue working on it. |
github-project-automation
bot
moved this from Ready for review
to Done
in Python PR digest
3 participants
Description
Reference to a task identifier as a string is kept indefinitely by the instrumentation. This just makes sure it gets removed once no longer needed.
Related #3458
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.