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

Set Task Shadow Names as the Primary Task Name #469

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

andreangelucci
Copy link

@andreangelucci andreangelucci commented Mar 6, 2025

Celery tasks have the shadow_name property used to define a custom name for tasks.
When a task has multiple executions, it becomes challenging to find their task results as they share the same task names, and the task IDs are usually UUIDs. Shadow names are useful to filter and identify results for monitoring.

This PR sets the shadow name as the primary choice for the TaskResult.task_name property. If it's not set, the default task_name will be used.

Issue #468 and #379

@andreangelucci andreangelucci changed the title Set Task Shadow Names as the Primary Task Name Set Task Shadow Names as the Primary Task Name #468 Mar 6, 2025
@andreangelucci andreangelucci changed the title Set Task Shadow Names as the Primary Task Name #468 Set Task Shadow Names as the Primary Task Name Mar 6, 2025
@andreangelucci andreangelucci changed the title Set Task Shadow Names as the Primary Task Name #468: Set Task Shadow Names as the Primary Task Name Mar 6, 2025
@andreangelucci andreangelucci changed the title #468: Set Task Shadow Names as the Primary Task Name Set Task Shadow Names as the Primary Task Name Mar 6, 2025
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

thanks for opening the PR. I will need some thorough review and thoughts on this before proceed

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

as you can see, all the CI builds are failing due to this change. so we need proper tests cases to validate and avoid regression

@andreangelucci
Copy link
Author

Hey, @auvipy , thanks for the feedback!
I fixed the failing unit tests by adding the missing shadow property to the mocks.

@andreangelucci andreangelucci marked this pull request as ready for review March 11, 2025 17:55
@auvipy
Copy link
Member

auvipy commented Mar 12, 2025

This PR sets the shadow name as the primary choice for the TaskResult.task_name property. If it's not set, the default task_name will be used.

I am thinking about changing the behavior, if it has any negetive effect on existing projects?

@andreangelucci
Copy link
Author

I am thinking about changing the behavior, if it has any negetive effect on existing projects?

@auvipy I agree, this will change the current behavior. The idea came up when I noticed that shadow names are the best way to add custom identification to a task, yet the task result lacks this information. I've added more details here: #468

Alternatively, we could create a new field in the model. What do you think about this?

@andreangelucci andreangelucci requested a review from auvipy March 14, 2025 17:14
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

so our concerns here are, if this break existing behavior? and can we keep the old behavior as primary and the shadow as secondary? need more thoughts to reach on a consensus. I'm open to more discussions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants