|
Boost : |
Subject: Re: [boost] [logging] Interest check on logging library.
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2009-01-01 09:51:39
Jason Wagner wrote:
>
> It started because I thought yours was no longer in development. I
> have no idea why I thought that, since looking at your cvs logs you
> have been consistently working on it. It was largely a mistake.
The library is very much alive, although for various reasons I didn't
touch it for the last few weeks.
> I'm writing a internet spaceship game. There's a client and a
> cluster of servers with different types. The protocol is simple
> message passing, with the serialization code shared between client
> and server. This leads to a very simple example like:
>
> if(message.size > packet.size) { log << severity::warn() << "Packet
> was larger than the buffer that contains it!" <<
> build_target::server() << " Packet source= " << connection << ",
> account = " << user << "\n" << dump_hex(packet); // error handling }
>
> On the client side, we note that the packet was too big and move on.
> On the server side, we actually log the packet since evil hackers are
> always trying to steal my space bucks. Since the template generates
> empty code at compile time, both gcc and msvc are smart enough to
> remove the " Packet source=" and ", account= " strings from the
> binary, providing less clue in the client as to what the server is
> watching for. On the server side, I probably want this record to go
> into a database, so having the entire thing as a single record is
> very important. That precludes items of the form:
>
> log << severity::warn() << "..."; if(server) // or #ifdef log <<
> "Packet source = "; //...
>
> I also really don't like having the conditional in the logging.
> Looking at the serialization code, one shouldn't have to wade through
> the logic of logging.
I see two issues with your approach:
1. Although compilers may be smart enough to optimize away writing
constants and simple variables into a dummy stream (which, I assume, log
is when logging is disabled), I'm not so sure about more complex
expressions, like writing the result of a function call, like dump_hex
in your example. I didn't investigate this, but I suspect the call will
still be present in the compiled code, which, AFAICT, is not what you
intended.
2. I'm not sure how you would implement filtering. More on this below.
From where I'm standing, I would suggest using macros. It's not that
bad in this particular area and would not require changing your code a lot:
if(message.size > packet.size)
{
LOG() << ...;
}
where LOG can be defined differently for client and server builds. In
connection to my library, it could be:
#ifdef SERVER
# define LOG(logger) BOOST_LOG(logger)
#else // client
# define LOG(logger) while (false) BOOST_LOG(logger)
#endif
That will drop unnecessary code for sure on practically any compiler.
> I think I could add this to your library using features. That's not
> a light weight thing to add, based upon what I see features need to
> be instantiated. In my scheme, since the if() test is built into the
> log record instantiation there's single point to change to affect all
> appropriate loggers. With your scheme I'd have to change the logger
> type, the if()s that wrap them or the macros that contain the ifs,
> and possibly client code as well.
>
> Switching to a different scheme, I could do:
>
> log << severity::warn() << "Packet was larger than the buffer that
> contains it!" << auditing::full() << " Packet source= " <<
> connection << ", account = " << user << "\n" << dump_hex(packet);
The problem is that you ignore run time filtering here, which should
happen before formatting the record (i.e. before the streaming statement
you mention above). If you are fine with it, your approach is
sufficient. Otherwise you'll eventually come to something more complex,
maybe something like my lib. :)
If all you need is to globally enable or disable logging at compile
time, you can define macros as I've shown above. There's no need to play
with loggers or complicate your code with conditionals. If you need run
time filtering, the following constraints appear:
1. You cannot mix the attributes (sorry, I don't know the equivalent
term for your library) into the streaming expression. The reason is
obvious: attributes participate in filtering, and filtering takes place
before the streaming.
2. You will have to write an "if" around the streaming statement. That
"if" will check the filters in run time and decide whether the record
will be processed or not. You can wrap it into a macro, but it will be
there, and it will not be zero-cost in terms of performance and code size.
3. All attributes you want to put to log have to be set up before that
"if" statement. My lib provides scoped attributes (or tags) for that
purpose, although they may not look as neat as your manipulators in the
streaming statement. In your use case the scoped attributes can be set
in the macros I've shown above, so that they will not be present in the
client code.
> Now, on the client side I can turn off full auditing at compile time
> and on the server time I can turn off packet logging at run time and
> have a condition that turns it on when I suspect a problem. There is
> a separation of concerns between what audit level and severity of the
> record.
>
> The whole thing could be rewritten with scope variables for
> connection and user that would allow them to be first class denizens
> of the database or fed into an event processing system without having
> to do entity extraction on the record. In addition, immediately
> insertable variables prevent the overhead of setting scope variables
> if not needed:
>
> log << severity::warn() << "Packet was larger than the buffer that
> contains it!" << auditing::full() << << set("User", user) <<
> set("connection",connection) " Packet source= " << connection << ",
> account = " << user << "\n" << dump_hex(packet);
I'm sorry, I'm afraid, I don't understand what you're saying here.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk