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:32:31

----- Original Message ----- 
From: "Andrey Semashev" <andrey.semashev_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Wednesday, March 17, 2010 9:26 PM
Subject: Re: [boost] [log] Boost.Log formal review closing down

> 
> On 03/17/2010 03:42 PM, vicente.botet wrote:
>>
>> ----- 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();
>> }
> 
>   void my_formatter(std::ostream& strm, logging::record const& rec)
>   {
>     logging::extract< unsigned int >("LineID", rec.attribute_values(),
>       lambda::var(strm) << lambda::_1 << ": ");
> 
>     logging::extract< severity_level >(
>       "Severity",
>       rec.attribute_values(),
>       lambda::var(strm) << "<" << lambda::_1 << "> ");
> 
>     strm << rec.message();
>   }

Sorry I find this yet clearer

void my_formatter(std::ostream& strm, logging::record const& rec)
  fmt::stream
    << attr< unsigned int >("LineID")
    << ": <" << attr< logging::severity_level >("Severity")
    << "> " << message()
}
 
>> *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.
> 
> That would complicate the library, both in terms of usage and 
> implementation. I'd rather not introduce several different interfaces 
> for the same thing.

I don't know if this will complicate or not your library. One thing is clear, accessing attributes by using string is expensive, and the user expects the best performance. If your library doesn't respond to this request, your library will not be used at the end.

My proposal was multiple to respond to the needs of other users. From my side it is enough to identify an attribute using a tag. What is wrong using tags?
 
>> * Which is the interest of registering the attribute "ProcessID"
> 
> Some users expressed the need to start logging by appending to the log 
> file leftover from the previous runs of the application. This attribute 
> can help to detect the point of the application restart. 

Well, maybe it could be useful. I have never use it in my ammpications as each process log on different files localy, or use distributed log service.

Could you add an example on the documentation?
BTW, which is the value of ProcessId, a number?

> The attribute 
> may also be useful if log records are sent to a remote process, which 
> may accumulate logs from a group of processes.

May be useful, but current there is no backend sending log recods remotely.

Best,
Vicente