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

[Question] About logger #1168

Closed
AndreaLanfranchi opened this issue Apr 6, 2024 · 6 comments
Closed

[Question] About logger #1168

AndreaLanfranchi opened this issue Apr 6, 2024 · 6 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@AndreaLanfranchi
Copy link
Contributor

Just wondering what's the rationale about logger instancing:

  • module optional argument even if valued is not considered in any log line
  • actually all modules where logger is instantiated have their own instance while a singleton should be more than enough

Thank you

@serengil serengil added the question Further information is requested label Apr 6, 2024
@serengil
Copy link
Owner

serengil commented Apr 6, 2024

Yeah, using logger similar to base models with singleton is okay but we will miss the module information in that case. With this approach, we can include module name into log messages easily. That is why, I added it but not used yet.

TLDR: I prefer to adopt the current approach.

@AndreaLanfranchi
Copy link
Contributor Author

Just for the sake of discussion I see some possible improvements with that approach.

  1. Providing module name by string constant might break the coupling amongst actual module name and what is written in the instantiation statement if the module gets renamed. Would suggest to provide the module name from __name__ variable
  2. Even implementing a singleton the module name of the function calling any log activity can be easily determined examining the stack. Here a sample.

This helps a lot reducing the amount of code being written and does not require to remember to update string constants on module renaming (if ever).

@serengil
Copy link
Owner

serengil commented Apr 6, 2024

1- Agree. Instead of string module name _ name _ will avoid mistakes. But end user is not using logger at all. So, giving wrong module name should not be happened.

2- suppose that one customize the logger and move logs to kafka instead of console. You will see module name in the event message. So, I think having module name is important in the logger.

@AndreaLanfranchi
Copy link
Contributor Author

AndreaLanfranchi commented Apr 6, 2024

1- Agree. Instead of string module name 'name' will avoid mistakes. But end user is not using logger at all. So, giving wrong module name should not be happened.

I don't mean this should impact user experience. I'm considering developer experience and code maintainability. Say you rename GostFaceNet.py to whatever.py ... you'd also need to change this accordingly

logger = Logger(module="basemodels.GhostFaceNet")

otherwise, in case the codes printout a logline with the module name (useful for debugging and asking for support) you might end up with an inconsistent info

2- suppose that one customize the logger and move logs to kafka instead of console. You will see module name in the event message. So, I think having module name is important in the logger.

Not disagreeing on that. I'm only saying you can get the name of the calling module at runtime even using a singleton instance of the logger.

@serengil
Copy link
Owner

serengil commented Apr 6, 2024

I did not disagree. Just thinking out loud.

I will play with the logger then.

Thank you for your comment and feedback.

@serengil serengil added the enhancement New feature or request label Apr 6, 2024
@serengil
Copy link
Owner

serengil commented Apr 8, 2024

Closed with PR - #1177

@serengil serengil closed this as completed Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants