CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
OE-11 Logging revamp (#11003) for 4.2.x (draft (logging_20190220)) #13909
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
Conversation
b3f47cd does not fix all problems of order dependency between log tag registration and config parsing/application. The next commit will focus on testing (formalizing) this aspect of LogTagManager and see how it can be fixed. |
Some non-critical commits have been taken off this branch (logging_20190220) and moved to dead-end branches:
|
See spec revised 2019-03-20 https://github.com/kinchungwong/opencv_OE11_logging_11003_wiki/blob/master/spec_revised_20190320.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Well done!
Please take a look on comments below.
if (cv_temp_msglevel >= (CV_LOG_STRIP_LEVEL)) break; \ | ||
auto cv_temp_logtagptr = CV_LOGTAG_PTR_CAST(CV_LOGTAG_EXPAND_NAME(tag)); \ | ||
if (!cv_temp_logtagptr) cv_temp_logtagptr = CV_LOGTAG_PTR_CAST(CV_LOGTAG_FALLBACK); \ | ||
if (!cv_temp_logtagptr) cv_temp_logtagptr = CV_LOGTAG_PTR_CAST(CV_LOGTAG_GLOBAL); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can try to utilize C++ "magic" for that.
I will take a look on this later (perhaps in a separate PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this in the most verbose way in order to make its internal workings obvious, for code review (design review) purpose.
The current code looks like it is doing null-pointer-coalescing (something available in C# as an operator), but there can be more logic added to it in between the lines. For example, it may perform additional log stripping checks. If it turns out the flexibility is not needed, we can discuss ways to simplify the code.
I am open to proposals that take away one or more "log tag injection mechanisms" -
- tag argument passed into the macro
- I believe it is worth keeping
- "re-definable"
CV_LOGTAG_FALLBACK
macro- good for getting existing OpenCV code to provide log tags while minimizing scope of code changes
- bad because may confuse many C++ analysis and refactoring tools. Some tools do not support/allow re-defining macros.
- ultimate
CV_LOGTAG_GLOBAL
macro- alternative: we can simply combine this functionality with
CV_LOGTAG_FALLBACK
, in other words, the default value forCV_LOGTAG_FALLBACK
would be the global log tag.- but if we want to accept this alternative (getting rid of
CV_LOGTAG_GLOBAL
), first we need accept that macros can be re-defined.
- but if we want to accept this alternative (getting rid of
- alternative: we can simply combine this functionality with
#define CV_LOG_WARNING(tag, ...) CV_LOG_WITH_TAG(tag, cv::utils::logging::LOG_LEVEL_WARNING, __VA_ARGS__) | ||
#define CV_LOG_INFO(tag, ...) CV_LOG_WITH_TAG(tag, cv::utils::logging::LOG_LEVEL_INFO, __VA_ARGS__) | ||
#define CV_LOG_DEBUG(tag, ...) CV_LOG_WITH_TAG(tag, cv::utils::logging::LOG_LEVEL_DEBUG, __VA_ARGS__) | ||
#define CV_LOG_VERBOSE(tag, v, ...) CV_LOG_WITH_TAG(tag, (cv::utils::logging::LOG_LEVEL_VERBOSE + (int)(v)), __VA_ARGS__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOG_LEVEL_VERBOSE + (int)(v)
Need to align this expression with:
CV_LOG_STRIP_LEVEL
definition like(CV_LOG_LEVEL_VERBOSE + 1)
setLogLevel(CV_LOG_LEVEL_VERBOSE)
call
My initial design assumes of adding of dedicated verbosity level variable (like tag's msgLevel). However we can merge them (msgLevel + verbosityLevel) in this way ("adding" of these values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Options:
- Combine "logLevel" and "v" into one value.
- A hack, with unintended consequences. I'm okay with removing this hack, but first we need to choose one of the alternatives below.
- Logging macro will use both "logLevel" and "v" for log filtering.
- Difficult. I haven't found a way to implement this. If anyone has idea, please let me know.
- Logging macro will pass "v" into
writeLogMessageEx
function, without processing it.- Logging macro is not responsible for filtering based on value of "v"
- If
writeLogMessageEx
wants to filter value of "v", user need to provide a user-defined backend. - Normally,
writeLogMessageEx
just print the value of "v" as part of log message.
- Logging macro will convert value of "v" into a string and concatenate it to the logging message.
writeLogMessageEx
will not receive the value unless it parses the log message.
Part of #11003, incomplete. Should pass build. Moved LogLevel enum to logger.defines.hpp LogTag struct used to convey both name and log level threshold as one argument to the new logging macro. See logtag.hpp file, and CV_LOG_WITH_TAG macro. Global log level is now associated with a global log tag, when a logging statement doesn't specify any log tag. See getLogLevel and getGlobalLogTag functions. A macro CV_LOGTAG_FALLBACK is allowed to be re-defined by other modules or compilation units, internally, so that logging statements inside that unit that specify NULL as tag will fall back to the re-defined tag. Line-of-code information (file name, line number, function name), together with tag name, are passed into the new log message sink. See writeLogMessageEx function. Fixed old incorrect CV_LOG_VERBOSE usage in ocl4dnn_conv_spatial.cpp.
Added LogTagManager. This is an initial version, using standard C++ approach as much as possible, to allow easier code review. Will optimize later. A workaround for all static dynamic initialization issues is implemented. Refer to code comments.
Note: new code does not fully handle old log config parsing behavior.
The intention of this commit is to illustrate the capabilities of the current design of LogTagConfigParser. The test contained in this commit is not complete. Also, design changes may require throwing away this commit and rewriting test code from scratch. Does not test whitespace segmentation (multiple tags on the config); will do in next commit.
This macro allows to be re-defined locally in other compilation units to apply a prefix to whatever argument is passed as the "tag" argument into CV_LOG_WITH_TAG. The default definition in logger.hpp does not modify the argument. It is recommended to include the address-of operator (ampersand) when re-defined locally.
See test_logtagmanager.cpp Failed tests are: non-global ("something"), setting level by name-part (first part or any part) has no effect at all.
The code change is intended to approximate the spec documented in https://gist.github.com/kinchungwong/ec25bc1eba99142e0be4509b0f67d0c6 Refer to test suite in test_logtagmanager.cpp Filter test result in "opencv_test_core" ... with gtest_filter "LogTagManager*" To see the test code that finds the bugs, refer to original commits (before rebase; might be gone) .. f345120 (2019-03-03T19:45:17Z) .... LogTagManagerTests substring non-confusion tests .. 1b848f5 (2019-03-03T01:55:18Z) .... Added a few tests for LogTagManager, some fail.
See test_logtagmanager.cpp in modules/core/test.
The forced push on 2019-04-20 from 8bec94b to cc98252 resolves some code review issues. However, the following issues require additional discussions:
Edit |
LogTagConfigParser::parseLogLevel - as part of resolving code review issues, this function is rewritten to simplify control flow and to improve conformance with legacy usage (for string values "OFF", "DISABLED", and "WARNINGS").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! Thank you 👍
Thanks @alalek for working with this long development and review process. I will update the check-list to clarify what items are completed and what items remain to be implemented in the future. Also, I will start a new PR that will add log tags into existing OpenCV source code (possibly into contrib modules as well). This may be yet another long process. Hopefully I can schedule things so that I can work on multiple things in parallel. |
OE-11 Logging revamp (opencv#13909) * Initial commit for log tag support. Part of opencv#11003, incomplete. Should pass build. Moved LogLevel enum to logger.defines.hpp LogTag struct used to convey both name and log level threshold as one argument to the new logging macro. See logtag.hpp file, and CV_LOG_WITH_TAG macro. Global log level is now associated with a global log tag, when a logging statement doesn't specify any log tag. See getLogLevel and getGlobalLogTag functions. A macro CV_LOGTAG_FALLBACK is allowed to be re-defined by other modules or compilation units, internally, so that logging statements inside that unit that specify NULL as tag will fall back to the re-defined tag. Line-of-code information (file name, line number, function name), together with tag name, are passed into the new log message sink. See writeLogMessageEx function. Fixed old incorrect CV_LOG_VERBOSE usage in ocl4dnn_conv_spatial.cpp. * Implemented tag-based log filtering Added LogTagManager. This is an initial version, using standard C++ approach as much as possible, to allow easier code review. Will optimize later. A workaround for all static dynamic initialization issues is implemented. Refer to code comments. * Added LogTagConfigParser. Note: new code does not fully handle old log config parsing behavior. * Fix log tag config vs registering ordering issue. * Started testing LogTagConfigParser, incomplete. The intention of this commit is to illustrate the capabilities of the current design of LogTagConfigParser. The test contained in this commit is not complete. Also, design changes may require throwing away this commit and rewriting test code from scratch. Does not test whitespace segmentation (multiple tags on the config); will do in next commit. * Added CV_LOGTAG_EXPAND_NAME macro This macro allows to be re-defined locally in other compilation units to apply a prefix to whatever argument is passed as the "tag" argument into CV_LOG_WITH_TAG. The default definition in logger.hpp does not modify the argument. It is recommended to include the address-of operator (ampersand) when re-defined locally. * Added a few tests for LogTagManager, some fail. See test_logtagmanager.cpp Failed tests are: non-global ("something"), setting level by name-part (first part or any part) has no effect at all. * LogTagManagerTests substring non-confusion tests * Fix major bugs in LogTagManager The code change is intended to approximate the spec documented in https://gist.github.com/kinchungwong/ec25bc1eba99142e0be4509b0f67d0c6 Refer to test suite in test_logtagmanager.cpp Filter test result in "opencv_test_core" ... with gtest_filter "LogTagManager*" To see the test code that finds the bugs, refer to original commits (before rebase; might be gone) .. f345120 (2019-03-03T19:45:17Z) .... LogTagManagerTests substring non-confusion tests .. 1b848f5 (2019-03-03T01:55:18Z) .... Added a few tests for LogTagManager, some fail. * Added LogTagManagerNamePartNonConfusionTest. See test_logtagmanager.cpp in modules/core/test. * Added LogTagAuto for auto registration in ctor * Rewritten LogTagManager to resolve issues. * Resolves code review issues around 2019-04-10 LogTagConfigParser::parseLogLevel - as part of resolving code review issues, this function is rewritten to simplify control flow and to improve conformance with legacy usage (for string values "OFF", "DISABLED", and "WARNINGS").
This pullrequest changes
Logging revamp for OpenCV 4.1.x, OE-11, issue #11003
This PR replaces #13642 because the old PR was not salvageable.
Please keep all high-level or design-level discussions to the issue thread #11003 to keep the dialogue in one place.
Rebase and squash will happen, therefore keep in mind code review comments might be lost. Please put persistent comments (specific to the code changes in this PR and which are not high-level design issues) in this PR.
Status
Do not merge yet - the PR contains numerous behavior changes that still need to be ironed out for backward compatibility. Also missing "meaningful" performance tests.
Checklist
OPENCV_LOG_LEVEL
)Regarding log config for tags at startup-time
(cross reference https://gist.github.com/kinchungwong/37b3eabea7cd7bef3056ca9763816db7)
logcat
.imgproc:D imgcodecs.*:V *:S
"."
)"*"
) and are anchored to the "name parts" obtained from splitting tag names at the delimiter, the period ("."
)."imgcodecs.tiff.decoder"
, the name parts are"imgcodecs"
,"tiff"
, and"decoder"
. It can be matched by:"imgcodecs*"
, by ending with an asterisk, or"*tiff*"
or"*decoder*"
, by surrounding with a pair of asterisks."img*"
,"*code*"
,"*codecs"
,"*tiff.decoder*"
,"*coder"
, because they don't match anything aligned to the period delimiter.