Boost.Logging: formal review
Hi, Being the author of at least 3 different solution for logging/tracing myself, I do believe it's one of the most important general purpose library in big project development. Unfortunately proposed submission in no way came close to being an acceptable choice. Numerous design and implementation issues, weak user and absent reference documentation and absent test suite (last two points is something that review manager is responsible for IMO. No tests or docs - no review. That's it) result in: My vote is NO. Please read further for detailed analysis. I. Design Flaws ------------------ 1. Filtering support I understand that author strived to minimize the solution, but IMO it lead to minimal value. IMO proper log system should support arbitrary user defined filtering of log entries. At the bare minimum it should support: entry level - levels set is ordered set of values indication importance of information provided. Filtering is based on threshold. Examples: DEBUG, INFO, MAJOR entry category - categories set is a set of unique values indicating kind of information provided Filtering is based on masking. Examples: ARGS, RETURN_VALUE, ERROR_LOG, DATA_FLOW, PROG_FLOW entry keyword - keyword set is set of user defined keywords (most frequently strings) identifying area of the program. Filtering is based on match of keywords. Keywords usually are used to mark specific part of application. Example: "NetworkLevel", "UI", "ABC" to mark class abc, "dfg.cpp" Also multithreaded application should support filtering based on thread id. The best solution would allow to plug-in at compile time any user defined Filter (in a form of Policy). This way I could have a log that filter nothing (no filters defined) or log that filter based on time of the day if I want. This submission does support level filtering - and that's it. 2. Configuration support Submitted solution supports configuration in a form of arbitrary modifiers that user could register in log. While this looks like very inflexible it's also as very inconvenient. In most cases I prefer log to be configured at run time from within configuration file. his would be very cumbersome with modifiers model. Not only each modifier needs to be added separately. I also need to keep track of it's name to be able to remove it and it's index to know how modifiers are ordered. This ability to configure log remotely in many locations makes it impossible to really know what and how is written into log. My preferred way to configure the log is with use of configuration string. First of all it's simplify an interface and you immediately see how your log entries going to look like. Here is an example: "keyword=*,-ACD;categ=prog_flow,return value,-details;l=debug;track=on;roll=10000;prefix=file ( line ) - time :;timeformat=%s:%m". Using pluggable factory pattern you could implement this to be as flexible. Eventually as an add-on you could provide an ability to register any custom modifiers, but that is not necessary in first draft. 3. Per output configuration. Log may have multiple appenders attached (BTW - I very much prefer term used by iostreams library - sink). what if I want different entry formatting/prefixing/filtering in different outputs. I may have errors_output which contains only errors. I may have end_user_log that contains only major information for end user with detailed time and date of the entry. And I may have developer_output that contains performance warnings with file and line information. Submitted library have no support for this. 4. Internationalization support I do see some vague traces in code that indicate that author new this needs to be supported. But since documentation have exactly zero information and I couldn't figure out how to do it myself, I assume that library doesn't support this. IMO it's required for industrial strength solution. 5. File and line This solution completely ignores file and line number of trace entry. IMO They are essential and should be collected for every entry (may not be reported - depends on prefix format) 6. Misfeature: pre-init cashing Library starts with cashing turned on to support pre-init logging. IMO this is too dangerous. Now developer needs to remember how many log entries are printed. Otherwise some of them will just start disappearing (could be different in every run). And it may start happening only at the user site. Much more reliable solution is to use some kind of startup log that does not require configuration. 7. Misfeature: log hierarchies I personally never have a need for that and have some difficulties to imagine why would anyone have. You could probably have it on top of existent solution as add-on. If it doesn't require any new interfaces (or some minimal addition). IMO library pays way to much attention to this. All these manipulate_logs(...) by name that is analyzed not needed. In my experience I never wonted to configure more than one log identically. And it's much more preferable to keep all the configuration of one log in one place. So I wouldn't look around to see what and how was configured. 8. Misfeature: compile-time log support As it clear from above IMO log is (almost) never always enabled (and in any case there is a way to implement this without any additional support). And always disabled log could be implemented on macro level. Based on this I believe all the compile_tile machinery employed by library is unnecessary and just complicate implementation 9. Interface IMO primary interface advertised by library should be macro-less. Documentation does need to cover how these macros needs to be written, but with user defined filters user will need to do the job oneself. Also in my experience I found that on some systems Even if( ... ) <actual log here> could be unsatisfactory from performance standpoint. I employ one global Boolean is_log_on flag to guard all my log entries. and it should be a part of these macros 10. Performance. I am not sure wheather it's design or implementation issue, but library does a lot of premature pessimizations, which could be avoided. See in more details bellow. II. Implementation flaws ------------------ 1. File appender hierarchy To implement custom rolling library employs inheritance. This is clear example where delegation would be much better choice. Single file_sink that could be initialized with file_roller. 2. Multithreading support implementation a) For some unclear reason author chose to reimplement some of Boost.Threads functionality in some places (and in some places Boost.Thread is used always). I believe log library shouldn't do this. b) I noticed some fishy construct (m_destroyed data member). It opens some thread safety issues IMO. c) static manager_type & manager() doesn't look thread safe. 3 . enabled_logger::m_stream For some reason enabled_logger allocates m_stream dynamically for each log entry. I do not see why we need to do this. Even more: why not reuse a single instance per thread? 4. collection in use appender_array and modifier_array are better be implemented using lists 5. exception support try { LOG << "aaa" << foo() } catch(...) { LOG << "exception" } This construct doesn't seems generate what is expected if foo() throws an exception 5. appender_array appender_array seems to be defined in 2 placed 6. Appenders copying.
From code it seems that each logger has its own copy of each appender. Why would we need that? If we do why all the appenders are so heavy?
7. #ifdef UNICODE #ifdef UNICODE are spread all over the code. It needs to be centralized 8. shared_memory.hpp This header refer to the library that is not part of boost yet. I don't think it should be part of this submission. 9. Time string cashing prepend_time and prepend_time_strf calls are not cashed. It's a lot of work for nothing. 10. write_msg locking write_msg shouldn't lock on appenders writing. Each appender should be locked independently instead. Documentation is better be commented by somebody else. I thing though : no reference docs whatsoever. As well as no test suite. Hope my comments were constructive. Regards, Gennadiy
I. Design Flaws ------------------
1. Filtering support I understand that author strived to minimize the solution, but IMO it lead to minimal value. IMO proper log system should support arbitrary user defined filtering of log entries. At the bare minimum it should support:
entry level - levels set is ordered set of values indication importance of information provided. Filtering is based on threshold. Examples: DEBUG, INFO, MAJOR
How's this different from levels, in the library?
entry category - categories set is a set of unique values indicating kind of information provided Filtering is based on masking. Examples: ARGS, RETURN_VALUE, ERROR_LOG, DATA_FLOW, PROG_FLOW
What? I'm really curious how a line of code for logging would look, in your wishful lib?
entry keyword - keyword set is set of user defined keywords (most frequently strings) identifying area of the program. Filtering is based on match of keywords. Keywords usually are used to mark specific part of application. Example: "NetworkLevel", "UI", "ABC" to mark class abc, "dfg.cpp"
In my lib, this is actually the log itself. You have a log, and can add appenders/modifiers to it.
Also multithreaded application should support filtering based on thread id.
And what's stopping you from making an appender that does just this? // trivial example struct only_for_thread_id { only_for_thread_id(int tid, appender_func forward_to) : tid(tid), forward_to(forward_to) {} void operator()(const std::string&,const std::string & msg) { if ( ::GetCurrentThreadId() == tid) forward_to(msg); } int tid; appender_func forward_to; }
The best solution would allow to plug-in at compile time any user defined Filter (in a form of Policy). This way I could have a log that filter nothing (no filters defined) or log that filter based on time of the day if I want.
See above.
2. Configuration support Submitted solution supports configuration in a form of arbitrary modifiers that user could register in log. While this looks like very inflexible it's also as very inconvenient. In most cases I prefer log to be configured at run time from within configuration file. his would be very cumbersome with modifiers model. Not only each modifier needs to be added separately. I also
Did you take a look at Darryl's proposal? (http://torjo.com/code/lib/alternate_manipulating.html)
need to keep track of it's name to be able to remove it and it's index to know how modifiers are ordered. This ability to configure log remotely in many locations makes it impossible to really know what and how is written into log. My preferred way to configure the log is with use of configuration string. First of all it's simplify an interface and you immediately see how your log entries going to look like. Here is an example: "keyword=*,-ACD;categ=prog_flow,return value,-details;l=debug;track=on;roll=10000;prefix=file ( line ) - time :;timeformat=%s:%m".
You can implement Configuration Support on top of the current library. And besides, if you want to configure logging, you'll still need to identify each log, appender and manipulator by a string.
3. Per output configuration. Log may have multiple appenders attached (BTW - I very much prefer term used by iostreams library - sink). what if I want different entry formatting/prefixing/filtering in different outputs. I may have errors_output which contains only errors. I may have end_user_log that contains only major information for end user with detailed time and date of the entry. And I may have developer_output that contains performance warnings with file and line information. Submitted library have no support for this.
I don't recall anyone else asking for this, but anyway, if other people wish it, I can implement it. And besides, you can implement it even now (not very efficiently), if you truly wish: // this is an appender struct modifiers_and_then_appender { modifiers_and_then_appender(appender_func a) : a(a) {} void operator()(const string&, const string& msg) { string copy(msg); .. // call each modifier a(copy); } modifiers_and_then_appender& add_modifier( modifier_func m) { modifiers.push_back(m); } std::vector<modifier_func> modifiers; appender_func a; }; And you could have: manipulate_logs("err").add_appender( modifiers_and_then_appender(write_to_file("errors_output")) .add_modifier(append_enter) .add_modifier(prepend_time) ); manipulate_logs("*").add_appender( modifiers_and_then_appender(write_to_file("end_user_log")) .add_modifier(append_enter) .add_modifier(prepend_time("...")) .add_modifier(whatever) ) );
4. Internationalization support I do see some vague traces in code that indicate that author new this needs to be supported. But since documentation have exactly zero information and I couldn't figure out how to do it myself, I assume that library doesn't support this. IMO it's required for industrial strength solution.
How many times have you needed internationalization support logging?
5. File and line This solution completely ignores file and line number of trace entry. IMO They are essential and should be collected for every entry (may not be reported - depends on prefix format)
Yes, you're right. Added to my TODO list
6. Misfeature: pre-init cashing Library starts with cashing turned on to support pre-init logging. IMO this is too dangerous. Now developer needs to remember how many log entries are printed. Otherwise some of them will just start disappearing (could be different in every run). And it may start happening only at the user site. Much more reliable solution is to use some kind of startup log that does not require configuration.
And where would that log write to? What if it's very important data? To console? What if you don't have console? And lets face it -- when one starts using a logging lib, will at least trace one call to the debug lib. If he sees the line doesn't show up anywhere, he'll most likely read the docs. If you take a look at the Basic Usage, you'll see the 'flush_log_cache()' call. Besides, even if you don't call it and you write 256 messages to a log, the caching will automatically turn itself off and flush. The same happens if the application ends, and caching was still on.
7. Misfeature: log hierarchies I personally never have a need for that and have some difficulties to imagine why would anyone have. You could probably have it on top of existent solution as add-on. If it doesn't require any new interfaces (or some
Have you heard of modules, and sub-modules? What if each module/sub-module was to have a log to write to?
8. Misfeature: compile-time log support As it clear from above IMO log is (almost) never always enabled (and in any case there is a way to implement this without any additional support). And always disabled log could be implemented on macro level. Based on this I
Please enlighten me: how can you implement a disabled log at macro level?
believe all the compile_tile machinery employed by library is unnecessary and just complicate implementation
It was asked by users of the lib. I never use it, but they need it.
9. Interface IMO primary interface advertised by library should be macro-less.
Please provide such an implementation, so that the following works: some_log << some_lengthy_function(); Which, if some_log is disabled, some_length_function() is not called -- without the use of macros.
Documentation does need to cover how these macros needs to be written, but with user defined filters user will need to do the job oneself. Also in my experience I found that on some systems Even if( ... ) <actual log here> could be unsatisfactory from performance standpoint. I employ one global Boolean is_log_on flag to guard all my log entries. and it should be a part of these macros
As I recall, noone else asked for this -- but it's not hard to implement.
II. Implementation flaws ------------------
2. Multithreading support implementation a) For some unclear reason author chose to reimplement some of Boost.Threads functionality in some places (and in some places Boost.Thread is used always). I believe log library shouldn't do this.
It was requested -- some users wanted to remove dependency on Boost.Thread.
b) I noticed some fishy construct (m_destroyed data member). It opens some thread safety issues IMO.
It's used for the global logs (the ones defined with BOOST_DEFINE_LOG), which return a static reference to a boost::logging::logger - to know if the log is used after its destruction. I'm looking for better alternatives.
c) static manager_type & manager() doesn't look thread safe.
It's there only for function overloading -- the returned reference is never used
3 . enabled_logger::m_stream For some reason enabled_logger allocates m_stream dynamically for each log entry. I do not see why we need to do this. Even more: why not reuse a single instance per thread?
Yes, in the future, I can re-implement it as a single instance per thread.
4. collection in use appender_array and modifier_array are better be implemented using lists
Have you done some tests, or is this just a gut feeling?
5. exception support
try { LOG << "aaa" << foo() } catch(...) { LOG << "exception" }
This construct doesn't seems generate what is expected if foo() throws an exception
What is expected?
5. appender_array appender_array seems to be defined in 2 placed
Yes, once in the log_impl's class definition, and once in namespace boost { namespace logging { namespace {
6. Appenders copying.
From code it seems that each logger has its own copy of each appender. Why would we need that?
Because if we had one appender for all loggers, we could run into thread-safety issues.
If we do why all the appenders are so heavy?
I don't understand what you mean.
7. #ifdef UNICODE #ifdef UNICODE are spread all over the code. It needs to be centralized
Yes, you're right.
8. shared_memory.hpp This header refer to the library that is not part of boost yet. I don't think it should be part of this submission.
Why not? First of all, you don't need to include it, if you don't use it.
9. Time string cashing prepend_time and prepend_time_strf calls are not cashed. It's a lot of work for nothing.
Please explain.
10. write_msg locking write_msg shouldn't lock on appenders writing. Each appender should be locked independently instead.
It's because appenders/modifiers could be added/deleted while someone is writing a message to the log. Thus, it needs to be thread-safe. Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!
BTW before I start, I recalled couple more issues I forgot to mention in original post: 1. Design: Lightweight interface Log library brings a lot of dependencies. In some case I do not want this if I need to use the library in header. IMO log needs to support/implement some lightweight debug abstract interface Something like: struct DebugLog { virtual void log( string const& ) = 0; virtual void log( int ) = 0; }; struct Log : DebugLog { ... }; Now I could use DebugLog interface without need to know an implementation. It comes with the price of virtual call. I primarily use this for debug only logging. 2. Design: Enter/Exit function scope support I believe it's required for log library to provide some kind of support for scope tracking. It could be an addon to core functionality to it needs to be configurable. 3. Design: hexdump I believe log library needs to provide support for dumping memory in hex format on log file. It could be an addon to core functionality. 4. Design: logged assert I believe log library needs to provide support for custom assert. Such assert would log into log file and invoke system assert depends in assert level. It could be an addon to core functionality. 5. Implementation/Design: log entry data copying. Modifiers implemented by applying modifier for string that represent entry data. This model is inefficient in following regards: a) you need to make a copy of original entry data for every output. b) you need to prepend the strings in most cases - which is inefficient operation. c) you couldn't share the work for different outputs - you need to repeat it for every string IMO better model would be to apply modifiers to outputs instead.
I. Design Flaws ------------------
1. Filtering support I understand that author strived to minimize the solution, but IMO it lead to minimal value. IMO proper log system should support arbitrary user defined filtering of log entries. At the bare minimum it should support:
entry level - levels set is ordered set of values indication importance of information provided. Filtering is based on threshold. Examples: DEBUG, INFO, MAJOR
How's this different from levels, in the library?
Not much. But this is just one of the many *custom* filters library needs to support and supply.
entry category - categories set is a set of unique values indicating kind of information provided Filtering is based on masking. Examples: ARGS, RETURN_VALUE, ERROR_LOG, DATA_FLOW, PROG_FLOW
What? I'm really curious how a line of code for logging would look, in your wishful lib?
Actually since macros are not primary interface in different subsystems log may look different. Couple examples: 1. LOG( INFO, PROG_FLOW, "SocketLib" ) << "Connection esteblished to: " << address; 2. LOG( DEBUG, RETURN_VALUE ) << "Fetched value: " << v; Here a LOG macro refer to some keyword by name. Usually I have keyword per file, per library or per class. 3. DEBUG_LOG( DATA_FLOW ) << "Input msg: " < msg; Here DEBUG_LOG automatically apply DEBUG level and refer to keyword by name
entry keyword - keyword set is set of user defined keywords (most frequently strings) identifying area of the program. Filtering is based on match of keywords. Keywords usually are used to mark specific part of application. Example: "NetworkLevel", "UI", "ABC" to mark class abc, "dfg.cpp"
In my lib, this is actually the log itself. You have a log, and can add appenders/modifiers to it.
It has nothing to do whatsoever with log name. I mean single log. And entries are filtered by string (or not string) keyword. Actually all above were an examples of what library should provide as possible filters. Acceptable solution would allow custom filter specification.
Also multithreaded application should support filtering based on thread id.
And what's stopping you from making an appender that does just this?
// trivial example struct only_for_thread_id { only_for_thread_id(int tid, appender_func forward_to) : tid(tid), forward_to(forward_to) {} void operator()(const std::string&,const std::string & msg) { if ( ::GetCurrentThreadId() == tid) forward_to(msg); } int tid; appender_func forward_to; }
You must be kidding, right? I want filtering on log level. So that entry appear or not appear in all outputs.
The best solution would allow to plug-in at compile time any user defined Filter (in a form of Policy). This way I could have a log that filter nothing (no filters defined) or log that filter based on time of the day if I want.
See above.
I see nothing.
2. Configuration support Submitted solution supports configuration in a form of arbitrary modifiers that user could register in log. While this looks like very inflexible it's also as very inconvenient. In most cases I prefer log to be configured at run time from within configuration file. his would be very cumbersome with modifiers model. Not only each modifier needs to be added separately. I also
Did you take a look at Darryl's proposal? http://torjo.com/code/lib/alternate_manipulating.html)
And how that has anything to do with what I am talking about? It's just a twist of the same unacceptable interface.
need to keep track of it's name to be able to remove it and it's index to know how modifiers are ordered. This ability to configure log remotely in many locations makes it impossible to really know what and how is written into log. My preferred way to configure the log is with use of configuration string. First of all it's simplify an interface and you immediately see how your log entries going to look like. Here is an example: "keyword=*,-ACD;categ=prog_flow,return value,-details;l=debug;track=on;roll=10000;prefix=file ( line ) - time :;timeformat=%s:%m".
You can implement Configuration Support on top of the current library.
I do not see how it possible and why should I do this on top of useless API.
And besides, if you want to configure logging, you'll still need to identify each log, appender and manipulator by a string.
Why is that? I do not want or need to know log name to configure it. I have an instance.
3. Per output configuration. Log may have multiple appenders attached (BTW - I very much prefer term used by iostreams library - sink). what if I want different entry formatting/prefixing/filtering in different outputs. I may have errors_output which contains only errors. I may have end_user_log that contains only major information for end user with detailed time and date of the entry. And I may have developer_output that contains performance warnings with file and line information. Submitted library have no support for this.
I don't recall anyone else asking for this, but anyway, if other people wish it, I can implement it.
As of now you don't support this.
And besides, you can implement it even now (not very efficiently), if you truly wish:
[...] That is completely different from what I need. I don't like an idea of modifiers in a first place. Now you want to propagate this inconvenience and inefficiency into every output.
4. Internationalization support I do see some vague traces in code that indicate that author new this needs to be supported. But since documentation have exactly zero information and I couldn't figure out how to do it myself, I assume that library doesn't support this. IMO it's required for industrial strength solution.
How many times have you needed internationalization support logging?
Me, personally - never. But may be the fact that I work in domestic department of American company could be accounted for that. But for the boost level library this is a showstopper. Internationalization is not an afterthought.
6. Misfeature: pre-init cashing Library starts with cashing turned on to support pre-init logging. IMO this is too dangerous. Now developer needs to remember how many log entries are printed. Otherwise some of them will just start disappearing (could be different in every run). And it may start happening only at the user site. Much more reliable solution is to use some kind of startup log that does not require configuration.
And where would that log write to? What if it's very important data? To console? What if you don't have console?
I think I covered that in my last statement.
And lets face it -- when one starts using a logging lib, will at least trace one call to the debug lib. If he sees the line doesn't show up anywhere, he'll most likely read the docs. If you take a look at the
I don't see how reading the docs will help. Solution is dangerous in it's nature.
Basic Usage, you'll see the 'flush_log_cache()' call.
Besides, even if you don't call it and you write 256 messages to a log, the caching will automatically turn itself off and flush. The same
And what if log is not configured yet? What will happened? We will loose all those 256 messages or program will crash?
happens if the application ends, and caching was still on.
7. Misfeature: log hierarchies I personally never have a need for that and have some difficulties to imagine why would anyone have. You could probably have it on top of existent solution as add-on. If it doesn't require any new interfaces (or some
Have you heard of modules, and sub-modules? What if each module/sub-module was to have a log to write to?
I primarily use one file for everything. Why would I want 10 different files? So to figure out what happened I will need to jump through 10 different output comparing timestamps? (*if* they are printed). I do *rarely* use more than additional logs for information dedicated to different target audience (operation departments, end users). But this is not driven by module/submodule hierarchy. To mark the modules I use much better idiom - keyword (all within bounds of same log).
8. Misfeature: compile-time log support As it clear from above IMO log is (almost) never always enabled (and in any case there is a way to implement this without any additional support). And always disabled log could be implemented on macro level. Based on this I
Please enlighten me: how can you implement a disabled log at macro level?
Numerous ways. Here is from top of my head: struct nil_stream {} template<typename T> nil_stream const& operator<<( nil_stream const& ns, T const&) { return ns; } #define LOG( .... ) if(true) {} else nil_stream() << In my practice though I never do that. I found that single if( log_is_on ) is good enough even in most performance tight places.
believe all the compile_tile machinery employed by library is unnecessary and just complicate implementation
It was asked by users of the lib. I never use it, but they need it.
Well it's just wrong solution then.
9. Interface IMO primary interface advertised by library should be macro-less.
Please provide such an implementation, so that the following works:
some_log << some_lengthy_function();
Which, if some_log is disabled, some_length_function() is not called -- without the use of macros.
I never said Interface should look like above. But users need to be able to write these helper macros themselves based on known public API.
Documentation does need to cover how these macros needs to be written, but with user defined filters user will need to do the job oneself. Also in my experience I found that on some systems Even if( ... ) <actual log here> could be unsatisfactory from performance standpoint. I employ one global Boolean is_log_on flag to guard all my log entries. and it should be a part of these macros
As I recall, noone else asked for this -- but it's not hard to implement.
There is nothing to implement here (for library author). Library just need to advertise macroless interface and users will write macros.
II. Implementation flaws ------------------
2. Multithreading support implementation a) For some unclear reason author chose to reimplement some of Boost.Threads functionality in some places (and in some places Boost.Thread is used always). I believe log library shouldn't do this.
It was requested -- some users wanted to remove dependency on Boost.Thread.
Well it still wrong. You couldn't justify that by user's request. BTW you do depend on Boost.Threads still.
b) I noticed some fishy construct (m_destroyed data member). It opens some thread safety issues IMO.
It's used for the global logs (the ones defined with BOOST_DEFINE_LOG), which return a static reference to a boost::logging::logger - to know if the log is used after its destruction. I'm looking for better alternatives.
c) static manager_type & manager() doesn't look thread safe.
It's there only for function overloading -- the returned reference is never used
I am not an expert. But it's still fishy.
3 . enabled_logger::m_stream For some reason enabled_logger allocates m_stream dynamically for each log entry. I do not see why we need to do this. Even more: why not reuse a single instance per thread?
Yes, in the future, I can re-implement it as a single instance per thread.
Well in a future we could reconsider this proposal.
4. collection in use appender_array and modifier_array are better be implemented using lists
Have you done some tests, or is this just a gut feeling?
I did not provide any tests, did you? And it's not a gut feeling. In places where it's important you never need random access.
5. exception support
try { LOG << "aaa" << foo() } catch(...) { LOG << "exception" }
This construct doesn't seems generate what is expected if foo() throws an exception
What is expected?
Try to run this and you will see.
5. appender_array appender_array seems to be defined in 2 placed
Yes, once in the log_impl's class definition, and once in namespace boost { namespace logging { namespace {
Why would you give the same name to 2 different things?
6. Appenders copying.
From code it seems that each logger has its own copy of each appender. Why would we need that?
Because if we had one appender for all loggers, we could run into thread-safety issues.
But I may have different logger for every log entry. It's unacceptable design. You need to lock on different level.
If we do why all the appenders are so heavy?
I don't understand what you mean.
So costly to copy.
8. shared_memory.hpp This header refer to the library that is not part of boost yet. I don't think it should be part of this submission.
Why not? First of all, you don't need to include it, if you don't use it.
Because it brings a wrong message to boost users. you refer to boost/shmem/... and it's not there.
9. Time string cashing prepend_time and prepend_time_strf calls are not cashed. It's a lot of work for nothing.
Please explain.
You may have 1000 messages per second. And you format time string for every single one.
10. write_msg locking write_msg shouldn't lock on appenders writing. Each appender should be locked independently instead.
It's because appenders/modifiers could be added/deleted while someone is writing a message to the log. Thus, it needs to be thread-safe.
You need to find an alternative solution. Every output has to be locked independently. This is one of the "premature pessimization" you force on the user.
Best, John
Regards, Gennadiy
4. Internationalization support I do see some vague traces in code that indicate that author new this needs to be supported. But since documentation have exactly zero information and I couldn't figure out how to do it myself, I assume that library doesn't support this. IMO it's required for industrial strength solution.
How many times have you needed internationalization support logging?
Me, personally - never. But may be the fact that I work in domestic department of American company could be accounted for that. But for the boost level library this is a showstopper. Internationalization is not an afterthought.
"Internationalization" is being used as a buzzword here: can you define what you expect to be possible, and then John can say if it is possible or not. All I can think to expect is that if I have a unicode string (or a string in any encoding) that it would get written to the log without corruption. Similarly a log path/filename containing non-ascii characters should work. (I agree that if either of those is not possible that is a show-stopper.) Darren
"Internationalization" is being used as a buzzword here: can you define what you expect to be possible, and then John can say if it is possible or not.
All I can think to expect is that if I have a unicode string (or a string in any encoding) that it would get written to the log without corruption. Similarly a log path/filename containing non-ascii characters should work. (I agree that if either of those is not possible that is a show-stopper.)
From what I gather no char/string/ostream type customization is currently possible in log itself. As for the filename. I think filesystem library deals with it.
Gennadiy
"Internationalization" is being used as a buzzword here: can you define what you expect to be possible, and then John can say if it is possible or not.
All I can think to expect is that if I have a unicode string (or a
Yes, the lib allows for Unicode. Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!
"John Torjo" <john.lists@torjo.com> wrote in message news:4378A391.8020300@torjo.com...
"Internationalization" is being used as a buzzword here: can you define what you expect to be possible, and then John can say if it is possible or not.
All I can think to expect is that if I have a unicode string (or a
Yes, the lib allows for Unicode.
Is it covered anywhere in docs? Would you care to show how this could be done? Gennadiy
Gennadiy Rozental wrote:
"John Torjo" <john.lists@torjo.com> wrote in message news:4378A391.8020300@torjo.com...
"Internationalization" is being used as a buzzword here: can you define what you expect to be possible, and then John can say if it is possible or not.
All I can think to expect is that if I have a unicode string (or a
Yes, the lib allows for Unicode.
Is it covered anywhere in docs? Would you care to show how this could be done?
Ok, my bad. To deal with Unicode, simply define the UNICODE macro. Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!
Yes, the lib allows for Unicode.
Is it covered anywhere in docs? Would you care to show how this could be done?
Ok, my bad. To deal with Unicode, simply define the UNICODE macro.
First of all any macro based solution wouldn't be acceptable. Second "simply define the UNICODE macro" doesn't do a bit. Did you see your own code? The whole design is never thought through in regards to custom character/string type. Here is extract from fwd header: (#ifndef BOOST_LOG_VC ignored) struct default_log_manager { typedef char char_t; typedef std::string string; typedef std::ostringstream stream; }; That's it: no ifdef UNICODE whatsoever (and again it wouldn't be acceptable anyway) Gennadiy
Gennadiy Rozental wrote:
Yes, the lib allows for Unicode.
Is it covered anywhere in docs? Would you care to show how this could be done?
Ok, my bad. To deal with Unicode, simply define the UNICODE macro.
First of all any macro based solution wouldn't be acceptable. Second "simply define the UNICODE macro" doesn't do a bit. Did you see your own code? The whole design is never thought through in regards to custom character/string type. Here is extract from fwd header:
(#ifndef BOOST_LOG_VC ignored)
struct default_log_manager {
typedef char char_t; typedef std::string string; typedef std::ostringstream stream; };
That's it: no ifdef UNICODE whatsoever (and again it wouldn't be acceptable anyway)
I've updated the lib, due to feedback from Paul Bristow. Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!
Yes, the lib allows for Unicode.
Is it covered anywhere in docs? Would you care to show how this could be done?
Ok, my bad. To deal with Unicode, simply define the UNICODE macro.
First of all any macro based solution wouldn't be acceptable. Second "simply define the UNICODE macro" doesn't do a bit. Did you see your own code? The whole design is never thought through in regards to custom character/string type. Here is extract from fwd header:
(#ifndef BOOST_LOG_VC ignored)
struct default_log_manager {
typedef char char_t; typedef std::string string; typedef std::ostringstream stream; };
That's it: no ifdef UNICODE whatsoever (and again it wouldn't be acceptable anyway)
I've updated the lib, due to feedback from Paul Bristow.
I looked on the original version of the library submitted for review. Even UNICODE isn't referenced anywhere. So we could safely say that submitted library doesn't support char/string customization required for Internationalization. That was point in my initial post. Gennadiy P.S. I personally think you are looking in wrong direction in attempts to support custom char and threading. You couldn't produce proper solution without Policy based design: LookingPolicy and EncodingPolicy could be used.
I've updated the lib, due to feedback from Paul Bristow.
I looked on the original version of the library submitted for review. Even UNICODE isn't referenced anywhere. So we could safely say that submitted library doesn't support char/string customization required for Internationalization. That was point in my initial post.
Ok, point taken.
P.S. I personally think you are looking in wrong direction in attempts to support custom char and threading. You couldn't produce proper solution without Policy based design: LookingPolicy and EncodingPolicy could be used.
I guess I need to think about it, but I do see your point. Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!
Gennadiy Rozental wrote:
BTW before I start, I recalled couple more issues I forgot to mention in original post:
1. Design: Lightweight interface Log library brings a lot of dependencies. In some case I do not want this if
Care to exemplify? What are the dependencies?
I need to use the library in header. IMO log needs to support/implement some lightweight debug abstract interface Something like:
struct DebugLog { virtual void log( string const& ) = 0; virtual void log( int ) = 0; };
struct Log : DebugLog { ... };
Now I could use DebugLog interface without need to know an implementation. It comes with the price of virtual call. I primarily use this for debug only logging.
Anybody else wants this?
2. Design: Enter/Exit function scope support I believe it's required for log library to provide some kind of support for scope tracking. It could be an addon to core functionality to it needs to be configurable.
Added to my TODO list.
3. Design: hexdump I believe log library needs to provide support for dumping memory in hex format on log file. It could be an addon to core functionality.
I can add this, even though I'm not sure why you'd want this.
4. Design: logged assert I believe log library needs to provide support for custom assert. Such assert would log into log file and invoke system assert depends in assert level. It could be an addon to core functionality.
Well, I want to adapt SMART_ASSERT to allow for this functionality.
5. Implementation/Design: log entry data copying. Modifiers implemented by applying modifier for string that represent entry data. This model is inefficient in following regards: a) you need to make a copy of original entry data for every output. b) you need to prepend the strings in most cases - which is inefficient operation. c) you couldn't share the work for different outputs - you need to repeat it for every string
IMO better model would be to apply modifiers to outputs instead.
Yes, I will consider this. IMO, is better to have a working model, which can be optimized later.
Actually since macros are not primary interface in different subsystems log may look different. Couple examples:
1. LOG( INFO, PROG_FLOW, "SocketLib" ) << "Connection esteblished to: " << address; 2. LOG( DEBUG, RETURN_VALUE ) << "Fetched value: " << v; Here a LOG macro refer to some keyword by name. Usually I have keyword per file, per library or per class. 3. DEBUG_LOG( DATA_FLOW ) << "Input msg: " < msg; Here DEBUG_LOG automatically apply DEBUG level and refer to keyword by name
This IMO is too much to type, and I personally woudn't use it in real-life apps.
Also multithreaded application should support filtering based on thread id.
And what's stopping you from making an appender that does just this?
// trivial example struct only_for_thread_id { only_for_thread_id(int tid, appender_func forward_to) : tid(tid), forward_to(forward_to) {} void operator()(const std::string&,const std::string & msg) { if ( ::GetCurrentThreadId() == tid) forward_to(msg); } int tid; appender_func forward_to; }
You must be kidding, right? I want filtering on log level. So that entry appear or not appear in all outputs.
Please read what you just wrote a couple of lines above: "filtering based on thread id."
2. Configuration support Submitted solution supports configuration in a form of arbitrary modifiers that user could register in log. While this looks like very inflexible it's also as very inconvenient. In most cases I prefer log to be configured at run time from within configuration file. his would be very cumbersome with modifiers model. Not only each modifier needs to be added separately. I also
Did you take a look at Darryl's proposal? http://torjo.com/code/lib/alternate_manipulating.html)
And how that has anything to do with what I am talking about? It's just a twist of the same unacceptable interface.
Unacceptable to you, yes.
need to keep track of it's name to be able to remove it and it's index to know how modifiers are ordered. This ability to configure log remotely in many locations makes it impossible to really know what and how is written into log. My preferred way to configure the log is with use of configuration string. First of all it's simplify an interface and you immediately see how your log entries going to look like. Here is an example: "keyword=*,-ACD;categ=prog_flow,return value,-details;l=debug;track=on;roll=10000;prefix=file ( line ) - time :;timeformat=%s:%m".
You can implement Configuration Support on top of the current library.
I do not see how it possible and why should I do this on top of useless API.
Funny, but I think your last statement is quite useless.
And besides, if you want to configure logging, you'll still need to identify each log, appender and manipulator by a string.
Why is that? I do not want or need to know log name to configure it. I have an instance.
Because when you have some setting in the configuration file, you need whom it refers to. Therefore, you need to have some log-to-string correspondence.
4. Internationalization support I do see some vague traces in code that indicate that author new this needs to be supported. But since documentation have exactly zero information and I couldn't figure out how to do it myself, I assume that library doesn't support this. IMO it's required for industrial strength solution.
How many times have you needed internationalization support logging?
Me, personally - never. But may be the fact that I work in domestic department of American company could be accounted for that. But for the boost level library this is a showstopper. Internationalization is not an afterthought.
Still what do you understand by internationalization? It can deal with Unicode, if this is what you want...
6. Misfeature: pre-init cashing Library starts with cashing turned on to support pre-init logging. IMO this is too dangerous. Now developer needs to remember how many log entries are printed. Otherwise some of them will just start disappearing (could be different in every run). And it may start happening only at the user site. Much more reliable solution is to use some kind of startup log that does not require configuration.
And where would that log write to? What if it's very important data? To console? What if you don't have console?
I think I covered that in my last statement.
Well, I don't think it's covered.
And lets face it -- when one starts using a logging lib, will at least trace one call to the debug lib. If he sees the line doesn't show up anywhere, he'll most likely read the docs. If you take a look at the
I don't see how reading the docs will help. Solution is dangerous in it's nature.
When you'll come up with something better, I'll use it. Until then...
Basic Usage, you'll see the 'flush_log_cache()' call.
Besides, even if you don't call it and you write 256 messages to a log, the caching will automatically turn itself off and flush. The same
And what if log is not configured yet? What will happened? We will loose all those 256 messages or program will crash?
Ok, I have not handled this case yet. I need to handle it -- have a "safe log", where I will write if program exists with cache still turned on, and logs have not been initialized yet.
happens if the application ends, and caching was still on.
7. Misfeature: log hierarchies I personally never have a need for that and have some difficulties to imagine why would anyone have. You could probably have it on top of existent solution as add-on. If it doesn't require any new interfaces (or some
Have you heard of modules, and sub-modules? What if each module/sub-module was to have a log to write to?
I primarily use one file for everything. Why would I want 10 different files? So to figure out what happened I will need to jump through 10
You can have multiple logs that output to the same file.
different output comparing timestamps? (*if* they are printed). I do *rarely* use more than additional logs for information dedicated to different target audience (operation departments, end users). But this is not driven by module/submodule hierarchy. To mark the modules I use much better idiom - keyword (all within bounds of same log).
I'm not even gonna ask why yours is such a better idiom...
8. Misfeature: compile-time log support As it clear from above IMO log is (almost) never always enabled (and in any case there is a way to implement this without any additional support). And always disabled log could be implemented on macro level. Based on this I
Please enlighten me: how can you implement a disabled log at macro level?
Numerous ways. Here is from top of my head:
struct nil_stream {}
template<typename T> nil_stream const& operator<<( nil_stream const& ns, T const&) { return ns; }
#define LOG( .... ) if(true) {} else nil_stream() <<
Amazing... You can turn on/off ***all*** logging... Wow! And to think that BOOST_LOG allows you to turn on/off certain logs -- allowing for some information to be logged while what's not important is turned off... Your solution is indeed much better... Not to say that if you write something like: LOG << some_lengthy_function(); some_lengthy_function() will still be executed, even though logging is disabled...
II. Implementation flaws ------------------
2. Multithreading support implementation a) For some unclear reason author chose to reimplement some of Boost.Threads functionality in some places (and in some places Boost.Thread is used always). I believe log library shouldn't do this.
It was requested -- some users wanted to remove dependency on Boost.Thread.
Well it still wrong. You couldn't justify that by user's request. BTW you do depend on Boost.Threads still.
??? If you use ts_appender(), yes. Otherwise, not.
4. collection in use appender_array and modifier_array are better be implemented using lists
Have you done some tests, or is this just a gut feeling?
I did not provide any tests, did you? And it's not a gut feeling. In places where it's important you never need random access.
It's not about random access. It's about so many tests have shown that std::vector is faster than std::list, especially when the vectors are small - in the case of logger_impl.
5. exception support
try { LOG << "aaa" << foo() } catch(...) { LOG << "exception" }
This construct doesn't seems generate what is expected if foo() throws an exception
What is expected?
Try to run this and you will see.
It tries to print as much as possible from the original "LOG << "aaa" << foo()" expression, and then it prints "exception". What do you expect?
5. appender_array appender_array seems to be defined in 2 placed
Yes, once in the log_impl's class definition, and once in namespace boost { namespace logging { namespace {
Why would you give the same name to 2 different things?
It's about context. Why not?
If we do why all the appenders are so heavy?
I don't understand what you mean.
So costly to copy.
How would you know?
9. Time string cashing prepend_time and prepend_time_strf calls are not cashed. It's a lot of work for nothing.
Please explain.
You may have 1000 messages per second. And you format time string for every single one.
Ok, you're right. I can improve on that.
10. write_msg locking write_msg shouldn't lock on appenders writing. Each appender should be locked independently instead.
It's because appenders/modifiers could be added/deleted while someone is writing a message to the log. Thus, it needs to be thread-safe.
You need to find an alternative solution. Every output has to be locked independently. This is one of the "premature pessimization" you force on the user.
I'll think about it. Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!
1. Design: Lightweight interface Log library brings a lot of dependencies. In some case I do not want this if
Care to exemplify? What are the dependencies?
<iostream> <sstream> <vector> <fstream> <cstdlib> <ctime> <boost/function.hpp> .... more I may need to use logging in some headers I prefer to keep lightweight. In this case lightweight interface is used.
5. Implementation/Design: log entry data copying. Modifiers implemented by applying modifier for string that represent entry data. This model is inefficient in following regards: a) you need to make a copy of original entry data for every output. b) you need to prepend the strings in most cases - which is inefficient operation. c) you couldn't share the work for different outputs - you need to repeat it for every string
IMO better model would be to apply modifiers to outputs instead.
Yes, I will consider this. IMO, is better to have a working model, which can be optimized later.
You couldn't change an API post review and introduce completely new modifiers model. It's a different library that needs different review
Actually since macros are not primary interface in different subsystems log may look different. Couple examples:
1. LOG( INFO, PROG_FLOW, "SocketLib" ) << "Connection esteblished to: " << address; 2. LOG( DEBUG, RETURN_VALUE ) << "Fetched value: " << v; Here a LOG macro refer to some keyword by name. Usually I have keyword per file, per library or per class. 3. DEBUG_LOG( DATA_FLOW ) << "Input msg: " < msg; Here DEBUG_LOG automatically apply DEBUG level and refer to keyword by name
This IMO is too much to type, and I personally wouldn't use it in real-life apps.
How DLOG( DATA_FLOW) is more to type then BOOST_LOG( DEBUG )? Depends on what you prefer one may select more or less verbose statement. But most importantly: in my view Filters should be configurable. IOW you just need log level - it's your choice. But if I need more filters I should be able to specify them.
Also multithreaded application should support filtering based on thread id.
And what's stopping you from making an appender that does just this?
// trivial example struct only_for_thread_id { only_for_thread_id(int tid, appender_func forward_to) : tid(tid), forward_to(forward_to) {} void operator()(const std::string&,const std::string & msg) { if ( ::GetCurrentThreadId() == tid) forward_to(msg); } int tid; appender_func forward_to; }
You must be kidding, right? I want filtering on log level. So that entry appear or not appear in all outputs.
Please read what you just wrote a couple of lines above: "filtering based on thread id."
Sorry typo. I want filtering on thread id. So that entry appear or not appear in all outputs based on which is current thread id.
need to keep track of it's name to be able to remove it and it's index to know how modifiers are ordered. This ability to configure log remotely in many locations makes it impossible to really know what and how is written into log. My preferred way to configure the log is with use of configuration string. First of all it's simplify an interface and you immediately see how your log entries going to look like. Here is an example: "keyword=*,-ACD;categ=prog_flow,return value,-details;l=debug;track=on;roll=10000;prefix=file ( line ) - time :;timeformat=%s:%m".
You can implement Configuration Support on top of the current library.
I do not see how it possible and why should I do this on top of useless API.
Funny, but I think your last statement is quite useless.
Ok Useless is wrong word. I meant to say that I would never use it since single configure string is so much more clear and convenient.
And besides, if you want to configure logging, you'll still need to identify each log, appender and manipulator by a string.
Why is that? I do not want or need to know log name to configure it. I have an instance.
Because when you have some setting in the configuration file, you need whom it refers to. Therefore, you need to have some log-to-string correspondence.
I don't see how log name comes into play here (it could but only if end user will choose to). I may have single log that doesn't care about the name at all. Or I may have 2 different named config parameters: config.get( "system_log_config" ) config.get( "admin_log_config" ) But this strings has nothing to do with log names either.
Much more reliable solution is to use some kind of startup log that does not require configuration.
And where would that log write to? What if it's very important data? To console? What if you don't have console?
I think I covered that in my last statement.
Well, I don't think it's covered.
Let me repeat: Much more reliable solution is to use some kind of startup log. IOW we use dedicated log that only used during startup.
And lets face it -- when one starts using a logging lib, will at least trace one call to the debug lib. If he sees the line doesn't show up anywhere, he'll most likely read the docs. If you take a look at the
I don't see how reading the docs will help. Solution is dangerous in it's nature.
When you'll come up with something better, I'll use it. Until then...
I think I did.
Basic Usage, you'll see the 'flush_log_cache()' call.
Besides, even if you don't call it and you write 256 messages to a log, the caching will automatically turn itself off and flush. The same
And what if log is not configured yet? What will happened? We will loose all those 256 messages or program will crash?
Ok, I have not handled this case yet. I need to handle it -- have a
This is my point.
7. Misfeature: log hierarchies I personally never have a need for that and have some difficulties to imagine why would anyone have. You could probably have it on top of existent solution as add-on. If it doesn't require any new interfaces (or some
Have you heard of modules, and sub-modules? What if each module/sub-module was to have a log to write to?
I primarily use one file for everything. Why would I want 10 different files? So to figure out what happened I will need to jump through 10
You can have multiple logs that output to the same file.
And how about thread safety: Don't you lock on log level? And what if I want to filter out specific subsystem? Do I need to register/unregister outputs? But this will still cause log statement to be executed. Why would I do that instead of common filtering mechanism with multiple filters?
different output comparing timestamps? (*if* they are printed). I do *rarely* use more than additional logs for information dedicated to different target audience (operation departments, end users). But this is not driven by module/submodule hierarchy. To mark the modules I use much better idiom - keyword (all within bounds of same log).
I'm not even gonna ask why yours is such a better idiom...
Because I have single idiom "filters", why you propose numerous different ways to do this task. And filters are less prone to be misused.
Please enlighten me: how can you implement a disabled log at macro level?
Numerous ways. Here is from top of my head:
struct nil_stream {}
template<typename T> nil_stream const& operator<<( nil_stream const& ns, T const&) { return ns; }
#define LOG( .... ) if(true) {} else nil_stream() <<
Amazing... You can turn on/off ***all*** logging... Wow!
Why all? I could use different macro with different logs? Macro are not primary interface. And as I told you in practice there is no need to do this at all.
And to think that BOOST_LOG allows you to turn on/off certain logs -- allowing for some information to be logged while what's not important is turned off... Your solution is indeed much better...
This is still the case for me. Even more that.
Not to say that if you write something like:
LOG << some_lengthy_function();
some_lengthy_function() will still be executed, even though logging is disabled...
How did you case to such conclusion? Since when false branch gets executed?
II. Implementation flaws ------------------
2. Multithreading support implementation a) For some unclear reason author chose to reimplement some of Boost.Threads functionality in some places (and in some places Boost.Thread is used always). I believe log library shouldn't do this.
It was requested -- some users wanted to remove dependency on Boost.Thread.
Well it still wrong. You couldn't justify that by user's request. BTW you do depend on Boost.Threads still.
??? If you use ts_appender(), yes. Otherwise, not.
So you do. You couldn't have it both ways.
4. collection in use appender_array and modifier_array are better be implemented using lists
Have you done some tests, or is this just a gut feeling?
I did not provide any tests, did you? And it's not a gut feeling. In places where it's important you never need random access.
It's not about random access. It's about so many tests have shown that std::vector is faster than std::list, especially when the vectors are small - in the case of logger_impl.
Again: I do not see a single test case. And unless you are using random access I believe you statement is wrong. Why would we have lists at all? you primarily use this collections for forward traversing and inserting removing operations. I would assume list would behave better.
5. exception support
try { LOG << "aaa" << foo() } catch(...) { LOG << "exception" }
This construct doesn't seems generate what is expected if foo() throws an exception
What is expected?
Try to run this and you will see.
It tries to print as much as possible from the original "LOG << "aaa" << foo()" expression, and then it prints "exception".
What do you expect?
First of all in my test (VC7.1) it did not print aaa and it printed second statement on the same line [aaa] ... [aaa] exception
If we do why all the appenders are so heavy?
I don't understand what you mean.
So costly to copy.
How would you know?
Just do sizeof(logger). Keep in mind that your design assumes that may copy logger for *every* log entry. Gennadiy
On 11/14/05, Gennadiy Rozental <gennadiy.rozental@thomson.com> wrote:
This IMO is too much to type, and I personally wouldn't use it in real-life apps.
How DLOG( DATA_FLOW) is more to type then BOOST_LOG( DEBUG )? Depends on what you prefer one may select more or less verbose statement. But most importantly: in my view Filters should be configurable. IOW you just need log level - it's your choice. But if I need more filters I should be able to specify them.
You can with BOOST_LOG_DEFINE_LEVEL. -- Caleb Epstein caleb dot epstein at gmail dot com
"Caleb Epstein" <caleb.epstein@gmail.com> wrote in message news:989aceac0511140855t6e8c35eif9ed57d04e488a76@mail.gmail.com...
On 11/14/05, Gennadiy Rozental <gennadiy.rozental@thomson.com> wrote:
This IMO is too much to type, and I personally wouldn't use it in real-life apps.
How DLOG( DATA_FLOW) is more to type then BOOST_LOG( DEBUG )? Depends on what you prefer one may select more or less verbose statement. But most importantly: in my view Filters should be configurable. IOW you just need log level - it's your choice. But if I need more filters I should be able to specify them.
You can with BOOST_LOG_DEFINE_LEVEL.
No. I need completely different dimension. not a different log level. See my review: category, keyword, thread id, time of the day etc. Gennadiy
Gennadiy Rozental wrote:
1. Design: Lightweight interface Log library brings a lot of dependencies. In some case I do not want this if
Care to exemplify? What are the dependencies?
<iostream>
Had you taken the time to look, this is included only in the source files.
<sstream>
Needed for buffering the messages. I've measured the effects of including or not including this, and at least for VC7.1, including <sstream> is compile-time wise virtually same as not including it. In fact, the only headers I include, in log.hpp, are <sstream> and <string>. So, I did minimize the dependencies.
<vector> <fstream> <cstdlib> <ctime> <boost/function.hpp> ....
They're only used when you need to manipulate the logs (add appenders/modifiers, set the level). This will most likely happen in very few places, so you pay compile-wise only when you do advanced stuff.
IMO better model would be to apply modifiers to outputs instead.
Yes, I will consider this. IMO, is better to have a working model, which can be optimized later.
You couldn't change an API post review and introduce completely new modifiers model. It's a different library that needs different review
I was only talking about changing (optimizing) the implementation.
Actually since macros are not primary interface in different subsystems log may look different. Couple examples:
1. LOG( INFO, PROG_FLOW, "SocketLib" ) << "Connection esteblished to: " << address; 2. LOG( DEBUG, RETURN_VALUE ) << "Fetched value: " << v; Here a LOG macro refer to some keyword by name. Usually I have keyword per file, per library or per class. 3. DEBUG_LOG( DATA_FLOW ) << "Input msg: " < msg; Here DEBUG_LOG automatically apply DEBUG level and refer to keyword by name
This IMO is too much to type, and I personally wouldn't use it in real-life apps.
How DLOG( DATA_FLOW) is more to type then BOOST_LOG( DEBUG )? Depends on
First of all, you'd have to prefix it with BOOST_: So lets see: BOOST_LOG(DEBUG) BOOST_DLOG(DATA_FLOW) BOOST_LOG(DEBUG, RETURN_VALUE) BOOST_LOG(INFO, PROG_FLOW, "SocketLib")
// trivial example struct only_for_thread_id { only_for_thread_id(int tid, appender_func forward_to) : tid(tid), forward_to(forward_to) {} void operator()(const std::string&,const std::string & msg) { if ( ::GetCurrentThreadId() == tid) forward_to(msg); } int tid; appender_func forward_to;
Sorry typo. I want filtering on thread id. So that entry appear or not appear in all outputs based on which is current thread id.
Using the only_for_thread_id, combined with other appenders (for instance, appender_array), you can accomplish that: manipulate_logs("*").add_appender( only_for_thread_id(1552,appender_array() .add(write_to_cout) .add(write_to_file("out.txt") ) .add(write_to_dbg_wnd) ) );
your log entries going to look like. Here is an example: "keyword=*,-ACD;categ=prog_flow,return value,-details;l=debug;track=on;roll=10000;prefix=file ( line ) - time :;timeformat=%s:%m".
You can implement Configuration Support on top of the current library.
I do not see how it possible and why should I do this on top of useless API.
Funny, but I think your last statement is quite useless.
Ok Useless is wrong word. I meant to say that I would never use it since single configure string is so much more clear and convenient.
Yes, it is. But as you app grows bigger and bigger, I think that single string could get pretty complex. Basically, on top of the lib, you can build your Configuration Support, or what I envisioned -- using multiple lines. It's a matter of choice.
I don't see how log name comes into play here (it could but only if end user will choose to). I may have single log that doesn't care about the name at all. Or I may have 2 different named config parameters:
config.get( "system_log_config" ) config.get( "admin_log_config" )
But this strings has nothing to do with log names either.
How do you set the log's settings, when you read them from the Configuration file?
Much more reliable solution is to use some kind of startup log that does not require configuration.
And where would that log write to? What if it's very important data? To console? What if you don't have console?
I think I covered that in my last statement.
Well, I don't think it's covered.
Let me repeat: Much more reliable solution is to use some kind of startup log. IOW we use dedicated log that only used during startup.
Well, I will allow for a startup log to be used in case the application ends, and the logs have not been initialized yet.
7. Misfeature: log hierarchies I personally never have a need for that and have some difficulties to imagine why would anyone have. You could probably have it on top of existent solution as add-on. If it doesn't require any new interfaces (or some
Have you heard of modules, and sub-modules? What if each module/sub-module was to have a log to write to?
I primarily use one file for everything. Why would I want 10 different files? So to figure out what happened I will need to jump through 10
You can have multiple logs that output to the same file.
And how about thread safety: Don't you lock on log level?
Ok, you're right. I guess it's better to have locking on the modifier/appenders.
And what if I want to filter out specific subsystem? Do I need to register/unregister outputs? But this will still cause log statement to be executed.
If you want to filter out a specific subsystem, you will disable the logs corresponding to that subsystem. When a message is about to be written to the log, if the log is disabled, the whole message is ignored. // assume x is disabled BOOST_LOG(x) << even << if << this is costly << it will not be executed;
Why would I do that instead of common filtering mechanism with multiple filters?
Efficiency, for starters.
Because I have single idiom "filters", why you propose numerous different ways to do this task. And filters are less prone to be misused.
What you call filters, can in my case be logs, and again, you can enable/disable them in an efficient manner.
Please enlighten me: how can you implement a disabled log at macro level?
Numerous ways. Here is from top of my head:
struct nil_stream {}
template<typename T> nil_stream const& operator<<( nil_stream const& ns, T const&) { return ns; }
#define LOG( .... ) if(true) {} else nil_stream() <<
Amazing...
LOG << some_lengthy_function();
some_lengthy_function() will still be executed, even though logging is disabled...
How did you case to such conclusion? Since when false branch gets executed?
When the false branch gets executed, it will look like: nil_stream() << some_lengthy_function(); While operator<< can be considered a nop, compilers will not necessary optimize away the some_lengthy_function() call.
5. exception support
try { LOG << "aaa" << foo() } catch(...) { LOG << "exception" }
This construct doesn't seems generate what is expected if foo() throws an exception
What is expected?
Try to run this and you will see.
It tries to print as much as possible from the original "LOG << "aaa" << foo()" expression, and then it prints "exception".
What do you expect?
First of all in my test (VC7.1) it did not print aaa and it printed second statement on the same line
The second statement was printed on the same line, because you did not add the append_enter to the appenders for LOG. Finally, I'd really love to see a construct that will work in the way you expect. Because, as you probably know, foo() is executed first, and only afterwards is "LOG << "aaa" << results_of_calling_foo" executed.
If we do why all the appenders are so heavy?
I don't understand what you mean.
So costly to copy.
How would you know?
Just do sizeof(logger). Keep in mind that your design assumes that may copy logger for *every* log entry.
sizeof(logger) is 12 And BOOST_LOG uses a reference to a logger, so the logger is never copied. Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!
<sstream>
Needed for buffering the messages. I've measured the effects of including or not including this, and at least for VC7.1, including <sstream> is compile-time wise virtually same as not including it.
In fact, the only headers I include, in log.hpp, are <sstream> and <string>.
Well , I guess it's compiler dependent. But still it would be nice to have.
IMO better model would be to apply modifiers to outputs instead.
Yes, I will consider this. IMO, is better to have a working model, which can be optimized later.
You couldn't change an API post review and introduce completely new modifiers model. It's a different library that needs different review
I was only talking about changing (optimizing) the implementation.
You couldn't really optimize this without changing modifiers model.
How DLOG( DATA_FLOW) is more to type then BOOST_LOG( DEBUG )? Depends on
First of all, you'd have to prefix it with BOOST_:
First of all I don't. I (as end user) could name my macros the way I prefer.
So lets see: BOOST_LOG(DEBUG)
BOOST_DLOG(DATA_FLOW) BOOST_LOG(DEBUG, RETURN_VALUE) BOOST_LOG(INFO, PROG_FLOW, "SocketLib")
So lets see: L_(DATA_FLOW) L_(DEBUG, RETURN_VALUE) L_(INFO, PROG_FLOW, "SocketLib") Note the last is used rarely this keyword usually shared.
Sorry typo. I want filtering on thread id. So that entry appear or not appear in all outputs based on which is current thread id.
Using the only_for_thread_id, combined with other appenders (for instance, appender_array), you can accomplish that:
manipulate_logs("*").add_appender( only_for_thread_id(1552,appender_array() .add(write_to_cout) .add(write_to_file("out.txt") ) .add(write_to_dbg_wnd) ) );
Whatever you do you couldn't mimic filter with use of appender. Your favorite example LOG << very_long_function(); hits you immediately. With above solution very_long_function() will be called. Filter is something that checked before log statement are invoked.
Ok Useless is wrong word. I meant to say that I would never use it since single configure string is so much more clear and convenient.
Yes, it is. But as you app grows bigger and bigger, I think that single string could get pretty complex.
Why? How my growing application affects my log configuration string?
Basically, on top of the lib, you can build your Configuration Support, or what I envisioned -- using multiple lines. It's a matter of choice.
Basically logs always externally configured. Always from within some king of configuration file using some kind of configuration parameters/strings. Why would you make user to jump through hoops to apply this configuration, when the most natural and convenient is to support this on library level? This is all beside the point that modifiers also unsafe since you need to keep track which one has which index and it will soon enough became maintenance nightmare.
I don't see how log name comes into play here (it could but only if end user will choose to). I may have single log that doesn't care about the name at all. Or I may have 2 different named config parameters:
config.get( "system_log_config" ) config.get( "admin_log_config" )
But this strings has nothing to do with log names either.
How do you set the log's settings, when you read them from the Configuration file?
sys_log.configure( config.get( "system_log_config" ) ); adm_log.configure( config.get( "admin_log_config" ) );
Let me repeat: Much more reliable solution is to use some kind of startup log. IOW we use dedicated log that only used during startup.
Well, I will allow for a startup log to be used in case the application ends, and the logs have not been initialized yet.
What is important is not what you allow, but what you prevent. Library needs to prevent misuse it advertise in "cashing" chapters of docs.
And what if I want to filter out specific subsystem? Do I need to register/unregister outputs? But this will still cause log statement to be executed.
If you want to filter out a specific subsystem, you will disable the logs corresponding to that subsystem.
When a message is about to be written to the log, if the log is disabled, the whole message is ignored.
Ok. I guess you could mimic keywords with use of named logs. But now you have 2 different concepts (named log and log level) to do essentially the same job: filter log entry. And these wouldn't help you a bit if you need to filter based on any other criteria: category, time of day, thread id etc. Why not just use single powerful concept?
// assume x is disabled BOOST_LOG(x) << even << if << this is costly << it will not be executed;
Why would I do that instead of common filtering mechanism with multiple filters?
Efficiency, for starters.
How efficiency has anything to do with filters I advocate? Or rather How the usage of filters would decrease performance? I easily could prove that without custom filter support your performance will suffer (like above in thread id example)
Please enlighten me: how can you implement a disabled log at macro level?
Numerous ways. Here is from top of my head:
struct nil_stream {}
template<typename T> nil_stream const& operator<<( nil_stream const& ns, T const&) { return ns; }
#define LOG( .... ) if(true) {} else nil_stream() <<
Amazing...
LOG << some_lengthy_function();
some_lengthy_function() will still be executed, even though logging is disabled...
How did you case to such conclusion? Since when false branch gets executed?
When the false branch gets executed, it will look like:
Since when false branch gets executed??
The second statement was printed on the same line, because you did not add the append_enter to the appenders for LOG.
Yeah. That's another annoying thing: I would really prefer '\n' to be added without extra efforts on my side.
Finally, I'd really love to see a construct that will work in the way you expect.
If we do why all the appenders are so heavy?
I don't understand what you mean.
So costly to copy.
How would you know?
Just do sizeof(logger). Keep in mind that your design assumes that may copy logger for *every* log entry.
sizeof(logger) is 12
From you code:
// note: each logger has its own copy of a write_to_file object. This needs to be shared, // so that different copies don't overwrite each-other I assumed Each logger has a copy of each appender. Is it right?
And BOOST_LOG uses a reference to a logger, so the logger is never copied.
What if I use scoped logger extensively? Gennadiy
You couldn't change an API post review and introduce completely new modifiers model. It's a different library that needs different review
I was only talking about changing (optimizing) the implementation.
You couldn't really optimize this without changing modifiers model.
Again, how would you know?
If you want to filter out a specific subsystem, you will disable the logs corresponding to that subsystem.
When a message is about to be written to the log, if the log is disabled, the whole message is ignored.
Ok. I guess you could mimic keywords with use of named logs. But now you have 2 different concepts (named log and log level) to do essentially the same job: filter log entry. And these wouldn't help you a bit if you need to filter based on any other criteria: category, time of day, thread id etc.
Ok, I finally get it. You're right about the filtering concept. It's more powerful that what I have right now. Best, John -- John Torjo, Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit! -- http://www.torjo.com/cb/ - Click, Build, Run!
participants (4)
-
Caleb Epstein -
Darren Cook -
Gennadiy Rozental -
John Torjo