Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: Refactor logging code into a global object #12954

Merged
merged 6 commits into from May 1, 2018

Conversation

@jimpo
Copy link
Contributor

commented Apr 11, 2018

This is purely a refactor with no behavior changes.

This creates a new class BCLog::Logger to encapsulate all global logging configuration and state.

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

Concept ACK, splitting the logging functionality out of util makes sense, util.cpp is large, haphazard and this is a clearly distinguishable concern from the rest.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 13, 2018

Concept ACK. Needs rebase

Copy link
Member

left a comment

Concept ACK

src/util.h Outdated
} \
} while(0)
#endif

template<typename... Args>
bool error(const char* fmt, const Args&... args)

This comment has been minimized.

Copy link
@sipa

sipa Apr 16, 2018

Member

I think this should move to logging as well, so the cyclic dependency between logging and util can be avoided.

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 16, 2018

Author Contributor

LogPrintf is also used in TraceThread, and I don't think either of them actually belong in logging (the whole point of this PR is to create a more cohesive separation). I can very easily imagine another method that gets added to util.h in the future requiring logging as well.

The dependency isn't really cyclic -- logging.cpp includes util.h, and util.h includes logging.h, but logging.h does not include util.h.

This comment has been minimized.

Copy link
@sipa

sipa Apr 16, 2018

Member

My view is that you should always treat the .h and the .cpp file as one unit. While this may indeed not be a cyclic dependency for the compiler, it is certainly one between the two modules semantically: logging can't work without util, and util can't work without logging. Finding out why that is the case helps creating a cleaner separation.

You're right that util is going to depend on logging though, as it contains a number of higher level functions. However, It looks like logging only really needs util for finding the debug log path, though. All the rest is in utiltime. This seems easy to fix, by instead having init query the path and call a setter on logging for it for example. If other reviewers are fine with that, we can fix that up in another PR.

This comment has been minimized.

Copy link
@Empact

Empact Apr 20, 2018

Member

Last I looked, when reviewing #13021, I saw another approach to solving the cyclic dependency is to include utiltime.h in logging.cpp and splitting out args and path handling from util.h. Didn’t go all the way to implementing that but would be happy to as a follow-on to #13021.

This comment has been minimized.

Copy link
@Empact

Empact Apr 20, 2018

Member

Lol, didn’t finish reading your comment @sipa. Anyway I’m for it.

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 20, 2018

Author Contributor

Fixed with d903273cf8e51d2789ba8a88c608a66e63dcdc14.

@jimpo jimpo force-pushed the jimpo:logging branch Apr 16, 2018
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

Needs rebase again. (Since this has some Concept ACKs, but is somewhat largish, it might be easier to merge in two steps. First the move-only commit in a separate pull request and then the refactoring in this pull request)

sipa added a commit that referenced this pull request Apr 20, 2018
…les.

b77b6e2 MOVEONLY: Move logging code from util.{h,cpp} to new files. (Jim Posen)

Pull request description:

  Split out first commit from #12954 to reduce amount of rebasing necessary.

  This introduces a cyclic dependency between `logging` and `util` that should be cleaned up in a future PR.

Tree-SHA512: 695e512f9c2f7b4ca65e367fc924358e3cb2dc531bcbb7a6f62710b2a87280b35aba7793aa272e457fcd65448abe3feb1deb3b8064ed208917ca356b0f410813
@jimpo jimpo force-pushed the jimpo:logging branch Apr 20, 2018
Copy link
Member

left a comment

Tested ACK d903273cf8e51d2789ba8a88c608a66e63dcdc14. Couple of comments inline but nothing to prevent merge.

* the timestamp when multiple calls are made that don't end in a
* newline.
*/
std::atomic_bool m_started_new_line{true};

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 20, 2018

Member

Note that this maintains the old (broken) behaviour:

  • two threads are running. Thread A logs a line that doesn't terminate with a newline (ie a 'continuing' log)
  • thread B then logs a line. This is treated as a 'continuation' log of thread A's previous log, and is printed on the same line, without a timestamp
  • thread A's 'continuation' log is printed on a new line, with a timestamp

No need to fix that in this PR.

extern bool fPrintToDebugLog;

extern bool fLogTimestamps;
extern bool fLogTimeMicros;
extern bool fLogIPs;

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 20, 2018

Member

In commit util: Establish global logger object. : any reason to exclude fLogIPs from g_logger?

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 23, 2018

Author Contributor

It's not actually used by the logger, it's just used in the net code. Arguably it should live there instead.

@@ -264,7 +259,7 @@ void BCLog::Logger::ShrinkDebugFile()
int nBytes = fread(vch.data(), 1, vch.size(), file);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 21, 2018

Member

Unrelated nit: Could change int to size_t here, so that it doesn't break when someone changes RECENT_DEBUG_HISTORY_SIZE to 5GB.

Copy link
Member

left a comment

Tested ACK d903273

  • ./src/qt/bitcoin-qt -regtest -conf="$(pwd)/bitcoin.conf" -debug=1
    • Displays all available logs.
  • ./src/qt/bitcoin-qt -testnet -conf="$(pwd)/bitcoin.conf" -debug=0
    • Displays only uncategorized logs.
  • ./src/qt/bitcoin-qt -testnet -conf="$(pwd)/bitcoin.conf" -debug=1
    • Displays all available logs.
  • ./src/qt/bitcoin-qt -testnet -conf="$(pwd)/bitcoin.conf" -help
    • Displays available logging categories for -debug.
  • ./src/bitcoind -debug=foobar
    • Warns of invalid logging category.
  • ./src/bitcoind -debug=1 -debugexclude=net,addrman,rand,leveldb
    • Displays all available logs (confusing, but this matches behavior on master).
  • ./src/bitcoind -debugexclude=net,addrman,rand,leveldb
    • Excludes specified log categories.
  • ./src/bitcoind -testnet -config="$(pwd)/bitcoin.conf" -debug=reindex -reindex
    • Shows reindex-related log messages.
  • ./src/bitcoind -testnet -config="$(pwd)/bitcoin.conf" -debug=net -reindex
    • Shows no reindex-related log messages, but plenty of net-related ones.

Nice change. The one nit I had definitely shouldn't hold this up.

src/logging.cpp Outdated
flag = BCLog::ALL;
return true;
}
for (unsigned int i = 0; i < ARRAYLEN(LogCategories); i++) {

This comment has been minimized.

Copy link
@jamesob

jamesob Apr 25, 2018

Member

nit: seems like you could phrase this more simply as

for (CLogCategoryDesc& cat : LogCategories) { ... }

There doesn't seem to be a need to have the index on hand.

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 30, 2018

Author Contributor

Nice, fixed. Also gets rid of the dependency of logging on utilstrencodings.

@jimpo jimpo force-pushed the jimpo:logging branch Apr 27, 2018
jimpo added 3 commits Apr 11, 2018
The object encapsulates logging configuration, and in a later commit,
set up routines will also be moved into the class.
@jimpo jimpo force-pushed the jimpo:logging branch Apr 27, 2018
jimpo added 3 commits Apr 11, 2018
Changing parameter types from pointers to references and uint32_t to
BCLog::LogFlags simplies calling code.
-BEGIN VERIFY SCRIPT-
sed -i "s/fileout/m_fileout/" src/logging.h src/logging.cpp
sed -i "s/mutexDebugLog/m_file_mutex/" src/logging.h src/logging.cpp
sed -i "s/vMsgsBeforeOpenLog/m_msgs_before_open/" src/logging.h src/logging.cpp
sed -i "s/logCategories/m_categories/" src/logging.h src/logging.cpp
sed -i "s/fPrintToConsole/m_print_to_console/" src/logging.h src/logging.cpp src/init.cpp
sed -i "s/fPrintToDebugLog/m_print_to_file/" src/logging.h src/logging.cpp src/init.cpp src/test/test_bitcoin.cpp src/bench/bench_bitcoin.cpp
sed -i "s/fLogTimestamps/m_log_timestamps/" src/logging.h src/logging.cpp src/init.cpp
sed -i "s/fLogTimeMicros/m_log_time_micros/" src/logging.h src/logging.cpp src/init.cpp
sed -i "s/fReopenDebugLog/m_reopen_file/" src/logging.h src/logging.cpp src/init.cpp
sed -i "s/fStartedNewLine/m_started_new_line/" src/logging.h src/logging.cpp
-END VERIFY SCRIPT-
This breaks the cyclic between logging and util.
@jimpo jimpo force-pushed the jimpo:logging branch to 8c2d695 Apr 29, 2018
@jnewbery

This comment has been minimized.

Copy link
Member

commented Apr 30, 2018

utACK 8c2d695. Only difference is changing to range based for loops per #12954 (comment)

@sipa

This comment has been minimized.

Copy link
Member

commented May 1, 2018

utACK 8c2d695. Nice to see this encapsulated.

@sipa sipa merged commit 8c2d695 into bitcoin:master May 1, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sipa added a commit that referenced this pull request May 1, 2018
8c2d695 util: Store debug log file path in BCLog::Logger member. (Jim Posen)
8e7b961 scripted-diff: Rename BCLog::Logger member variables. (Jim Posen)
1eac317 util: Refactor GetLogCategory. (Jim Posen)
3316a9e util: Encapsulate logCategories within BCLog::Logger. (Jim Posen)
6a6d764 util: Move debug file management functions into Logger. (Jim Posen)
f55f4fc util: Establish global logger object. (Jim Posen)

Pull request description:

  This is purely a refactor with no behavior changes.

  This creates a new class `BCLog::Logger` to encapsulate all global logging configuration and state.

Tree-SHA512: b34811f54a53b7375d7b6f84925453c6f2419d21179379ee28b3843d0f4ff8e22020de84a5e783453ea927e9074e32de8ecd05a6fa50d7bb05502001aaed8e53
@jimpo jimpo deleted the jimpo:logging branch May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.