Re: [boost] [Review] The review of the Boost.Logging library starts today, November 7th

From: Caleb Epstein (caleb.epstein_at_[hidden])
Date: 2005-11-07 12:32:45

On 11/6/05, Hartmut Kaiser <hartmut.kaiser_at_[hidden]> wrote:

- What is your evaluation of the design?

I think the design is overall quite good. I like the named logger concept
and the run-time configurability of the system is a great strength.

- What is your evaluation of the implementation?

I have some questions and suggestions:

The namespace has been changed from the original poposed versions from "log"
to "logging" but the subdirectory names remain "log". This should be
reconciled.

boost/log/log.hpp:

   - Should BOOST_LOG_DEFINE_LEVEL use BOOST_STATIC_CONSTANT?
   - The "default_" level should be renamed "default_level".
   - enabled_logger::enabled_logger (logger&): initialize m_stream in the
   member initialization or the ctor body but not both
   - Remove old versions of macro definitions that have been rewritten
   (BOOST_IS_LOG_ENABLED, BOOST_SCOPED_LOGL).

boost/log/log_impl.hpp: DEFAULT_INDEX should probably be lower-cased and
possibly renamed. It should probably use BOOST_STATIC_CONSTANT.

boost/log/log_manager.hpp: DEFAULT_CACHE_LIMIT: See concerns above.

boost/log/functions.hpp: Macro guard looks like the victim of a Find/Replace
gone awry: JT_BOOST_appender_funcTIONS_HPP.

I'd recommend getting rid of the prepend_time functor with its $xx format
and use the strftime-based one instead. It might be also be nice to provide
another functor that uses Boost.DateTime for higher-resolution timestamps.

append_enter should be renamed append_newline. Enter is not a chracter, it
is a key on a keyboard. Likewise "enter" should be changed to "newline"
throughout the docs.

boost/log/extra directory. Does this name really make sense? Shouldn't this
just be "appenders"? I won't comment on the quality of the code in here,
since its mostly mine :-) There's actually a bug in my
rolling_file_appender. The "for" loop in rolling_file_appender::rollover
should read:

