Skip to content
This repository has been archived by the owner on Jan 19, 2018. It is now read-only.

Implement LoggerAdapter to provide extended paths in the output. Improve #571 #572

Closed

Conversation

concaf
Copy link
Contributor

@concaf concaf commented Feb 24, 2016

screenshot from 2016-02-24 22-23-17

A logger adapter has been created in applogging.py, and the logging instance created by it is the same which is being used by other files, unlike before when each file created their own instance.

This is done so that additional contextual output can be outputted in the logging messages, which is currently only the filename which is making the logging calls.

In the other files, only one import (from atomicapp.applogging import Logging) and one initialization (logger = Logging.global_logger(__file__)), and it's done.

The code is poor, but it would be great if I could get some feedback on the approach and where can it be improved.

Also, I have not touched the cockpit logger and not included the check for --verbose, will do once this is reviewed.

P.S. Sorry for the changes around imports, the IDE rearranged them according to PEP8.

@cdrage
Copy link
Member

cdrage commented Feb 24, 2016

I know it's a pain, but is it possible to revert the changes made from the IDE? We can make the PEP8 changes after in a separate commit.

This will make the commit history a lot cleaner.

Also, any reason why it's reporting the compiled files (.pyc) rather than .py?

Otherwise, unit + integration tests pass. I'll comment on the code after the commit is updated :)

@concaf concaf force-pushed the debug_log_expand_pathnames branch from e0c4915 to 7b52749 Compare February 25, 2016 07:14
@concaf
Copy link
Contributor Author

concaf commented Feb 25, 2016

@cdrage
Removed the PEP-8 changes.

And as for .pyc files, I'm not sure. It sometimes shows .pyc and the other times .py (see image above), but should it matter since these are debug messages and we get the file names just fine?

class Logging:
class AtomicappLoggingAdapter(logging.LoggerAdapter):
"""
A class to pass contextual information to logs.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of "A class". Maybe "A logging adapter?" Bit more descriptive :)

@concaf
Copy link
Contributor Author

concaf commented Feb 26, 2016

@cdrage Thanks for the pointers. I'll work on the comments.
Could you review the code once you get time, so that I know whether to proceed with this approach or not.
Then I'll work on the comments, --verbose and the cockpit logger part.

@cdrage
Copy link
Member

cdrage commented Feb 26, 2016

@containscafeine this all looks good to me 👍 feel free to work on the other stuff.

I'll await for @dustymabe for secondary review

@dustymabe
Copy link
Contributor

@containscafeine - can you look at dustymabe@e2c57cc and give me your input? I think solving this in a way that uses the facilities of the logging library (i.e. creating a custom formatter) is a better way.

@concaf
Copy link
Contributor Author

concaf commented Mar 1, 2016

@dustymabe I knew I was unnecessarily complicating stuff 🤦, but on another note, at the moment we have to modify just the pathname, which we can deal with using customOutputFormatter, but what if we have to pass in some contextual information to the logs, say, the atomicapp name, we can modify the LoggerAdapter then.
For this particular issue, it's not required, but does it make sense later or do we just modify the logging messages in the future?

@dustymabe
Copy link
Contributor

@containscafeine I would like to not modify our logging structure unless we have a need to do so. If you think having the longer filename is something you want in right now then I say we go with something like dustymabe@e2c57cc for now.

What do you think?

@concaf
Copy link
Contributor Author

concaf commented Mar 2, 2016

+1 for dustymabe@e2c57cc

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

Successfully merging this pull request may close these issues.

3 participants