OE-11 Logging revamp (#11003) for 4.2.x (draft (logging_20190220))#13909
Merged
alalek merged 13 commits intoopencv:masterfrom Apr 21, 2019
kinchungwong:logging_20190220
Merged
OE-11 Logging revamp (#11003) for 4.2.x (draft (logging_20190220))#13909alalek merged 13 commits intoopencv:masterfrom kinchungwong:logging_20190220
alalek merged 13 commits intoopencv:masterfrom
kinchungwong:logging_20190220
Conversation
Contributor
Author
|
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. |
Contributor
Author
|
Some non-critical commits have been taken off this branch (logging_20190220) and moved to dead-end branches:
|
Contributor
Author
|
See spec revised 2019-03-20 https://github.com/kinchungwong/opencv_OE11_logging_11003_wiki/blob/master/spec_revised_20190320.md |
alalek
reviewed
Apr 10, 2019
Member
alalek
left a comment
There was a problem hiding this comment.
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); \ |
Member
There was a problem hiding this comment.
I believe we can try to utilize C++ "magic" for that.
I will take a look on this later (perhaps in a separate PR).
Contributor
Author
There was a problem hiding this comment.
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_FALLBACKmacro- 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_GLOBALmacro- alternative: we can simply combine this functionality with
CV_LOGTAG_FALLBACK, in other words, the default value forCV_LOGTAG_FALLBACKwould 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__) |
Member
There was a problem hiding this comment.
LOG_LEVEL_VERBOSE + (int)(v)
Need to align this expression with:
CV_LOG_STRIP_LEVELdefinition 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).
Contributor
Author
There was a problem hiding this comment.
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
writeLogMessageExfunction, without processing it.- Logging macro is not responsible for filtering based on value of "v"
- If
writeLogMessageExwants to filter value of "v", user need to provide a user-defined backend. - Normally,
writeLogMessageExjust 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.
writeLogMessageExwill 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.
Contributor
Author
|
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").
Contributor
Author
|
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. |
a-sajjad72
pushed a commit
to a-sajjad72/opencv
that referenced
this pull request
Mar 30, 2023
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.