Timestamped logging macro for elapsed physical time#546
Timestamped logging macro for elapsed physical time#546adi-65-ray wants to merge 14 commits intolf-lang:mainfrom
Conversation
hokeun
left a comment
There was a problem hiding this comment.
This is a great start. Thanks! I left some suggestions.
|
Just to provide the context @adi-65-ray (Aditya Ray) is an M.S. student at ASU, and he kindly volunteered to fix this issue. This PR will be his very first contribution. |
edwardalee
left a comment
There was a problem hiding this comment.
This is a nice feature to have, but there remain some subtleties. If we can resolve these, it might be nice to have this be part of the LF_PRINT_DEBUB macro rather than being a whole new macro.
There was a problem hiding this comment.
Do we really want to set LOG_LEVEL globally for all tests? What are the consequences of this? Can it be set within your one test that needs it?
There was a problem hiding this comment.
I tried redefining LOG_LEVEL locally in the test file but it gave waring for redeclaration and -Werror is set so it gets stuck.
There was a problem hiding this comment.
I looked into this and I came up with two solutions:
- I can create a separate test command which uses the global debug flag. so it would be unit-tests and unit-tests-debug with the LOG_LEVEL =4.
- I create a separate executable of reactor-c with LOG_LEVEL=4, link that to my test file and run the test within unit-tests.
I wanted to take the 2nd option but the CMakeLists.txt file of core has lot of conditional dependencies and I don't know should include all or is it fine if I link only a few of them.
There was a problem hiding this comment.
@adi-65-ray It will be great if you can use LOG_LEVEL=4 only for the unit tests involving debug-level logging.
There was a problem hiding this comment.
Why isn't sufficient to just set logging: DEBUG as a target parameter in the relevant tests?
There was a problem hiding this comment.
Hello Prof. Lee, @edwardalee
I think maybe you are confused.
This PR is adding a unit test, which is just run as make unit-tests right in reactor-c/.
There is no .lf file, so there are no target properties (e.g., logging: DEBUG). So adding this flag does not affect most tests, such as the long tests like lf-default.
The CI for this part, which is the unit-tests-single such as here, only takes 9 to 15 seconds to finish, so I don't think adding this will increase the CI test's time.
If we still really want to separate this Aditya suggested two ways.
- Create another unit-test in the
Makefile.
unit-tests: clean
# In case NUMBER_OF_WORKERS has been set, unset it.
cmake -B build -UNUMBER_OF_WORKERS
cmake --build build
cd build && make test
## CREATE THIS PART
unit-tests-debug: clean
# In case NUMBER_OF_WORKERS has been set, unset it.
cmake -B build -UNUMBER_OF_WORKERS -DLOG_LEVEL=4
cmake --build build
cd build && make test
However, I say this is not the right way to do, and complicate things more.
- Override the log level in the
test/general/logging_test.c
We can override the log level using#define LOG_LEVEL 4, but it shows a warning, and warnings are set as errors due to-Werrorflag.
Do you still suggest fixing this problem? I say this is too trivial, and fixing this will complicate the CMake.
I’m open to your guidance if you think this should still be addressed or if there is a cleaner solution I may have missed.
There was a problem hiding this comment.
We just talked with @Hokuen and he suggested us to create separate binaries on the test such as I mentioned as method 1.
@adi-65-ray will work on it.
There was a problem hiding this comment.
I see. But why does this need to be a unit test rather than a regular LF test? It seems the same trick of redirecting stdout could work in a startup reaction and the check could be performed in a shutdown reaction.
There was a problem hiding this comment.
I also think that makes sense. Would you like to create a LF test @adi-65-ray?
Why are these numbers negative? |
initially my test was wrong I wasn't using assertions and just printing it. I'll update the description so that others don't get confused. |
|
Hello, |
This PR is a fix for #395.
I have implemented a macro which prepends the physical lapsed time to debug messages
LF_TIMESTAMP_PRINT_DEBUGinlogging_macros.hI have also included a test code to test the functionality of the macro to check its expected output.
Example output:
DEBUG: [12345678]Hello World