Hartmut Kaiser <hartmut.kaiser <at> gmail.com> writes:
--------------------------------------------------- Please always state in your review, whether you think the library hould be accepted as a Boost library!
I don't think the library is ready for acceptance yet, but I do think that it embodies many of the elements required to make a widely useful logging library, including some apparently unique/novel ones, and that such a library would make a great addition to Boost. While I think the library could do with some further refinement, the scope of the library needs to be limited sufficiently that it is possible to produce a high-quality implementation. Once the library reaches that quality while offering sufficient flexibility and extensibility it should be accepted. Later submissions of add-ons/enhancements can be included or mini-reviewed as appropriate (new appenders, enhanced/specialised filtering etc). So - thats a "not yet" going on abstain (given that my name is on many of the source files I'm not sure that the review manager should actually count my vote either way).
Additionally please consider giving feedback on the following general topics:
- What is your evaluation of the design?
Under the features there is a pretty good design waiting to get out, however it needs to be revisited in a number of areas: 1) A given message is directed to a particular named log, from where it is sent to 1 or more named appenders. It is easy to connect/disconnect appenders to logs. It is also easy to set the log level. However, I find the log level facility useless, because in general I want to filter the messages to a particular appender, not globally - if one of my appenders is the system error log, I always want errors, all errors, and nothing else, going to it. If I have some sort of console debug output, I might want to turn on debug level output to it alone (in general from only a few logs). I can do this reasonably well if I connect the "system error log" to "*.error" and adjust what goes to the "debug console" by specifying eg. "foo.bar.debug" output to go there, or more likely "foo.bar.*" and turning off "foo.bar.debug" if it is too noisy. If some library author decides that log levels are the way to go, I can't get my app to direct that libraries errors to the error log. Alternatively, if I write a library, and I want to ensure that it is useable in this scenario and in some level-based logging scheme, I would presumably need to log an error with a level of err to a log called "mylib.error" and require that the app using mylib connect appenders to "mylib.*" and set levels on "mylib.*" if the app is using "level based" filtering. While I find level-based filtering limited, I do think the ability to apply some sort of filter other than just a bool enable/disable is useful. The type of filter used should be customisable - a very simple fast filter (bool or just a non-empty appender set) would suit many uses, while some might require a very elaborate check against some sort of filter settings data structure. Either this can be encapsulated as a policy, or (and I think this would be less intrusive and as effective) the enabled_logger etc interface could be tweaked and documented to support something like: #define BOOST_LOGC(log_name, cond) if (logger_keeper keep = logger_and_bool (log_name, (cond) )); else (*keep.stream()) set<pthread_t> threads_to_log; bool thread_enabled() { // needs a mutex return threads_to_log.count(pthread_self()); } BOOST_LOGC(some_log, thread_enabled()) << "Thread " << pthread_self() << " is enabled."; if thread-based filtering is desired by default just replace BOOST_LOG with: #define BOOST_LOG(log_name) if (logger_keeper keep = logger_and_bool(log_name, thread_enabled() )); else (*keep.stream()) BOOST_LOG(some_log) << "Thread " << pthread_self() << " is enabled."; Extending logger_and_bool to logger_and_predicate and the appropriate c'tors for the keeper/enabled_log would allow the predicate to perform checks that require access to the logger itself as well. These tests are (potentially) heavyweight but so long as a simple call to BOOST_LOG simply bypasses them there is no cost to the user who doesn't need them. For example, Gennady would presumably like some form of fully extensible set of attributes to apply filtering rules to based on log name. He could simply maintain whatever map<name_string, attr_type> he needs and write appropriate predicates to filter on this. Of course that could be too slow, in which case a more efficient mapping from log to attributes would be needed - by allowing customisation/extension of the logger_impl - see below. 2) While the library aims to offer a customisation point through replacing the manager type, the manager concept is un(der)documented, and the various interfaces that depend on the manager type are too closely coupled to allow much customisation anyway. Note that this may be ok, if the design of the standard manager is flexible enough without extensive customisation, and any remaining customisation points/concepts are clearly documented. In any case, the use of the customisation needs to be illustrated by example (ie. an alternate manager replacing all the default interfaces). 3) Closely related to the above, I would like to see cleaner separation between the components that make up this library. This is something that is hard to do when the implementation is a feature driven moving target (I've been trying to do it on and off). This separation needs to maintain or increase the existing loose coupling between compilation units that use the library to log, those that set/modify filtering and those that do actual output of logged messages, while reflecting that separation in the implementation. Currrently appenders do this, but the logging and filtering is too internally intertwined. If the library formalised that separation allowing each module to be customised, library code could use logging independent of how the application (or other libraries) used it/controlled it (and without forcing library recompilation). 4) Configuring modifiers using the index to order them is awkward and error- prone. The concept of some sort of indexed insertion into a sequence of modifiers doesn't even seem to be one that there is a real use for - an implied by order of insertion approach would work at least as well. 5) While applying modifiers to logs rather than to appenders is (potentially) more efficient as it avoids doing the modification N times for N appenders, in reality I have yet to encounter a case where the log format wasn't associated with the destination, rather than the source (ie. a log file should have a consistent format). An extreme example is when the log is a structured log such as the windows event log. A simple example is the newline modifier. If an appender is expecting to receive log messages it is up to the appender to delimit the messages in the appropriate way - be that by packing it into a syslog UDP packet or just sticking a newline on the end. I don't think the newline modifier should exist at all, but in other cases it does make sense to include log specific information. I'm not sure that for typical use where N is small that the efficency gains are likely to be significant, but it would also be possible to improve efficiency issue in cases where it is a real concern by using a multiplexing appender (that forwarded formatted output multiple "real" appenders). The modifier concept also blurs the line between insertion of information and formatting - if a log should always contain some particular information at the start of it what is wrong with just writing it to the enabled_logger's stream when it is created. This also leverages all the built-in and extended output and formatting facilities of std streams (including rather more internationalisation support than found in strings). This fits in with the user extensible logger attributes, predicates etc above. I would like to see 2 points at which the content can be modified - when the stream is created, so that informartion that should be a prefix on all logged messages via a particular log, regardless of destination can be inserted there, and another associated with the appender, rather than any particular log, so that any content that must always be inserted in messages to that log can be inserted/formatted there.
- What is your evaluation of the implementation?
There are some areas of the core library, especially robust thread safety, that need attention. The most glaring of these is the m_deleted hack to try to work around the use of a reference in place of a copy of the logger (*not* the logger_impl - copying a logger is cheap - it is only a shared_ptr). There is also an outstanding fixme re. the problem of logger_impls being kept alive by their presence in the logger collection even after they are otherwise out-of-scope. The appenders supplied are, in general, not much more than examples. While this is fine in that user-written appenders are a great customisation point, I think it is really essential that at least a few robust, high performance appenders be offered. I think it is obvious from the above design comments that I think an overall cleanup/refactor reflecting the final design is in order.
- What is your evaluation of the documentation?
The documentation is a nice friendly tutorial. However, there is a severe lack of design documentation supporting rationale, and no real reference section.
- What is your evaluation of the potential usefulness of the library?
A good logging facility is essential in most (I'm tempted to say all) applications of any complexity, and current solutions tend to introduce undesirable coupling or lack features/flexibility. The library is potentially very useful not just as a developer's debugging tool but as a part of any application that needs configurable logging of any sort.
- Did you try to use the library? With what compiler? Did you have any problems?
Yes (an earlier version) gcc 3.3 and 3.4. I didn't have any real problems with it, but I haven't tried to install and use "out of the box" as I was modifying it at the time.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I've made a reasonably in-depth study of and changes to versions of this library. I'd say I'm very familiar with it.
- Are you knowledgeable about the problem domain?
Reasonably so - I've written logging components as part of various systems before.