Adding detailed logs and enabling the debug level to watch every step in prod #39
Conversation
…onfigure logs in a separate file logging_config.py that is imported to the __main__.py to trigger logs configuration before other modules inheret it; add debug logs to the watcher to better monitor each step
…e modules; implement logging to be configured globally but keep separate logger for each module; add level for logging to be configurable in the config file
…ct with all available logging level to validate user input from the config file
|
I thought this would be an easy change, but I need to dive more into this, to see the bigger picture. Hence I think this shouldn't be rushed too much then. I will try to come back to this on Thu or Fri. Thanks for your quick PR in any case! |
|
Regarding where to configure logging. It must be done before the repository and launcher classes are created, because stuff happens when they are created. Maybe that is not such a good idea, perhaps these classes should only start to actually do something at a later time. But that is how it is now, and I think fine for now. The global variables are all (or perhaps most) in app = Flask(__name__)
config = Config()
setup_logging(level=config.scrapyd().log_level)
repository = ...This still sets up logging when this file is loaded, but a lot happens already here when the file is loaded, that would need refactoring. If we can keep these 'side-effects' at this place, then it is already more readable / understandable. |
|
Yes, we can put logging to the entry point of the app, no problem, but I would also create a ticket (not urgent) to refactor this to adhere to better practices so we can discuss it as a team and maybe make it cleaner. But since we are hot fixing stuff, I can put everything together for now, no problem. Working on it. |
…api.py; keep logging_config file
scrapyd_k8s/api.py
Outdated
| @@ -3,8 +3,8 @@ | |||
| from .config import Config | |||
| config = Config() | |||
| from .logging_config import setup_logging | |||
| logging_level = config.scrapyd().get('logging_level', 'INFO') | |||
| setup_logging(logging_level) | |||
| log_level = config.scrapyd().get('logging_level', 'INFO') | |||
There was a problem hiding this comment.
uhm, and also the config option, primarily!
There was a problem hiding this comment.
What do you mean exactly? I don't understand the comment, sorry.
scrapyd_k8s/logging_config.py
Outdated
| numeric_level = logging.getLevelName(level_name) | ||
| if not isinstance(numeric_level, int): | ||
| raise ValueError( | ||
| f"Invalid logging level '{logging_level}'." | ||
| f"Invalid logging level '{log_level}'." | ||
| ) | ||
| logging.basicConfig( | ||
| level=numeric_level, |
There was a problem hiding this comment.
Do you really need a numeric level here?
logging.setLevel accepts both number and string.
There was a problem hiding this comment.
Numeric level is what returned by the getLevelName and yes we do need it because we validate user input. This is a normal way when we validate the input and then use the value returned by validation to move further with the code.
remove limited reconnection attempts; cap backoff time with 15 min; configure logs in a separate file logging_config.py that is imported to the main.py to trigger logs configuration before other modules inheret it; add debug logs to the watcher to better monitor each step