PolicyStat / jobtastic

Make your user-responsive long-running Celery jobs totally awesomer.
http://policystat.github.com/jobtastic/
MIT License
644 stars 61 forks source link

Jobtastic Task uses the root logger #44

Open rhunwicks opened 10 years ago

rhunwicks commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 7 years ago

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.