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 08:42:26

----- 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,

Best,
Vicente