ID generator for celery extension #51
Conversation
…tch capabilities of the correlation id middleware * Added optional "header_key" param to asgi_correlation_id.extensions.celery.load_correlation_id * Added optional "generator" param to asgi_correlation_id.extensions.celery.load_correlation_id * Added optional "generator" param to asgi_correlation_id.extensions.celery.load_celery_current_and_parent_ids
|
I think this looks good 👍 Would you mind adding one or two tests, and would you mind updating the |
|
I am actually going to rethink this PR a bit. We have access to celery and we just realized we can't correlate between the actual task id and the id that these functions create. I'm still investigating, but it looks like the actual task id comes in through the kwargs of @task_prerun.connect(weak=False)
def worker_prerun(task: 'Task', **kwargs: Any) -> None:
...I am going to see if we can either provide a generator to match how it is currently done or use the default task id so we can correlate with flower. |
|
After investigating further, it is possible to get the task_id from the task_prerun hook. task_prerun = Signal(
name='task_prerun',
providing_args={'task_id', 'task', 'args', 'kwargs'},
)I am going to add this as an opt-in parameter so as not to break people's existing code. |
|
Not sure how the current implementation would break existing code, but sounds good. Let me know when it's ready for another look 👍 |
|
What is currently in this pr should not break existing code since I defaulted the new arguments with the values that were hardcoded. The addition I will be making that lets it use the celery task id will also not break existing code since I will make it Optional and opt-in. I will hopefully get to updating this this week. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@dapryor are you still working on this? 🙂 |
|
@sondrelg Yes :) Apologies. I have been busy with work. Will get back to this soon |
…ilter and standardized the string trimming process for the filtering logic * changed type from int to Optional[int] for optional parameter 'uuid_length' in CeleryTracingIdsFilter * changed default value from 32 to None for optional parameter 'uuid_length' in CeleryTracingIdsFilter * created and utilized function `_trim_string` to standardize string trimming logic in filters * added test to test string trimming for CeleryTracingIdsFilter * added test to ensure the default behavior of the new filter matched the behavior of the old filter. This assumes default generators are used
…he internally generated celery_id * Added optional argument `use_internal_celery_task_id` to `load_celery_current_and_parent_ids` * When `use_internal_celery_task_id` is set to True, the internal celery task ID will be used and the generator function will be ignored * Updated README with information on this new argument
|
@sondrelg I have updated the PR. It includes the signature change from #52 and the extra parameter for allowing the internal celery task id to be used. I honestly would like some help with the celery test since the current structure of those test is async with a session celery app. This doesn't seem to lend itself well to test that change signal functions of the session app. Plus celery testing is already a huge pain... |
sondrelg
force-pushed
the
main
branch
2 times, most recently
from
f0c6c88
to
237e183
Compare
|
Looks good, thanks @dapryor 👏 |
|
The new version (3.1.0) is now release. Would you mind verifying that it works ok for you @dapryor? |
|
@sondrelg everything seems to be working! will open an issue if i hit anything. |
Refer to #50
Please review and I can address any requested changes :)
Added customizable generator options to celery extension to better match capabilities of the correlation id middleware