Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please support local timezone #7

Closed
toge opened this issue Apr 7, 2020 · 14 comments
Closed

Please support local timezone #7

toge opened this issue Apr 7, 2020 · 14 comments
Labels
enhancement New feature or request

Comments

@toge
Copy link

toge commented Apr 7, 2020

Thank you for your great work!
I love this library.

One thing to prevent using quill is supporting local time zone.
Please support local timezone.
Currently quill::PatternFormatter::_timezone_type is private and hard coded.

@azuresigma
Copy link

Second this. Timestamp in local time zone will be convenient. It will be even better if the logger can take TZ environment variable.

typos:

  1. https://github.com/odygrd/quill/wiki/3.-Formatters has %(levelname) for log level, but actually it is 'level_name'. I figured it out by grep source code.
  2. It will also be nice to put QUILL_STRING in the link above. QUILL_STRING was mis-spelled in a couple of comments in code(only 1 'L' for QUILL).

suggestion:

  1. I would suggest to print level_name like 'DEBUG', instead of 'LOG_DEBUG'. It is more common to see '[DEBUG]' for log level string.
  2. It is 'nice to have' if local date can be appended to file name for file_handler. I see there is a daily_file_handler, but I don't need rotation.

I have looked at many logging libraries online. I was about to give up to write my own and then I found quill:) This library looks really promising. Great work! Thank you!

@odygrd
Copy link
Owner

odygrd commented Apr 10, 2020

Hello,

Thanks for the good feedback and the comments

  1. the localtime zone support is implemented. I think i have forgotten to make it possible for the user to change the default gmt time. I will fix this for sure in the next release. Thanks a lot for catching this.

  2. Thanks a lot for the typos I will look into them and fix them also

With regards to DEBUG vs LOG_DEBUG. The reason for using 'LOG_DEBUG' is that in a large application in past we found out that the keyword 'DEBUG' could be printed also by other libraries. Therefore, we wanted to be able to filter out the debug logs only and a good way was by using 'LOG_DEBUG'. To keep everyone happy I think those predefined strings can be moved to tweakme.h where each user can customise them based on their needs

@odygrd odygrd added the enhancement New feature or request label Apr 10, 2020
@azuresigma
Copy link

hi,
regarding documentation in https://github.com/odygrd/quill/wiki/3.-Formatters.

  1. "The following formatter is used by default :
    ascii_time [thread_id] filename:line log_level logger_name - message"
    log_level should be level_name? I first copy/paste the string, didn't work. It will be more user friendly to reference https://github.com/odygrd/quill/blob/master/examples/example_custom_formatter.cpp, or put this line of code there. So first time users can easily customize without deep drive into code.
    file_handler->set_pattern(QUILL_STRING("%(ascii_time) %(logger_name) - %(message)"), "%D %H:%M:%S",
    quill::PatternFormatter::TimestampPrecision::MilliSeconds);

Now I know https://github.com/odygrd/quill/blob/master/examples is a good place for documentation. But it is not so obvious by just reading README.

  1. asctime in the table should be ascii_time.

@azuresigma
Copy link

Regarding 'DEBUG' vs 'LOG_DEBUG', for users who need 'LOG_DEBUG' to filter, they can do
file_handler->set_pattern(QUILL_STRING("... LOG_%(level_name)")?

If the above line works, then it is up to the user how to make log level string unique for filtering. It can be "DEBUG", "[DEBUG]", or "LOG_DEBUG", depending on various application needs.

@odygrd
Copy link
Owner

odygrd commented Apr 11, 2020

Thanks a lot for feedback and ideas.

  • I have updated the formatter wiki to match with the code and with your suggestions.
  • The log level names are now "DEBUG" etc. Thanks for the nice idea to just prefix "LOG_" in the formatter string in 756b5da
  • append_date to file handler in 854c792
  • localtime support in 5e63725

@odygrd
Copy link
Owner

odygrd commented Apr 12, 2020

If there are no more requests in this, I am closing it tomorrow

@azuresigma
Copy link

It looks really good! Thanks!

@azuresigma
Copy link

FYI. I tried to build quill 28a4575 with my existing fmt 6.1.2, got a lot of compile errors about ostream.h:120, type_identity_t was not declared in this scope. changed my fmt to 6.2.0 and everything worked.

  • local time: working as expected
  • append date to file name: working as expected
  • new level string: working as expected

@azuresigma
Copy link

A workaround for fmt 6.1.2: include quill.h before 6.1.2 fmt/format.h.

@azuresigma
Copy link

azuresigma commented Apr 12, 2020

Now quill requires users to have the fmt 6.2.0, which seems to have some backward compatibility issue. Is every user willing to upgrade to this version? Is it possible/better to let users specify where fmt installation is, instead of always using the bundled fmt headers? It is your call. I have no issue to use the latest fmt version as my app is pretty new, not in production yet.

@odygrd
Copy link
Owner

odygrd commented Apr 12, 2020

I see ... :(

I tried a few things like putting the bundled fmt under separate namespace etc but it won't work because of all the preprocessor definitions when the fmt 6.1.2 header is included

If your app is new one solution would be instead of having your own lib fmt version to use the bundled one from Quill directly, like include quill/bundled/fmt

To support different fmt versions the only solution looks like supporting an FMT_EXTERNAL cmake option where Quill uses the external fmt library instead of the bundled one

@odygrd
Copy link
Owner

odygrd commented Apr 12, 2020

Closing this and moving it to #11

@odygrd odygrd closed this as completed Apr 12, 2020
@odygrd
Copy link
Owner

odygrd commented Apr 12, 2020

Hey @azuresigma

Could you please try with b66fd65 and confirm.

Documentation is here will add to the README later..

I hope it resolves the issue of using an external version of lib fmt!

@azuresigma
Copy link

Hi, sorry I didn't see your comment here. I did see your commit b66fd65 yesterday and tried it. It worked!

I used 'cmake -DCMAKE_PREFIX_PATH=/my/fmt/fmt-config.cmake-directory/ -DQUILL_FMT_EXTERNAL=ON -DCMAKE_INSTALL_PREFIX=/new/quill/install-dir/'.

Make output showed bundled fmt cc files were not compiled. The app program builds and works as expected.

Thank you for the great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants