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

Jobtastic Task uses the root logger #44

Open
rhunwicks opened this issue Aug 27, 2014 · 4 comments
Open

Jobtastic Task uses the root logger #44

rhunwicks opened this issue Aug 27, 2014 · 4 comments

Comments

@rhunwicks
Copy link
Contributor

I thought that the best practice was to get the logger at the top of the module:

import logging
logger = logging.getLogger(__name__)

class JobtasticTask(Task):

    def apply_async(self, args, kwargs, **options):
        if task_id:
            logger.info('Found existing cached and completed task: %s', task_id)

Currently, JobtasticTask just uses the root logger in a few places (logging.info('Found existing cached and completed task: %s', task_id)) which makes it hard to create special logging configurations for Tasks.

Is there a reason to use the root logger, or can I create a PR to change them?

@rhunwicks
Copy link
Contributor Author

For example, I have this in my logs:

[27/Aug/2014 04:14:20] INFO [root:256] Current status: FAILURE

Which I know is telling me that a task has failed and comes from logging.info('Current status: %s', task_meta.status) in JobtasticTask.apply_async but it isn't easy to work out what's going on without more context - specifically the actual task that failed, for example:

[27/Aug/2014 04:14:20] INFO [myapp.tasks.RefreshMaterializedView:256] Current status: FAILURE

@rhunwicks
Copy link
Contributor Author

Looking at the current Celery docs (http://celery.readthedocs.org/en/latest/userguide/tasks.html#logging) and bearing in mind the requirement for Celery 2.x compatibility, we could use:

try:
    from celery.utils.log import get_task_logger
    logger = get_task_logger(__name__)
except ImportError:
    # get_task_logger is new in Celery 3.X
    logger = logging.getLogger(__name__)

I think that would also allow use to remove all the logging setup in the Tasks, such as:

        if get_task_logger:
            self.logger = get_task_logger(self.__class__.__name__)
        else:
            # Celery 2.X fallback
            self.logger = self.get_logger(**kwargs)

@winhamwr
Copy link
Contributor

Is there a reason to use the root logger, or can I create a PR to change them?

No good reason at all. Your get_task_logger plus fallback solution looks splendid. I'd love to see a pull request.

Thanks for filing an issue and researching a solution!
-Wes

@ses4j
Copy link

ses4j commented Jun 13, 2017

This seems to have been completed partially, but there are still a bunch of calls to logging.info. They should probably be replaced with a local logger so they can be enabled/disabled.

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

No branches or pull requests

3 participants