search

mit-dci / opencbdc-tx

A transaction processor for a hypothetical, general-purpose, central bank digital currency
Other
896 stars 198 forks source link

Raft node logging #180

Closed mszulcz-mitre closed 1 year ago

mszulcz-mitre commented 2 years ago

Affected Branch

trunk

Basic Diagnostics

Description

Problem

Pull request #174 replaced the usage of std::cout with a logger in class raft::node. Here's the changes: image

In my tests, the logger calls don't write logs. When I step through the calls in a debugger, they resolve to the info method in the class nuraft::logger, which is deprecated and has an empty body. Here's the relevant parts of the class from the NuRaft repository (https://github.com/eBay/NuRaft/blob/0a084b65531a7004a48df445a0bf532b626c01db/include/libnuraft/logger.hxx#L24-L44): image

Potential Solution

Instead of calling the info method, call the put_details method. This method wraps calls to logging methods in cbdc::logging::log such as info: https://github.com/mit-dci/opencbdc-tx/blob/95c25e28ecb2dd05e209506337921e52af37cae3/src/util/raft/console_logger.cpp#L12-L58
This method appears to not be used yet in the source code, but it is used in a unit test: https://github.com/mit-dci/opencbdc-tx/blob/95c25e28ecb2dd05e209506337921e52af37cae3/tests/unit/raft_test.cpp#L674-L718

Another consideration

Many parts of the code call the error method in class logging::log to log error messages (grep returns more than 50 instances): https://github.com/mit-dci/opencbdc-tx/blob/95c25e28ecb2dd05e209506337921e52af37cae3/src/util/common/logging.hpp#L106-L111 raft::node, however, uses std::cerr: https://github.com/mit-dci/opencbdc-tx/blob/95c25e28ecb2dd05e209506337921e52af37cae3/src/util/raft/node.cpp#L53-L56 It's not clear to me why std::cerr is used in some parts of the code (about 30 instances) and the logger is used in other parts, but in this case at least, the put_details method could be used to replace the call to std::cerr.

Code of Conduct

metalicjames commented 2 years ago

@mszulcz-mitre I pushed #181 which fixes the immediate bug.

In terms of the other uses of std::cerr, it's currently used anywhere a log instance isn't available, or when outputting directly to the console without log formatting is required (in a console application, for example).