-
Notifications
You must be signed in to change notification settings - Fork 18
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
Overhaul logging mechanism #80
Conversation
7b4d7e8
to
5864d74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, human readable logs are a very welcome change! A few things:
b196c3d
to
6ecaab0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A few more things, mostly about includes. I would prefer if we kept iostreams out of as many translation units as possible because it tends to be quite expensive to compile.
I've addressed the additional feedback by @fknorr and in that process also made some improvements to the As mentioned in the comment above, I've mainly gotten rid of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! After some digging, the tie-in between fmt and iostreams in our code, aka <fmt/ostr.h>
has two origins:
print_utils.h
and.cpp
exposeostream& operator<<
for thesubrange
andchunk
types, which are internally implemented usingfmt
, so we are going back and forth between fmt and stringstreams.- The Allscale headers define
ostream& operator<<
on theGridBox
type. Its only use is inregion_map_testspy::print_regions
, and that is currently dead code as the only (transitive) invocation is commented out ingraph_compaction_tests:142
.
However, the major offender now is the Allscale assertions header which pulls in <iostreams>
unconditionally. I feel like this is beyond the scope of this PR now, so I'll cease my iostreams crusade.
This changes the logging mechanism used throughout the runtime from having to pass around the `detail::logger` class to simple free-standing macros such as `CELERITY_INFO(...)`. While the new mechanism no longer allows for arbitrarily nested "contexts", a single context can be set through `CELERITY_SET_LOG_CTX` (currently only used in worker jobs). Full list of changes: - Get rid of JSON output for trace log level - Remove logger class, use macros instead - Only a single level of context supported - Adjust log level for many messages, distinguish between debug/trace - Add additional information in various logging messages - Default log level to info (release builds) and debug (debug builds)
...to control how large task/command graphs can be before their GraphViz output is omitted.
Replaces `log_map` (which was an `std::unordered_map`) with `std::tuple` to have less runtime overhead and allow for arbitrary types to be logged. Changes to `user_bench` utilities: - Logging through `log_map` is no longer supported. - Replace `log_user_config` by `log_once` function to log messages only on master node. - Print delta time since previous event. - Ensure thread safety for all functions.
fd3ddd4
to
4fae2d4
Compare
This changes the logging mechanism used throughout the runtime from having to pass around the
detail::logger
class to simple free-standing macros such asCELERITY_INFO(...)
. While the new mechanism no longer allows for arbitrarily nested "contexts", a single context can be set throughCELERITY_SET_LOG_CTX
(currently only used in worker jobs).Full list of changes: