Logging clean up #3518
Logging clean up #3518
Conversation
There was a problem hiding this comment.
The %{function} placeholder works well for me:
debug mixxx::Logger::debug [aoide] mixxx::network::WebTask - aoide::ListCollectionsTask(0xce066a0) Received reply for request QUrl("http://[::1]:37719/c")
debug WTrackTableView::loadTrackModel [Main] WTrackTableView::loadTrackModel() LibraryTableModel(0x3f62220)
debug BaseSqlTableModel::select [Main] LibraryTableModel(0x3f62220) select() took 0 ms 45
debug WLibrary::switchToView [Main] trying to restore position
debug GLSLWaveformWidget::GLSLWaveformWidget [Main] Created QGLWidget. Context Valid: true Sharing: true
This will become even more useful once we drop our custom logging wrapper in favor of logging categories.
|
Probably only works for debug builds. |
Probably. Then we need conditional compilation depending on if debug symbols are available or not. |
If this only works for some conditions, we unfortunately can't rely on this. We need to keep this in mind for future considerations. Can we remove the function part for all? I cannot remember that I have missed that info, and in hard cases it can be made visible by the environment variable. This will speed up the normal case and produces shorter and better readable debug messages. |
Please enclose in an ifdef. This is valuable information as the example I published demonstrates. Otherwise you loose the context.
|
|
If we keep the function, can we put if after the thread name? I find it much harder to read at the moment. |
On one hand you are right, but the issue is that currently the info is only sometimes available. The author of the message needs to make sure that it is understandable without the function name. Practically it is not much of a difference to find in files for "WLibrary::switchToView" or for "trying to restore position vs. debug". The other question rises, why only for qDebug(). I have just checked how it is implemented. Qt uses the compiler generated function name PRETTY_FUNCTION which has no runtime overhead. It is controlled via a QT_MESSAGELOGCONTEXT flag. So if we need to rely on this feature, we have the option to have it always enabled. For now I like to disable it, to not have an inconsistent state and merge this PR as it is. Waiting for a consistent solution. In general I am in the opinion to not clutter the output by default with function names using the self explaining messages. What do you think? |
|
We still have lots of legacy log messages where the class name is explicitly logged as part of the log message, which causes lots of inconsistencies. Instead this context should be provided implicitly and added to the log message. Without context many log messages are not self-contained. I don't want to lookup each and every log message in the IDE for the context when reading a log file. This is not feasible. Please also note that we still misuse the debug log level as info. I agree that info/warn/error log messages should be comprehensible without access to the source code. |
|
Yes, that would work for me. So we have these todos:
Unfortunately I have no interest to do this within this PR. Can we merge this to go back to the old somehow consistent state and than do the transition in another PR once one finds time. |
|
Could we keep the removed template pattern in a TODO comment? As a blueprint for re-adding it later. In this case it makes sense to preserve deleted code snippets to avoid reinventing the wheel later. |
|
We still have it in the git history. |
We need a TODO comment that mentions the extension anyway. Including that part of the template pattern for clarification and illustration would help to rediscover it. |
|
If you remove the {function} pattern, then please don't set if QT_MESSAGE_PATTERN env is set. This way, the user can still use a custom pattern that includes it. |
|
In general, comments are better than none, so I will summerize the state of discussion.
Yes right. This behaviour does not change here. Qt takes care of it. No need to replicate the logic in Mixxx. |
|
There is an unrelated issue with libkeyfinder |
this removes the confusing
debug unknown
entries
and a redundant check.