Added logging functionality (spdlog) #190
Conversation
quantum-shift
force-pushed
the
spdlog_logger
branch
2 times, most recently
from
13065dc
to
f036e9d
Compare
There was a problem hiding this comment.
Hi @quantum-shift,
Thanks a lot for the PR! Overall, everything looks fairly nice. I noticed that we still have a few uses of std::cout e.g. this line etc. I think it would be good to unify these as well. I have some more detailed comments below. Please let me know, many thanks.
Best,
Serhan
CMakeLists.txt
Outdated
| #--- create uninstall target --------------------------------------------------- | ||
| include(cmake/prmonUninstall.cmake) | ||
|
|
||
| #--- code format targets ------------------------------------------------------- | ||
| include(cmake/clang-format.cmake) | ||
| include(cmake/python-format.cmake) | ||
|
|
There was a problem hiding this comment.
We probably don't need this empty line.
package/CMakeLists.txt
Outdated
| @@ -1,5 +1,6 @@ | |||
| find_package(nlohmann_json REQUIRED) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
We probably don't need this empty line.
package/src/MessageBase.h
Outdated
| void error(const std::string& message); | ||
| }; | ||
|
|
||
| #endif // PRMON_MESSAGEBASE_H |
There was a problem hiding this comment.
Missing newline at the end of the line.
package/src/countmon.cpp
Outdated
| @@ -10,6 +10,7 @@ | |||
| #include <iostream> | |||
| #include <sstream> | |||
|
|
|||
| #include "MessageBase.h" | |||
There was a problem hiding this comment.
We don't really need to include this header in the source files since it's already in the header, no?
package/src/countmon.h
Outdated
| @@ -6,17 +6,19 @@ | |||
|
|
|||
| #ifndef PRMON_COUNTMON_H | |||
| #define PRMON_COUNTMON_H 1 | |||
| #define MY_NAME "countmon" | |||
There was a problem hiding this comment.
I think we can define this in the source file instead (applies to other cases too) and un-define it after using. Also, we can perhaps call it something more explanatory, e.g. MONITOR_NAME.
There was a problem hiding this comment.
If it's going to be private maybe should it be something like THIS_MONITOR_NAME_ (or were you thinking of calling it, e.g., COUNTMON_NAME_ (different in each monitor).
I agree that undefining it at the end is smart - stops it from leaking out.
package/src/utils.cpp
Outdated
| @@ -103,4 +103,4 @@ const void prmon::fill_units(nlohmann::json& unit_json, | |||
| unit_json["Units"]["Avg"][param.get_name()] = param.get_avg_unit(); | |||
| } | |||
| return; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Missing newline at the end of the line.
|
Thank you, @amete. It was decided to not change the help output of prmon (that is, not integrate it with the logger). Hence, I have kept the std::cout statements there. Please let me know if there was a change in plan. I have addressed your other comments in a commit. |
Yes, this also my recollection (it's quite standard behaviour for |
That indeed makes sense 👍 |
There was a problem hiding this comment.
Just one utterly trivial point from me! Thanks @quantum-shift
CMakeLists.txt
Outdated
| set(BUILD_BENCHMARK_LOG OFF) | ||
| endif() | ||
| set(BUILD_BENCHMARK_LOG "${BUILD_BENCHMARK_LOG}" | ||
| CACHE BOOL "Build binary which benchmarks spdlog speed" FORCE) |
There was a problem hiding this comment.
Very trivial point, but can you change which to that (it's a defining clause)
quantum-shift
force-pushed
the
spdlog_logger
branch
from
0a6833a
to
e4f8bd8
Compare
spdlog was added as a submodule All previous logging statements replaced with new logger .github/workflows/ci.yml was updated to initialise submodules recursively
quantum-shift
force-pushed
the
spdlog_logger
branch
from
e4f8bd8
to
1da1807
Compare
As discussed in issue #189, spdlog is integrated into prmon. spdlog (version 1.8.5) is added as a submodule. All previous logging statements are replaced with the new logger.
Further, command line switches will be added to handle the logging level of monitors.