Re: [boost] [log] Comments

Subject: Re: [boost] [log] Comments
From: Vladimir Prus (ghost_at_[hidden])
Date: 2010-03-15 16:22:35

On Monday 15 March 2010 22:39:05 Andrey Semashev wrote:

> On 03/15/2010 09:10 PM, Vladimir Prus wrote:
> > On Monday 15 March 2010 20:24:49 Andrey Semashev wrote:
> >>
> >> I can't provide logger features for every case. Sometimes users will
> >> have to extend the library. Writing a class template with a couple
> >> constructors and a trivial method doesn't look very complex to me.
> >
> > First, it seems to me that if two major projects use component/severity scheme,
> > then presumably it's of general utility. Second, in the thread you have
> > mentioned, you yourself provide example solution that are far from trivial.
> > Maybe I'm missing something? In essence, I would like to have:
> >
> > 	magic_logger lg;
> > 	DO_MAGIC(lg, "my_component", debug)<<  "hi there";
> >
> > Could you show what will it take?
> 
> I've shown that, in the thread you mentioned. It simply appears that I 
> don't think that's complex and you do. No problem, if it deems that 
> necessary, I can add yet another feature to the library distribution.

OK.

> >> It's not scary, it's verbose.
> >> If you want to get down to the guts of the library, you're welcome. You
> >> get the full control at the cost of verbosity. But most people won't
> >> need it and will just use the shorthands, such as "extract" and "attr".
> >> I see no problem with it.
> >
> > As already expressed by Tom, logging library is fairly basic component
> > that should be useable by everybody. And lambda functions in current C++
> > in not something everybody know so forcing the user to pick between
> > using lambda, and much more verbose non-lambda style does not help.
> > Another aspect mentioned during review is that you roll your own lambda
> > implementation. Which means that folks that are familiar with boost::lambda
> > might run into things that don't work, or work differently. Further,
> > suppose one has access to a compiler that already implement native lambda.
> > He probably would prefer that to your lambda emulation, but you impose
> > a convenience tax on that.
> >
> > Why cannot you provide and equally convenient ordinary functions?
> 
> Can you suggest a better interface? Because I can't imagine how to make 
> it shorter, while keeping the other features intact.

Your example when using a freestanding function was:

           shared_ptr< logging::attribute > attr = attrs["System"];
       if (attr)
       {
         optional< System > sys = attr->get< System >();
         if (!sys)
           throw runtime_error("The System attribute has invalid type");

         if (sys.get() != m_sys)
           return false;
       }
       else
         throw runtime_error("The System attribute not found");

The corresponding code using lambda would be, presumably:

        flt::attr< ...  >("System") == m_sys

Would it be possible to make the following work in the first case:

        if (attrs["System"] != m_sys)
                return false;

? Or, if changing type/behaviour of operator[] is undesirable, what
about

        if (attrs->get_nothrow("System") != m_sys)
                return false;

?

 
> >>> * Why can't I use the library without named parameters? Surely,
> >>> there are low-tech ways to specify things.
> >>
> >> What are these?
> >
> > The simplest would be:
> >
> > 	file_log_parameters p;
> > 	p.filter = ... ;
> > 	p.formatter = ... ;
> > 	init_log_to_file(p);
> >
> > This is straightward for any user, produces clear error messages,
> > and is not much of a inconvenience.
> 
> The filter is the property of the log frontend, and formatter is the 
> property of the backend. Using named parameters allows to direct the 
> appropriate parameters to their particular receivers. 

Is this some magic provided by Boost.Parameters, or you still need
to manuall route the properties to relevant components?

> Also, it's more 
> efficient than filling the structure.

This is init code, so performance is likely not very important.

> >> Boost.Parameter is used in various places of the library, but its
> >> primary goal is to provide abstraction between components that would
> >> otherwise be tightly coupled. For instance, loggers consist of number of
> >> features, each of which is independent from others. Named parameters
> >> elegantly allow to interact with a particular feature without bothering
> >> other features.
> >
> > Could you give more examples?
> 
> http://boost-log.sourceforge.net/libs/log/doc/html/log/extension/sources.html#log.extension.sources.creating_a_new_logger_feature
> 
> In the end of the section you can see how two logger features are 
> composed into a single logger. Both features require a parameter to open 
> a log record. Named parameters allow to pass these parameters to the 
> respective features, while neither of the features is aware of the other.

I agree that this composition of features won't work with 'big-struct-with-all-parameters'
approach suggested above. Then, what about this:

        my_composite_logger lg;
        lg.set_feature_1(...);
        lg.set_feature_2(...);

It seems to me that such interface will not require that features know about each other.
 
> Another example. init_log_to_file may receive a number of named 
> parameters. Some of them are used by the sink frontend, some - by the 
> backend. In fact, you can actually pass all parameters to the frontend 
> constructor - it will pick those it needs and pass the rest to the 
> backend. That way the frontend is not aware of parameters that the 
> backend requires.

Just to clarify -- is there a check that all named parameters are actually
used. I admit I don't know much about Boost parameter, so might be doing
something wrong, but the attached diff to basic_usage example still
compiles. Is this a bug or a feature?

> >>> * The library went too far with namespaces. Why are *six* nested namespaces
> >>> necessary? This appears to be just asking for extra typing for no good
> >>> reason.
> >>
> >> Use namespace aliases. Most of the time you will only need the sources
> >> namespace. Other namespaces were introduced to keep code clarity and
> >> avoid name clashes.
> >
> > Of course I can use namespace aliases to alleviate the problem. However,
> > I don't see how namespaces are necessary. Are there actually things with
> > the same names in those different namespaces?
> 
> Yes. attr is a formatter and a filter, for example. 

Oh, indeed! But.. why? Cannot one object serve both roles?

> There are "format" 
> names in "formatters" and "keywords" namespaces. Perhaps other exist, I 
> didn't examine the code thoroughly.

> >> Record identifiers proved to be very useful in communication,
> >> surrounding the piece of log. It's much easier to write "see, there's
> >> that error in line 2764" than to go for lengthy explanations of how to
> >> find it in the file.
> >
> > What other libraries use this record id by default?
> 
> None, perhaps. That doesn't make this feature any less valuable.
 
> >>> Also, it prints the thread "number",
> >>> on my system a hex string. I am not sure thread id should be part of trivial
> >>> logging.
> >>
> >> Why not? Why can't a multithreaded application use trivial logging?
> >
> > Because hex string, in a log, is completely useless. It is of any use if you
> > are attached to a still-running application, and can actually figure what
> > a thread id means in your program.
> 
> I believe, Robers has already answered this. I agree with him.
> 
> >>> Finally, I think by default, everybody things of logging as glorified
> >>> std::cout, so trivial logging better print messages to console, not to a file.
> >>
> >> I disagree. First, you don't need the library to spam the console.
> >
> > Well, I do. Maybe because my console is usually not cmd.exe ;-)
> 
> You mean you can't use std::cout? What kind of console are we talking about?

Oh, of course I can use std::cout to print to console. However, I want to
be able to enable and disable log message from specific components, and
std::cout cannot do that.

> >> Second, I consider spamming the console to be a bad idea in the first
> >> place. In my practice there are cases when writing _anything_ on the
> >> console is forbidden.
> >
> > It seems like a fairly specialized requirement. What is trivial logging
> > supposed to be? If it is supposed to be printf logging on steriods, then it
> > should behave like printf as much as possible, including printing to console.
> 
> Log, in the common sense, is perceived as a file. Console is not a good 
> place to write logs, since this is a place for user interaction, not for 
> a protocol of execution.

I think that many non-deamon Linux program that permits enabling logging logs
things to stdout. In fact, I don't remember any non-daemon program that would
put logs to a file when invoked by user.

> >>> * Why does the library have light_rw_mutex? I would really prefer if everything
> >>> thread-related be located inside Boost.Thread, so that at least the code can
> >>> be readily audited.
> >>
> >> light_rw_mutex can be more efficient than shared_mutex.
> >
> > Can, or is?
> 
> It is, in some configurations. In particular, on POSIX and NT6 (when 
> enabled). On POSIX someone on this list presented tests comparing 
> pthread_rwlock_t (which is used by light_rw_mutex) against shared_mutex, 
> former being faster. On NT6 light_rw_mutex is of size of one pointer 
> which is ~6 times smaller than shared_mutex. The performance is 
> comparable, according to my tests.
> 
> >> If it gets to  Boost.Thread, I'll be first to use it.
> >
> > It's not like maintainer of Boost.Thread left on a space mission to outer
> > space and is not coming back ;-) Could you just propose this for inclusion?
> > We have too many bits hidden in detail namespace of various libraries.
> 
> This matter is kind of separate from the library review. I have no 
> problems proposing this component for inclusion Boost.Thread, after the 
> review. 
> >>> * It appears that the default behaviour of fmt::attr is to throw is the attribute
> >>> with such name is not found. This seems like a bad choice to me. A logging
> >>> library should never fail -- it is the last resort at diagnosing a problem in the field,
> >>> and if it throws by default, it's useless.
> >>
> >> That was my thought initially. Then this thread had happened:
> >>
> >> http://article.gmane.org/gmane.comp.lib.boost.devel/185956
> >>
> >> You can suppress exceptions with exception handlers on different levels
> >> of the library.
> >
> > I saw that -- my point is that *default* behaviour should be to go on no
> > matter what is wrong.
> 
> I think, this is probably the point where different opinions collide 
> with none being right or wrong. In a classical case of logging as an 
> application execution protocol writing, you're absolutely right. In 
> other cases, when logging plays more central role in the application, 
> the other way around is more desirable.

I'm not sure I understand those other cases. Let's wait what others will
say.

> >>> * Building the library with threading=single results in unresolved references to pthread functions.
> >>> I belive that library should not use any sync mechanisms in this case.
> >>
> >> Yes, I've been told that. Presumably, the references are from
> >> Boost.ASIO, which is used by the syslog backend. Not sure what would be
> >> the best solution - to drop single-threaded builds or to exclude syslog
> >> in ST builds.
> >
> > Well, if this the case, Boost.ASIO should be fixed not to contain its own
> > thread-related mechanisms -- which only reinforces my position about
> > rw_mutex.
> 
> No, it doesn't reinforce your point. ASIO may well not be suited to be 
> used in a ST environment.

It might not be suited, but by using custom threading code as opposed to
Boost.Thread it manifests this unsuitability in a inconvenient way.

- Volodya

> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
> 

Thanks,