Logging UTC time, not local time

[eao197]

I have found myself in a situation where I need to use UTC-time in log files. But it seems that standard pattern formatter in spdlog always use details::os::local_time for conversion from std::time_t to std::tm.

So now I have a choice:

• write my own formatter, but it will be half backed and ugly copy of mature implementation of spdlog::pattern_formatter class;
• copy and paste the whole implementation of spdlog::pattern_formatter and change just one line.

I don't like both of them. I think it is possible to slightly improve spdlog::pattern_formatter implementation:

1. Crate a basic implementation for pattern_formatters:

class pattern_formatter_base : public formatter
{
public:
    explicit pattern_formatter_base(const std::string& pattern);
    pattern_formatter_base(const pattern_formatter_base&) = delete;
    pattern_formatter_base& operator=(const pattern_formatter_base&) = delete;
    void format(details::log_msg& msg) =0;
protected:
    // This will have almost all of the current pattern_formatter::format implementation.
    void format(details::log_msg& msg, const std::tm & tm_time);
private:
    const std::string _pattern;
    std::vector<std::unique_ptr<details::flag_formatter>> _formatters;
    void handle_flag(char flag);
    void compile_pattern(const std::string& pattern);
};

template<typename tm_provider>
class pattern_formatter_template : public pattern_formatter_base
{
public :
  ...
  void format(details::log_msg& msg) override {
    format(msg, tm_provider::get_time(log_clock::to_time_t(msg.time));
  }
};

using pattern_formatter = pattern_formatter_template<local_time_provider>;

Where local_time_provider will be:

struct local_time_provider {
  static std::tm get_time(std::time_t t) { return details::os::local_time(t); }
};

And there also will be gm_time_provider:

struct gm_time_provider {
  static std::tm get_time(std::time_t t) { return details::os::gmtime(t); }
};

It makes possible to do something like:

using gmtime_pattern_formatter = spdlog::pattern_formatter_template<
  spdlog::gm_time_provider >;
spdlog::set_formatter(std::make_shared<gmtime_pattern_formatter>());

It you decide that this is an appropriate idea I could implement it in my spdlog fork and make pull request.

[gabime]

I could change the private members of pattern_formatter class to be protected and then you could just inherit it and override the format method (short method, so copy/paste is not so bad):

class gmt_formatter : public spdlog::pattern_formatter
{
public:
    explicit gmt_formatter(const std::string& pattern) : pattern_formatter(pattern) {}
    void format(spdlog::details::log_msg& msg) override
    {
        try
        {
            auto tm_time = spdlog::details::os::gmtime(spdlog::log_clock::to_time_t(msg.time));
            for (auto &f : _formatters)
            {
                f->format(msg, tm_time);
            }
            //write eol
            msg.formatted.write(spdlog::details::os::eol, spdlog::details::os::eol_size);
        }
        catch (const fmt::FormatError& e)
        {
            throw spdlog::spdlog_ex(fmt::format("formatting error while processing format string: {}", e.what()));
        }

    }
};

and then in main:

       ...
        spdlog::set_formatter(std::make_shared<gmt_formatter>("%+"));
       ...  

[eao197]

I don't think that making internals of pattern_formatter protected instead of private is a good idea. This creates a dependency of the implementation of pattern_formatter. If you change the way of holding nested formatters (by some compile-time magic tricks) in some future version it breaks all derived classes.

Another solution could be as follows:

class pattern_formatter : public formatter
{
public :
  ... // all the current public stuff
protected :
  virtual std::tm get_time(std::time_t t) {
    return details::os::local_time(t);
  }
private:
  ... // all the current private stuff

and then in pattern_formatter::format:

inline void spdlog::pattern_formatter::format(details::log_msg& msg)
{
    try
    {
        auto tm_time = get_time(msg.time));

So I can write:

class gmt_pattern_formatter : public spdlog::pattern_formatter
{
protected:
   std::tm get_time(std::time_t t) override {
      return spdlog::details::os::gmtime(t);
  }
...
};

But price will be another virtual call. It could be very small price in many cases I think.

[gabime]

I agree, but another virtual call in this critical path should not be taken lightly..

[eao197]

Because of that my first proposal is based on templates.

[gabime]

The template idea while might be right, requirs too many changes. I am not even sure it worth it for such small feature.. especially since this won't solve the big picture. There could be many customization needed by different users. and then what? more templates? if the gmt need becomes popular I prefer just to add a flag to the ctor and solve this simple problem in a simple way.

[eao197]

Ok. I see you point.

[cernobaiv]

It would have been a nice feature for myself too.

[ThePhD]

I would like to also say that I need time to be formatted in UTC as well.

I think a constructor flag would be very helpful for this.

[ThePhD]

I implemented what I believe to be a minimal-impact, simple change to handle local vs. UTC time (with potential for future expansion without undue implementator burden). Please let me know if #451 seems reasonable!

[gabime]

Merged #451
Thanks @ThePhD