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

Add new setting: 'task_args_repr_function'. #6595

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arnau126
Copy link

@arnau126 arnau126 commented Jan 13, 2021

Description

Add new setting: task_args_repr_function (perhaps there is a more appropriate name).
This setting allows you to choose which function do you want to use to generate argsrepr and kwargsrepr (the string representation of the task args and kwargs used for logging purposes).

Default is "celery.utils.saferepr.saferepr" which is the currently used function.

Examples

def task_args_repr_function(o, **kwargs):
    return json.dumps(o)

so it will be easier to parse them later on in some backends like the one of django-celery-results.

or

def task_args_repr_function(o, **kwargs):
    if isinstance(o, dict):
        o = o.copy()
        if 'card_number' in o:
            o['card_number'] = '***'
        if 'phone_number' in o:
            o['phone_number'] = o['phone_number'][:3]
        if 'email' in o:
            o.pop('email')
    return repr(o)

so we don't have to specify kwargsrepr param at each task call.

Tests

Tests not done yet. Please let me know if you agree with this approach, and I will add some tests.

@auvipy
Copy link
Member

auvipy commented Jan 13, 2021

it will tke sometime to review & reach a consensus. I will start tomorrow

Copy link
Member

@thedrow thedrow left a comment

Choose a reason for hiding this comment

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

I have a few comments.
This requires tests to be merged, as well.

@cached_property
def repr(self):
repr_function = self.app.conf.task_args_repr_function
if not repr_function:
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, this fallback is inappropriate for most if not all cases.
We should raise an ImproperlyConfigured exception instead if that is the case.

Copy link
Author

@arnau126 arnau126 Jan 20, 2021

Choose a reason for hiding this comment

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

With task_protocol=1, the backend receives the original args and kwargs values (i.e. as tuple and dict)
With this fallback I was trying to get the same.
Anyway, I will just raise ImproperlyConfigured as you suggest.
Explicit is better than implicit.

if not repr_function:
return lambda o, **kwargs: o
elif isinstance(repr_function, str):
return mlazy(symbol_by_name, repr_function)
Copy link
Member

Choose a reason for hiding this comment

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

We should test if repr_function is a callable.
If it isn't this should also raise an ImproperlyConfigured exception.

``task_args_repr_function``
~~~~~~~~~~~~~~~~~

.. versionadded: 5.?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. versionadded: 5.?
.. versionadded: 5.1

@thedrow
Copy link
Member

thedrow commented Mar 29, 2021

@arnau126 This is a friendly ping to remind you to complete this pull request.

@xirdneh
Copy link
Member

xirdneh commented Apr 27, 2021

@arnau126 If you don't have time to finish this PR I can take over.

@arnau126
Copy link
Author

@thedrow, sorry I'm very busy these days.
@xirdneh I'd be very grateful if you take over the PR. Thanks!

@auvipy
Copy link
Member

auvipy commented Oct 5, 2021

@thedrow, sorry I'm very busy these days. @xirdneh I'd be very grateful if you take over the PR. Thanks!

what remains to draw the finishing line?

@auvipy auvipy modified the milestones: 5.2, 5.3 Oct 5, 2021
@AidaPaul AidaPaul mentioned this pull request Oct 7, 2021
@auvipy
Copy link
Member

auvipy commented Dec 17, 2021

@thedrow, sorry I'm very busy these days. @xirdneh I'd be very grateful if you take over the PR. Thanks!

can you return to this now?

@xirdneh
Copy link
Member

xirdneh commented Dec 17, 2021

@auvipy Hey, sure thing I can take over.

@auvipy
Copy link
Member

auvipy commented Dec 17, 2021

ok great

@saichander17
Copy link

Hi all,
Just checking to see if someone is working on this PR and any estimate on when this would be released.
@xirdneh

@auvipy
Copy link
Member

auvipy commented Mar 9, 2022

no one is working. you can take over

@berryscone
Copy link

any further progress on it?

@auvipy auvipy closed this Dec 8, 2022
@auvipy auvipy reopened this Dec 8, 2022
@auvipy
Copy link
Member

auvipy commented Dec 17, 2022

no we need new contributors to take this over

@Nusnus Nusnus modified the milestones: 5.3, Future Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants