Fix exception log #409
Fix exception log #409
Conversation
…ting" when printing a raised exception.
| @@ -54,11 +54,11 @@ def init_db_pool(): | |||
|
|
|||
| global connection_pool | |||
| if connection_pool is None: | |||
| logger.info("Initialize the database connection pool.", database=db, user=user, password=pw, host=host) | |||
| logger.info("Initialize the database connection pool.") | |||
There was a problem hiding this comment.
why do you delete the extra info? I agree that revealing the password might not be such a great idea, but the rest might actually be helpful in some cases...
| try: | ||
| connection_pool = ThreadedConnectionPool(db_min_connections, db_max_connections, database=db, user=user, password=pw, host=host) | ||
| except psycopg2.Error as e: | ||
| logger.warning("Can't connect to database") | ||
| logger.warning("Can't connect to database", exc_info=True) |
There was a problem hiding this comment.
There is also https://docs.python.org/3.7/library/logging.html#logging.Logger.exception which is a neat wrapper around logger.error(msg, exc_info=True). Might look cleaner.
I disagree with the log level. This should at least be ERROR, as it affects the correct workings of the system in a significant way. Well, nothing's gonna work properly without a db connection...
Same with most of the other log level in this PR.
There was a problem hiding this comment.
I indeed saw this method, but wrapping around the log level error. We were currently using warning, and my thought is that it should only be a warning for the entity service app (at this level): depending on the context, we may want to do something different. It is not yet an error as we are raising another exception which should become and error if not handled.
For example, if I request the entity service status which call the db to get the number of runs, if the database is not reachable, the application just need to return a 503 to the caller, asking to send the request back in a short time. The app is not in a wrong state. But when a task is trying to write something to the db and the db is not reachable, that may be logged as an error.
I would also argue that this specific service should handle the lost of connection to any other service it is using as it can, and will happen (e.g. the other service restarted, the database is reaching its bandwidth limits...). We are currently relying too much on the fact that everything will be up when requested.
In conclusion, I would even argue that it should only be a debug message here or even simply removed, but everytime we are using the DBConn context, we should explicitely say what to do if a ConnectionError occurs. This is slightly mentioned in the issue #408
But in the meanwhile and because we are not handling any of these, I'm agreeing with you that it should be an error.
To print an exception in a log, the line
fails raising: "TypeError: not all arguments converted during string formatting"
Instead, as we are mainly printing the exception in a
try exceptcontext, use the paramexc_info=Trueto print it.