Make log4cxx optional (?)
Opened this issue · 18 comments
Another possibility would be to use Boost's logger, since Boost is already a dependency. But then we can also make the Boost logger support optional.
Yeah but the thing is that Boost Logger is still too new to be considered :(
(not available on Ubuntu 12.04)
I think that Boost >= 1.48 is required to install the former library, so it would just be another dependency. I don't know if there's some APT packages for it for pre-Boost 1.54 systems though.
Some Windows users (poke @aescande) would also like to see the dependency made optional (or removed), to make Windows compilation easier. I guess we could wrap the log4cxx calls in some generic logging macros (LOGGER_INIT, LOGGER_INFO, LOGGER_ERROR etc.). Thus, disabling the logger or even changing the logger library would be fairly straightforward.
Ok, this is more or less the case in core/debug.hh, so this should be even easier than expected.
After thinking about it, I think that having one handler stored as a static pointer to function is fine. If you want to enable logging then you got to define your handler. We can provide one printing on stdout by default. This is good enough I think :)
I guess we need at least debug, info and error. With static pointers to functions, users could log info to a file and error to stderr for instance.
Yeah but this can just be an enum to keep a simple interface.
Then the dispatch can be done into the class somehow...
We can consider something like:
// Register logging functions
registerLogger (INFO, ...);
registerLogger (ERROR, ...);
...
// Log some information
log (INFO, "something amazing happened");Having this API is fine, my recommendation is to only have one underlying static variable to minimize issues with initialization/destruction as well as potential concurrency issues with multi-threading.
It is reasonable to make the assumption that you don't change the logger while solving a problem but the log method must be thread safe...
We can also have multiple logger instances defined by some identifier (e.g. string such as "roboptim.core").
getLogger ("roboptim.core").log (INFO, "log message");This seems to be what log4cxx is doing. This can be done with a unique global map, and getLogger takes care of logger creation when necessary.
Thread-safety is indeed my main concern (and the usual thorn in the side of developers when dealing with loggers).
This is why separating the issues is important. A logger is two things:
- A singleton (difficult to make it thread safe)
- And a class (which can be thread safe, efficiency is another, separate, concern)
My suggestion is keeping 1. minimum, one static object, one function to set it.
Once this is done, then 2. can be implemented.
For 1. I feel that omniorb had it right:
http://omniorb.sourceforge.net/omni40/omniORB/omniORB004.html
namespace omniORB {
typedef void (logFunction)(const char);
void setLogFunction(logFunction f);
};
You may want to make it slightly more complicated with severities but not too much more.
The for 2. lockless structures in boost can be the solution. Then you got that then you can write on the disk whatever you want later.
Another thing to consider: a simple const char-based logger usually requires an extra step on the user's side to handle stringstream expressions (e.g. "matrix: " << mat). I guess log4cxx handles that in their macros that prefix the logged expression with a stringstream and returns the resulting string.
Converting a matrix to a string is too slow to be done synchronously so ultimately, it is better either to copy (for small objects) or to keep a shared pointer (for bigger ones, if possible) so that you can do that separately.
One solution is to add directly into the logger the possibility to log basic types such as double, matrices and vectors to defer the transformation.
Ultimately if the data volume is too important, relying on binary logging may be needed.
This was just a dummy example, plus logging should be kept to a minimum in most cases (except in debugging scenarios where performance does not really matter).
Being able to store expressions simply containing references/pointers to large data would indeed be an interesting solution, but this is probably overkill in this case (and probably requires a lot of work). This could probably be part of a standalone logging library that could then be used in the user-defined logging functions considered here, but this would imply using a more complex log function signature...
I think that for now, we should keep things simple (thread-safe, handling strings and stringstream expressions), and leave the heavy-lifting to the user-defined log functions (possibly calling Boost.Log, log4cxx etc.).
Yeah, of course. My only point is that if you make an interface at some point, it is better to have it take directly the object you want to log (matrices...) than strings as it will allow you to process them on the logger side.
For this, defining a boost::variant with supported types and expecting a user-defined visitor that describes how to deal with different types may be a solution. This just makes the user's job more difficult, since one needs to define how to deal with strings, matrices, etc.
Yeah something like that. I would be more in favor to just use an abstract class to avoid the maintenance cost of the template. A more complex design could allow the user to register its own type but again this seems a bit too much knowing that we mainly log vector and matrices.