Re: [boost] [log] Boost.Log formal review closing down

Subject: Re: [boost] [log] Boost.Log formal review closing down
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2010-03-17 17:13:22

Continuation ...
----- Original Message ----- 
From: "vicente.botet" <vicente.botet_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Wednesday, March 17, 2010 1:42 PM
Subject: Re: [boost] [log] Boost.Log formal review closing down

> 
> 
> ----- Original Message ----- 
> From: "vicente.botet" <vicente.botet_at_[hidden]>
> To: <boost_at_[hidden]>
> Sent: Wednesday, March 17, 2010 9:26 AM
> Subject: Re: [boost] [log] Boost.Log formal review closing down
> 
> 
>> 
>> Hi, 
>> 
>> I though the review will continue until Sunday. 
>> 
>> 
>> Here is a a part of my review. I will continue this evening.
>> 
>>> - What is your evaluation of the design?
>> 
>> First say that in general I like the library general architecture. Going into details:
>> 
>> * Trivial log
>> Trivial log should be logged to std::cerr 
>> Trivial log should contain in the record message only the application part and the severity. If the user want line number, a timestamp, thread identifier the library need to let the user configure this trivial logger.
>> 
>> * Filters, Formatters and attributes:
>> An attribute is a different think than a filter. Filters contains attributes as part of its expressions.
>> 
>> attr< logging::severity_level >("Severity") should return the attribute value when called.
>> 
>> The following expression
>> 
>> attr< logging::severity_level >("Severity") >= logging::info
>> 
>> should return a filter. Can this make be possible and unanbigous?
>> 
>> Note that I have removed the trivial namespace which didn't add any information in the preceding expressions.
>> 
>> The same applies to formatters
>> fmt::stream
>> << fmt::attr< unsigned int >("LineID")
>> << ": <" << fmt::attr< logging::trivial::severity_level >("Severity")
>> << "> " << fmt::message()
>> 
>> should be just written
>> 
>> fmt::stream
>> << attr< unsigned int >("LineID")
>> << ": <" << attr< logging::severity_level >("Severity")
>> << "> " << message()
>> 
>> or even the library could provide some functions returning the predefined attrributes:
>> 
>> fmt::stream
>> << line_id()
>> << ": <" << severity_level()
>> << "> " << message()
>> 
>> Specific formats could be written as
>> 
>> fmt::stream
>> << (format("%08x") % line_id())<< ": <" << severity_level()<< "> " << message()
>> 
>> 
>> * Rotation on files.
>> Can rotation be based only on time? Can we rotate at 2:00AM every day?
>> How many rotation files are maintained? Can this number be configured?
> 
> * Custom formatting functons
> I find this code extremly complex
> void my_formatter(std::ostream& strm, logging::record const& rec)
> {
>    namespace lambda = boost::lambda;
> 
>    unsigned int line_id;
>    if (logging::extract< unsigned int >(
>        "LineID", rec.attribute_values(), lambda::var(line_id) = lambda::_1))
>    {
>        strm << line_id << ": ";
>    }
> 
>    logging::trivial::severity_level severity;
>    if (logging::extract< logging::trivial::severity_level >(
>        "Severity", rec.attribute_values(), lambda::var(severity) = lambda::_1))
>    {
>        strm << "<" << severity << "> ";
>    }
> 
>    strm << rec.message();
> }
> 
> I would expect something simpler as
> void my_formatter(std::ostream& strm, logging::record const& rec)
> {
>    strm << rec.line_id() << ": ";    strm << "<" << rec.severity() << "> ";
>    strm << rec.message();
> 
> }
> What am I missing?
> *Attributes:
> I don't like that std::string is used as attribute key. I suspect that this is not the more efficient way to have dynamics attributes.
> I would prefer the library manage several kinds of attributes.
> 
> Some attributes could be identified staticaly using a tag.
> 
> tagged_attr< unsigned int, tag_severity >
> 
> Others using a std::string key
> 
> named_attr< unsigned int >("id")
> 
> and others using an int index
> 
> index_attr< unsigned int >(3)
> 
> The mapping could be done at two levels: 
> The first level maps &type_info, to either the value_type extractor.
> - For tagged attributes the extractor is able to get the attribute value directly.
> - For named attributes and index attributes the extractor needs the additional name or index information to get attribute value.
> 
> 
> * Which is the interest of registering the attribute "ProcessID"
> 
> * Severity: I would like that any sink filters thie severity the log_record is not added at all. Could this optimization be taken in account. For example if  severity must be GT error to be logged, the following will not evaluate the ineficient_function().
> 
> BOOST_LOG_SEV(slg, normal) << ineficient_function();
> 
> More later,

* __FILE__, __LINE__ and BOOST_CURRENT_FUNCTION are missing. The library could make easier is use. 

* all the logger macros should be able to generate non executed code. In addition I would like that the filter on the severity be included in. For example

#if defined (BOOST_LOG_INCLUDED)
#define BOOST_LOG_SEV(LOGGER, SEVERITY) if (true); else logger
#else
#define BOOST_LOG_SEV(LOGGER, SEVERITY) \
    if (boost::log::core::enabled(SEVERITY) 
    //as before
#endif

The rational is that most of the time the application should not trace nothing, except critical errors. This test avoids the construction of the record_log, and evaluate the filter on all the attributes. In the same way the severity is a special attribute, we need special filter for this attribute.

- What is your evaluation of the implementation?

I 'm confident the author know well C++ and will be able to improbe the library if he take care of the suggestions of this review.

I would like the assynchronous front end uses one lock_free queue by thread which will have less contention than the actual implementation using one queue for all the threads.

- What is your evaluation of the documentation?

The documentation is quite good. It can be however be improved by:
* adding links when a class or funcion appears on the text to the references, 
* adding links to the complete examples or even bettern include them with systax highligled on the documentation
* adding the output expected by the execution of concrete examples.

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

Completly useful. Every body uses logs in a profesional context.

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

Yes I have tried to use it on cygwin-gcc3.4, and mingw-gcc4.4 and msvc Express 9.
* I get a lot of errors on cygwin as there are nor wide characters on cygwin.
* when compiling the library using just bjam, there were alot of errors because the library don't work on single_threaded environements.
* when I compiled with msvc on mingw, a 'mc' tool was missing
* when I compiled with gcc4.4 on mingw the same issue with 'mc'

Unfortunatelly these error have not been corrected.

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

I have spent a lot of time trying to use the library without success. So my review is based on the documentation.

- Are you knowledgeable about the problem domain?

Yes, I use to use log in all my applications, and we have an asynchronous implementation using one queue by thread that outperforms the one using one queue for all the threads.

In our applications the use of asynchronous logging is an imperative.

3. Please explicitly state in your review whether the library should be accepted.

I think the architecture of the library is the correct one. I have signaled some points of improvement either on the design, the implementation or the documentation. I know the author and I don't expect he will take care of all my suggestions. 

Anyway I think that Boost needs a Log library, so even if Boost.Log should be improved in quite a lot details, it is not less useful other libraries already included on Boost.

Thus, I vote for a provisional inclusion on Boost, as there are a lot of improvements that will need to be checked for coherency.

Best,
Vicente