Add support for customizing the prefix format

[santigl]

This PR proposes a way of adding support to customize the prefix format used by glog.

With it users can declare a callback (PrefixAttacherFunction(std::ostream&, const LogMessageInfo&)) that will receive the relevant log details for the line and a stream to write a corresponding prefix.

That callback is enabled by passing it to a new overloaded InitGoogleLogging(). (The callback cannot be changed once the instance is initialized.)

If no callback is specified, LogMessage will follow the same path as it does now and write the current hardcoded format.

For an example use case, see the unit tests in src/logging_custom_prefix_unittest.cc.

[santigl]

This currently fails the CI step that builds with C++98, which does not provide std::function. The changes in this PR could be made conditional on the value of CXX_STANDARD, only providing the new functionality for C++11 and later.

[RippeR37]

@santigl great work, I really wanted that functionality as well.

For the whole C++11 thing - why not just use a function pointer instead of std::function<>? Since this is a global state you don't really need any stateful callback here and wouldn't it be noticably faster? And it would be C++98 compatible as well.

Also, why using std::string for log-specific data like severity (instead of enum/index), filename (instead of const char*)? There can be a noticeable performance penalty because of this.

[santigl]

Thanks for your input, @RippeR37!

An std::function gives more flexibility, For example, if you are implementing a wrapper around glog for a logging facility in a library, you could easily customize the callable with information that you gathered at runtime right before calling InitGoogleLogging().

Having said that, I have not done any performance testing (I was hoping to get reviews on the overall idea first). But, if benchmarks show that it's faster, I'm happy to add support/make it a raw pointer across all implementations.

+1 on the const char pointers. Since that is how they are stored internally, that will reduce the overhead. And the reason why I didn't add the enum value for the severity is that I felt that it was exposing an implementation detail from glog's internals.

[RippeR37]

An std::function gives more flexibility, For example, if you are implementing a wrapper around glog for a logging facility in a library, you could easily customize the callable with information that you gathered at runtime right before calling InitGoogleLogging().

Yes, but as far as I know, glog is "static"/global anyway (i.e. you can't have two instances at the same tme) so you can do the same by having global state and a free function which reads that state (instead of using std::function<>) and - as a bonus - not depend on C++11 in any way (which would also be nice for pre-C++11 users of this library).

Having said that, I have not done any performance testing (I was hoping to get reviews on the overall idea first). But, if benchmarks show that it's faster, I'm happy to add support/make it a raw pointer across all implementations.

I did a quick benchmark of overhead of calling function via std::function versus function pointer. In the field it might be bigger or smaller difference (and might depend on compiler/stdlib/switches/...) but in some cases I've seen a difference of function pointers being up to 7x faster then std::function.

https://quick-bench.com/q/5VlPI3I0QPW3-RT_Ywvc8g3ZT_E

+1 on the const char pointers. Since that is how they are stored internally, that will reduce the overhead. And the reason why I didn't add the enum value for the severity is that I felt that it was exposing an implementation detail from glog's internals.

IMHO it would still be better to not waste cpu cycles on enum=>string conversion. Exposing level as an int (0=INFO, 1=WARNING, 2=ERROR, 3=FATAL, verbose(N)=-N) or enum (+ something for verbose levels, e.g. old plain enum with predefined values only for named levels but also allowing for unnamed levels like verbose ones?) would be much better, especially for systems which can log hundreds of MBs or even GBs of data. It would be very bad to lose performance due to using this functionality.

[santigl]

That is a quite a big difference, I'll be happy to change the callback parameter to a pointer.

For the log levels strings, glog already has an array with the human-readable representations. If we pass a pointer to them there wouldn't be an additional cost, since glog currently indexes into that array to generate the prefix.

[RippeR37]

Thanks for the change! Now it looks like it shouldn't have a noticeable performance regression and does introduce much better flexibility for the users. 👍

[santigl]

Thanks for the feedback, @RippeR37!

[ukai]

I'm not convinced to have a feature to change logging format.
better to file issue first to explain the needs?

[santigl]

Ok, @ukai. I opened #578.

[cheshirekow]

Personally, I would like to see a customizable log message format. Logging frameworks that I use in other languages can be customized. For an organization that creates software in multiple languages, it can be hugely beneficial (and perhaps a requirement) that messages are logged in a consistent way regardless of which language that software is written in.

[edbaunton]

@ukai There are many other logging libraries available in C++, all of which have the functionality to customise logging. This is a sorely missing feature of glog.

• log4cpp allows for customising the log message
• log4cplus allows specifying patterns for the log layout
• log4cxx allows for specifying a PatternLayout
• spdlog allows the user to customise the formatting
• plog allows for a number of formatters
• boost allows for log record formatting
• Panetheios allows the user to specify a custom back-end to specify the log format.
• BDE allows for customising the record format

Allowing the user to customise the format would make for a really portable, performant and feature rich logging library. What is the downside that concerns you with this feature?

[sergiud]

@ukai Unless you are strongly opposed to this feature, I will go forward and merge it once it's ready.

@santigl It would be nice to have an example how to setup a custom prefix, preferably somewhere in README.rst.

[santigl]

That'd be great! Thank you, @sergiud.

I will address those comments and document the feature in the README.

[Arfrever]

If I read this correctly, even with -DWITH_CUSTOM_PREFIX=ON, behavior is not changed by default, when new variant of InitGoogleLogging function is not used, so I wonder why this feature is disabled by default or why there is even cmake option for this feature...

[sergiud]

Since there are some reservations regarding this feature e.g. from @ukai, the compromise is to declare it experimental and allow future (potentially breaking) changes based on user comments. Meanwhile, users (and package maintainers) still have the ability to enable the option at anytime. Hence, I don't see any issue here.