for (int suffix = num_files - 2; suffix >= 0; --suffix) {

Otherwise you end up with num_files + 1 files, not num_files.

Also, in general, do the loggers really need the m_destroyed member? Why
isn't the shared_ptr in m_impl enough to ensure the object lifetimes?

I will need to do some tests to back this up, but I have an abiding concern
that the overhead of all the string copying and resizing that results from
the prepend operations will make logging more computationally expensive than
it need be in the (IMHO) common case of prepending a timestamp and possibly
thread ID to each message. It might make more sense for the modifiers to
operate directly on output streams. Modifiers would receive a std::ostream&
in addition to two strings and all output would be done on the stream. Just
a thought, and I need to back this up with concrete profile results before I
can claim anything other than a gut feeling.

Jamfiles contain references to d:/boost/boost_latest with a comment "FIXME
remove this!". Remove this :)

There are no tests, though the examples exercise most of the functionality.
Provide a Jamfile to build the examples.

- What is your evaluation of the documentation?

The documentation covers the important features of the library.
Appearance-wise, I think the HTML pages look anachronistic. I'd suggest
rewriting the documenatation using one of the meta-formats like QuickBook,
ReStructured Text or the like to allow for a more Boost-like look and feel
to the pages, as well as the possibility of rendering in formats other than
HTML.

In general I see a lot of (parenthetical phrases) that would be better set
off by commas, not parentheses. Specific comments:

motivation.html:
"it's up to you how much information you log, and where.." Remove extra
period.

"The *Boost Log Library *is a small library, who:". Should be "which" or
"that" (English majors?), but not "who".

"allows for easy and *efficient *setting the level of logs/ log
hierarchies<file:///U:/src/logging/boost_root/libs/log/doc/manipulating_logs.html#level>".
Wording is awkward and at this point, the level concept has not been
introduced. How about (new point) "allows messages to be logged with an
associated level (e.g. debug, info, warning, error)", and then "allows logs
to be easily configured to include or exclude messages of a given level."
Personally I prefer the more verbose term "severity level" for this concept,
but level is OK.

logs.html:
The first section should probably be named "Log Concepts".

Very awkward paragraph:

"*the appender(s) of the log*: where will messages that are written to this
log be outputted to. For instance, you can have a log that outputs messages
to the console and to a certain file at the same time."

Instead, how about:

"* Zero (one?) or more appenders. Appenders are functors that receive the
messages you log and write them someplace. The Boost.Log library provides
appenders for writing to a file, standard output, a debug window
(Windows-specific), among others.".

The word appenders should link to modifier_logger.html.

Again:

"*the modifier(s) of the message*: functions that modify the original
message. Such functions can prefix the original message by the current time,
and/or by the current thread's ID, append an enter to the message, etc."

How about:

"* Zero or more modifiers. Modifiers are functors that can be used to
(suprise!) modify the messages that a user logs. Standard modifiers are
provided for prepending a timestamp, thread ID, or append a newline to each
message."

The word modifiers should link to modifier_logger.html.

And:

"the log's *level*. When you write a message to the log, you specify its
level. If the message's level is higher or equal to the the log's level, the
message get written (to this log's destinations). Otherwise, the message is
ignored (in fact it won't even be processed)"

To:

"The log's level. Users of the Boost.Log library specify a level with each
logging call, or a default value is used. Only messages with a greater than
or equal to the log's configured level will actually be logged [*]

As a note or perhaps part of the paragraph above:

"[*] Messages that are ignored are not even formatted, saving processing
time. Because this check involves a simple numeric comparison, it is
possible (and even recommended?) to leave debugging log statements in
production code. They should not measurably affect your application's
performance when they are disabled."

Perhaps this would be a good place to discuss or link to a discussion of the
compile-time enabled/disabled loggers.

Declaring/Defining Logs:

Change:

In order to be used, logs need to first be declared (and defined somewhere).
You'll declare logs in a header file:

To:

Logs must be both declared and defined before they may be used. In general,
users will declare logs in a header file:

modifier_logger.html:

"append an enter to the message, if not already found". Should be "newline",
not "enter".

"prepend the log' name (example, if the log is named "app.charts.dbg",
prepend this to the message)". Change to "prepend the log's name, for
example "app.charts"". I think the use of "dbg" here is a remnant of the
pre-levels design.

manipulating logs.html:

There are a number of cases in this section where the terminology gets
muddled. Modifiers are called both modifiers and manipulators. Stick to a
single name.

"Note that if modifier functions A and B have the *same *index, they way
they get called is unspecified (you don't know for sure if A is called
before B, or vice-versa)". Change "way they get called" to "order in which
they are called" and drop the parenthesized phrase.

"The *name *can associate a modifier func with a certain name. This is only
useful if you might want to later delete this modifier func". Should read
"The methods that take a name argument can be used to associate a name with
a modifier. This name can be used later to remove the modifier". Perhaps
have "remove the modifier" as a link to the documentation for del_modifier.

I like Daryl's proposed change to the way appenders are associated with
logs. This might make sense for modifiers as well, and could make it quite
easy to write a totally configuration-driven method to initialize the
Boost.Log subsystem. However, doesn't this push thread-safety concerns into
the appenders? For example, if several logs share a given appender, is it
now up to the appender to serialize access to operator() instead of the log.

- What is your evaluation of the potential usefulness of the library?

Extremely useful. I plan to replace usage of the log4cplus library (a log4j
port) and another home-grown logging library with Boost.Log in production
code.

- Did you try to use the library? With what compiler? Did you have any
> problems?

Yes, with GNU g++ 3.3.4 on Linux. Some minor issues getting the Jamfile to
work (had to add path to Boost.Log to <includes> and some compile warnings,
but the code compiled and works. Here are the warnings from gcc:

"g++" -c -Wall -ftemplate-depth-255 -g -O0 -fno-inline -I"../../../bin
/boost_root/libs/log/build" -I"/home/nbde52d/src/boost" -I"../../.." -I
"/home
/nbde52d/src/boost" -o
"../../../bin/boost_root/libs/log/build/libboost_log.a/g
cc/debug/runtime-link-static/functions.o" "../src/functions.cpp"
"/usr/bin/objcopy" --set-section-flags .debug_str=contents,debug "../../../b
in/boost_root/libs/log/build/libboost_log.a/gcc/debug/runtime-link-static/functi
ons.o"

../src/functions.cpp: In constructor `
boost::logging::prepend_time::prepend_time(const
boost::logging::logging_types::string&)':
../src/functions.cpp:133: warning: comparison between signed and unsigned
integer expressions
../src/functions.cpp:135: warning: comparison between signed and unsigned
integer expressions
../src/functions.cpp:138: warning: comparison between signed and unsigned
integer expressions
../src/functions.cpp:138: warning: comparison between signed and unsigned
integer expressions
../src/functions.cpp:139: warning: comparison between signed and unsigned
integer expressions
../src/functions.cpp:144: warning: comparison between signed and unsigned
integer expressions
../src/functions.cpp:146: warning: comparison between signed and unsigned
integer expressions
../src/functions.cpp:148: warning: comparison between signed and unsigned
integer expressions

(all of the _idx variables should be of type
logging_types::string::size_type; this is even typedef'd above)

"g++" -c -Wall -ftemplate-depth-255 -g -O0 -fno-inline -I"../../../bin
/boost_root/libs/log/build" -I"/home/nbde52d/src/boost" -I"../../.." -I
"/home
/nbde52d/src/boost" -o
"../../../bin/boost_root/libs/log/build/libboost_log.a/g
cc/debug/runtime-link-static/log.o" "../src/log.cpp"
"/usr/bin/objcopy" --set-section-flags .debug_str=contents,debug "../../../b
in/boost_root/libs/log/build/libboost_log.a/gcc/debug/runtime-link-static/log.o"

../../../boost/log/log_impl.hpp: In constructor `
boost::logging::logger_impl::logger_impl(const
boost::logging::logging_types::log_name_string_type&)':
../../../boost/log/log_impl.hpp:127: warning: `
boost::logging::logger_impl::m_is_compile_time' will be initialized after
../../../boost/log/log_impl.hpp:124: warning: `level_type
boost::logging::logger_impl::m_level'
../src/log.cpp:73: warning: when initialized here
gcc-C++-action
../../../bin/boost_root/libs/log/build/libboost_log.a/gcc/debug/r
untime-link-static/log_manager.o

(chage order of member initializers in ctor)

The "multiple_threads" example would not compile properly without changing
this line in use_logs.cpp:
thread( bind(writer_thread,idx) );
to
thread noname ( bind(writer_thread,idx) );

- How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

I've spent a good deal of time looking at this library in both its present
form and at earlier versions.

- Are you knowledgeable about the problem domain?

Yes. I have used a number of logging APIs in a number of different
languages, and implemented a couple myself.

I vote that this library be accepted into Boost pending some work on the
documentation and after addressing some very minor implementation issues.

--
Caleb Epstein
caleb dot epstein at gmail dot com