Closed johnkerl closed 1 month ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 89.99%. Comparing base (
7cacee1
) to head (16e7cdb
). Report is 3 commits behind head on main.
Is this something we should add to CMakeLists.txt too where we default it to OFF? Maybe for debug builds it could be INFO.
It's is strictly run-time (once compiled in). spdlog
takes care of it. Using the (zero-other-dependency) example program I used before (included below). We should be able to tuck this away in the non-public part of the logger.
As it has a marginal cost (of checking a run-time condition) you could tuck it inside #if .. #endif for debug mode. But if would loose the benefit of 'always being there' without mods to code which is, if I understand it correctly, @johnkerl is after here (and which I also find appealing).
$ g++ -std=c++17 spdlog_ex_headeronly.cpp -o spdlog_ex_headeronly
$ ./spdlog_ex_headeronly # no setting
[14:26:53.559466] [my_demo] [I] [thread 2308290] Welcome to spdlog!
[14:26:53.559523] [my_demo] [E] [thread 2308290] Some error message with arg: 1
[14:26:53.559539] [my_demo] [I] [thread 2308290] Elapsed time: 6.8161e-05
[14:26:53.559548] [my_demo] [W] [thread 2308290] Easy padding in numbers like 00000012
[14:26:53.559584] [my_demo] [C] [thread 2308290] Support for int: 42; hex: 2a; oct: 52; bin: 101010
[14:26:53.559600] [my_demo] [I] [thread 2308290] Support for floats 1.23
[14:26:53.559612] [my_demo] [I] [thread 2308290] Positional args are supported too..
[14:26:53.559622] [my_demo] [I] [thread 2308290] left aligned
[14:26:53.559631] [my_demo] [I] [thread 2308290] Elapsed time: 0.000164105
$
$ SPDLOG_LEVEL=critical ./spdlog_ex_headeronly # critical only
[14:27:04.225658] [my_demo] [C] [thread 2308818] Support for int: 42; hex: 2a; oct: 52; bin: 101010
$
$ SPDLOG_LEVEL=error ./spdlog_ex_headeronly # error or higher
[14:27:15.571203] [my_demo] [E] [thread 2309374] Some error message with arg: 1
[14:27:15.571263] [my_demo] [C] [thread 2309374] Support for int: 42; hex: 2a; oct: 52; bin: 101010
$
It is activated by calling one line as demonstrated eg here and shown in the example I compiled and use here.
Is this something we should add to CMakeLists.txt too where we default it to OFF? Maybe for debug builds it could be INFO.
@nguyenv do you mind if I take this as a follow-on PR?
I looked into this and there is a potentially simpler two-line solution because it is indeed covered upstream. This is should do:
modified libtiledbsoma/src/utils/logger.cc
@@ -37,6 +37,7 @@
#include <spdlog/fmt/ostr.h>
#include <spdlog/sinks/basic_file_sink.h>
#include <spdlog/sinks/stdout_color_sinks.h>
+#include <spdlog/cfg/env.h>
namespace tiledbsoma {
@@ -72,6 +73,7 @@ Logger::Logger() {
#endif
}
set_level("INFO");
+ spdlog::cfg::load_env_levels(); // eg ./example SPDLOG_LEVEL=my_demo=warn
}
Logger::~Logger() {
Demo, from R for ease of use, but note that this tickles use in R (I also looked into changing the R wrapper package, this is working via the same two-line trick but widely shipped) and then libtiledbsoma in the last two lines:
$ SPDLOG_LEVEL="debug" Rscript -e 'library(tiledbsoma); sdf <- SOMADataFrameOpen("/tmp/tiledb/somadf_ts")'
[2024-09-09 17:35:28.930] [default] [Process: 3125929] [debug] [SOMADataFrameOpen] uri /tmp/tiledb/somadf_ts ts (now)
[2024-09-09 17:35:28.969] [default] [Process: 3125929] [debug] [TileDBObject] initialize SOMADataFrame with '/tmp/tiledb/somadf_ts' at (now)
[2024-09-09 17:35:28.969] [default] [Process: 3125929] [debug] [TileDBArray$open] Opening SOMADataFrame '/tmp/tiledb/somadf_ts' in READ mode
[2024-09-09 17:35:28.970] [default] [Process: 3125929] [debug] [tiledb_array] query is READ
[2024-09-09 17:35:28.975] [default] [Process: 3125929] [debug] [TileDBArray$update_metadata_cache] updating metadata cache for SOMADataFrame '/tmp/tiledb/somadf_ts' in READ
[2024-09-09 17:35:28.977] [tiledbsoma] [Process: 3125929] [Thread: 3125929] [debug] [SOMAArray] static method 'ctx' opening array '/tmp/tiledb/somadf_ts'
[2024-09-09 17:35:28.977] [tiledbsoma] [Process: 3125929] [Thread: 3125929] [debug] [SOMAArray] opening array '/tmp/tiledb/somadf_ts'
$
Without the env var nothing is shown or seen:
$ Rscript -e 'library(tiledbsoma); sdf <- SOMADataFrameOpen("/tmp/tiledb/somadf_ts")'
$
@eddelbuettel kerl/libtiledbsoma-env-logging-level -- thank you! :)
I find this very useful, particularly on #2407 / [sc-51048]. If useful to other folks, great; if not, hopefully not too disruptive.