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

Adding minute_file_sink_mt #2727 #2729

Open
wants to merge 11 commits into
base: v1.x
Choose a base branch
from
Open

Adding minute_file_sink_mt #2727 #2729

wants to merge 11 commits into from

Conversation

mmanoj
Copy link

@mmanoj mmanoj commented May 4, 2023

Adding minute_file_sink_mt #2727 to support minute_file_sink to support every 1 min file rotation.
minute_file_sink.h.txt

5306b1f

mmanoj added 2 commits May 4, 2023 14:20
example driver code:
auto logger = spdlog::minute_logger_mt("minutes_basic_logger", "logs/min-log.txt",false,60);
Copy link
Contributor

@tt4g tt4g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented on the points of concern.
It must also pass @gabime review after modification.

{
filename_t basename, ext;
std::tie(basename, ext) = details::file_helper::split_by_extension(filename);
return fmt_lib::format(SPDLOG_FILENAME_T("{}_{:04d}-{:02d}-{:02d}-{:02d}_{:02d}{}"), basename, now_tm.tm_year + 1900, now_tm.tm_mon + 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return fmt_lib::format(SPDLOG_FILENAME_T("{}_{:04d}-{:02d}-{:02d}-{:02d}_{:02d}{}"), basename, now_tm.tm_year + 1900, now_tm.tm_mon + 1,
return fmt_lib::format(SPDLOG_FMT_STRING(SPDLOG_FILENAME_T("{}_{:04d}-{:02d}-{:02d}-{:02d}_{:02d}{}")), basename, now_tm.tm_year + 1900, now_tm.tm_mon + 1,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, under testing

filename_t basename, ext;
std::tie(basename, ext) = details::file_helper::split_by_extension(filename);
return fmt_lib::format(SPDLOG_FILENAME_T("{}_{:04d}-{:02d}-{:02d}-{:02d}_{:02d}{}"), basename, now_tm.tm_year + 1900, now_tm.tm_mon + 1,
now_tm.tm_mday, now_tm.tm_hour,now_tm.tm_min,ext);//colock details define in os-inl.h
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
now_tm.tm_mday, now_tm.tm_hour,now_tm.tm_min,ext);//colock details define in os-inl.h
now_tm.tm_mday, now_tm.tm_hour,now_tm.tm_min,ext);

class minute_file_sink final : public base_sink<Mutex>
{
public:
// create hourly file sink which rotates on given time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// create hourly file sink which rotates on given time
// create every minute file sink which rotates on given time

bool should_rotate = time >= rotation_tp_;
if (should_rotate)
{
if (remove_init_file_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you would delete an empty log file.
Why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part kept as it is from hourly log sink.

bool should_rotate = time >= rotation_tp_; if (should_rotate) { if (remove_init_file_) { file_helper_.close(); details::os::remove(file_helper_.filename());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I figured out why this code is there.
Perhaps, the author of sink wanted to clean up the log file created in the constructor, if it was empty, removing it at the time of rotation.

The following scenario is assumed.

  1. Create log file log.2023-01-01-12-01.txt in the constructor.
  2. More than one minute passes without a log being recorded.
  3. The contents of the file to be rotated are empty. Delete it as unnecessary and record log messages in a new log file.

Can you add a comment to this code as I am having difficulty understanding the purpose of this code?

* If max_files > 0, retain only the last max_files and delete previous.
*/
template<typename Mutex, typename FileNameCalc = minute_filename_calculator>
class minute_file_sink final : public base_sink<Mutex>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

daily_sink and hourly_sink can change the rotation time, but minute_file_sink seems to have a fixed rotation time. I suggest renaming it to every_minute_file_sink to avoid confusion.

{
auto now = log_clock::now();
tm date = now_tm(now);
date.tm_min = 0;
Copy link
Contributor

@tt4g tt4g May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
date.tm_min = 0;

The most important rotation feature seems to be broken.
The minute of the next rotation time is always 1 (1 minute every hour).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tt4g
Thanks for suggestions and review,I will add configuration to make X MIN interval to do the rotation as configurable number of minutes.Hope it more flexible.I also go through other changes as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with configurable rotation interval minutes

auto logger = spdlog::minute_logger_mt("basic_logger", "logs/min-log.txt",false,60,2);

Fixes as per suggestions
Copy link
Author

@mmanoj mmanoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed suggested changes and configurable minutes

@tt4g
Copy link
Contributor

tt4g commented May 4, 2023

I think the current rotation implementation is wrong.
minute_file_sink rotates at intervals (minutes).
However, the daily sink and hourly sink rotate at a specified time (hour or minute).
If you are adding sinks that work differently, I think you should change the name of the sinks to make it easier to understand.

@mmanoj
Copy link
Author

mmanoj commented May 4, 2023

@tt4g

Only sink name change required? hope the other implemented logic and suggestions are fixed?

@tt4g
Copy link
Contributor

tt4g commented May 4, 2023

Only sink name change required? hope the other implemented logic and suggestions are fixed?

Did you not develop this sink because you wanted it?
If you wanted this sink that rotates on an interval, change the name.
If not, please fix it.


if (rotation_minute < 0 || rotation_minute > 59)
{
throw_spdlog_ex("daily_file_sink: Invalid rotation time in ctor");
Copy link
Owner

@gabime gabime May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why can't it be greater than 59? what would happen if someone wants 90 minutes for example?
  2. should be "minute_file_sink: Invalid rotation_minutes value in ctor"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabime

Thanks for the feedback and got your point, remove the validation considering the concern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmanoj Note: In the current rotation implementation, rotation_m_ should not be allowed to be 0.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tt4g
Implement the check for 0 min.

@mmanoj
Copy link
Author

mmanoj commented May 7, 2023

@tt4g

Yes, This is required as per my project requirements and hope many people like the feature.I change the name to "minute_interval_logger_mt" to make it meaningful.

mmanoj added 3 commits May 7, 2023 12:48
Change the logger name to reflect the minutes interval based rotation and remove the check for max min 59 in configuration.
@gabime
Copy link
Owner

gabime commented May 7, 2023

@mmanoj Please add a unit test for this sink. Something very basic just to verify it compiles right (and perhaps test to validate it throws on invalid input).


template<typename Factory = spdlog::synchronous_factory>
inline std::shared_ptr<logger> minute_interval_logger_st(const std::string &logger_name, const filename_t &filename, bool truncate = false,
uint16_t max_files = 0,int minute = 0, const file_event_handlers &event_handlers = {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint16_t max_files = 0,int minute = 0, const file_event_handlers &event_handlers = {})
uint16_t max_files = 0, int minute = 1, const file_event_handlers &event_handlers = {})

minute = 0 will thrown.

//
template<typename Factory = spdlog::synchronous_factory>
inline std::shared_ptr<logger> minute_interval_logger_mt(const std::string &logger_name, const filename_t &filename, bool truncate = false,
uint16_t max_files = 0, int minute = 0, const file_event_handlers &event_handlers = {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint16_t max_files = 0, int minute = 0, const file_event_handlers &event_handlers = {})
uint16_t max_files = 0, int minute = 1, const file_event_handlers &event_handlers = {})

minute = 0 will thrown.

@mmanoj
Copy link
Author

mmanoj commented May 7, 2023

@mmanoj Please add a unit test for this sink. Something very basic just to verify it compiles right (and perhaps test to validate it throws on invalid input).

@gabime Sure,can you refer some example, I'm checking under test/test_daily_logger

@gabime
Copy link
Owner

gabime commented May 7, 2023

  1. Just look at any test in the tests for example. please create a new file in tests with onr or two tests.
  2. Also, I think a better name for the sink would be “periodic_file_sink”. This way in the future it could be extended to support any chrono units, not just minutes.
  3. Please remove the lime with the email address from the top comments. I try to have same header format for all files

@mmanoj
Copy link
Author

mmanoj commented May 8, 2023

  1. Working On the test cases files
  2. Rename the file and update the sink names as per the advice.Also we can make it seconds as base unit, so we can represent minutes and hours from that. What do you think?
  3. Done.

Updated as per advice.Please check and update if any changes required.
@mmanoj
Copy link
Author

mmanoj commented May 8, 2023

@gabime
Copy link
Owner

gabime commented May 8, 2023

  1. I meant using std::chrono::duration<Rep, Period> interval as for the interval. see https://github.com/gabime/spdlog/blob/57a9fd0841f00e92b478a07fef62636d7be612a8/include/spdlog/spdlog.h#LL85C12-L85C12 for an example.

However, for this to work the generated filename should probably also contain seconds in the name and the units restricted to seconds or higher,(e.g. not accept milliseconds or micros using std::enable_if)
so I suggest leaving this for a later stage and start with minutes for this initial version

  1. The name should be periodic_file_sink instead of periodic_interval_file_sink.

using std::chrono::duration
@mmanoj
Copy link
Author

mmanoj commented May 8, 2023

  1. I meant using std::chrono::duration<Rep, Period> interval as for the interval. see https://github.com/gabime/spdlog/blob/57a9fd0841f00e92b478a07fef62636d7be612a8/include/spdlog/spdlog.h#LL85C12-L85C12 for an example.

However, for this to work the generated filename should probably also contain seconds in the name and the units restricted to seconds or higher,(e.g. not accept milliseconds or micros using std::enable_if) so I suggest leaving this for a later stage and start with minutes for this initial version

  1. The name should be periodic_file_sink instead of periodic_interval_file_sink.

Updated as per advice.Please check and update.Once this clear I will work on the unit test

@gabime
Copy link
Owner

gabime commented May 10, 2023

I see the pr contains 2 sink files? which one is it?

@mmanoj
Copy link
Author

mmanoj commented May 10, 2023

I see the pr contains 2 sink files? which one is it?

https://github.com/mmanoj/spdlog/blob/v1.x/include/spdlog/sinks/periodic_file_sink.h

@mmanoj
Copy link
Author

mmanoj commented May 15, 2023

I see the pr contains 2 sink files? which one is it?

https://github.com/mmanoj/spdlog/blob/v1.x/include/spdlog/sinks/periodic_file_sink.h

@gabime

Any feedback regarding the PR?

@@ -0,0 +1,217 @@
// Copyright(c) 2015-present, Gabi Melman & spdlog contributors.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file as we use the other one

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// create every periodic file sink which rotates on given time
periodic_file_sink(
filename_t base_filename, bool truncate = false, uint16_t max_files = 0,
std::chrono::duration<int> rotation_period = 0, const file_event_handlers &event_handlers = {})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be

  periodic_file_sink(
       filename_t base_filename, 
       std::chrono::minutes rotation_interval,
       uint16_t max_files max_files,
       bool truncate_first = false,       
       const file_event_handlers &event_handlers = {})

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
throw_spdlog_ex("periodic_file_sink: Invalid rotation time in ctor");
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add some sanity restriction on max_files, especially since we store all the names in a buffer

  if (max_files > 2000)  
   {
         throw_spdlog_ex("periodic_file_sink: max_files param exceeds max of 2000");
   }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@gabime
Copy link
Owner

gabime commented May 16, 2023

Thanks. Would you add a test?

@mmanoj
Copy link
Author

mmanoj commented May 19, 2023

Thanks. Would you add a test?

If current implementation is fine,I will work on it.Please confirm.

@gabime
Copy link
Owner

gabime commented May 19, 2023

The thing is, I cannot know if it even complies without a minimal test. Just add minimal test that includes it and use it in the most basic way.

@mmanoj
Copy link
Author

mmanoj commented May 22, 2023

The thing is, I cannot know if it even complies without a minimal test. Just add minimal test that includes it and use it in the most basic way.

sure, will work on this and update you.

@mmanoj
Copy link
Author

mmanoj commented Aug 2, 2023

The thing is, I cannot know if it even complies without a minimal test. Just add minimal test that includes it and use it in the most basic way.

sure, will work on this and update you.

Can you help to understand how spdlog test framework work.I'm not able to run the test cases except example application under spdlog/tests/example directory.

for example how to run test_daily_logger.cpp

inline std::shared_ptr<logger> periodic_logger_mt(const std::string &logger_name, const filename_t &filename,std::chrono::minutes period = 1,uint16_t max_files = 0, bool truncate = false,
const file_event_handlers &event_handlers = {})
{
return Factory::template create<sinks::periodic_file_sink_mt>(logger_name, filename,period,max_files, truncate, event_handlers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Factory::template create<sinks::periodic_file_sink_mt>(logger_name, filename,period,max_files, truncate, event_handlers);
return Factory::template create<sinks::periodic_file_sink_mt>(logger_name, filename, period,max_files, truncate, event_handlers);

inline std::shared_ptr<logger> periodic_logger_st(const std::string &logger_name, const filename_t &filename,std::chrono::minutes period = 1,uint16_t max_files = 0, bool truncate = false,
const file_event_handlers &event_handlers = {})
{
return Factory::template create<sinks::periodic_file_sink_st>(logger_name, filename,period, max_files,truncate, event_handlers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Factory::template create<sinks::periodic_file_sink_st>(logger_name, filename,period, max_files,truncate, event_handlers);
return Factory::template create<sinks::periodic_file_sink_st>(logger_name, filename, period, max_files,truncate, event_handlers);

: base_filename_(std::move(base_filename))
, file_helper_{event_handlers}
, truncate_(truncate_first)
, max_files_(max_files)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if the max file size was also configurable
like 20 * 1024 *1024 (20MB)
like rotating_file_sink

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kegechen

Thanks for the suggestion,I will look in to that.I have some issues on running the test case as I mention above.If someone can help to fix it I can quickly close the PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't help, unfortunately I'm not Not familiar with the test case either
Catch2 tutorial may help

Copy link
Contributor

@kegechen kegechen Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by the way, to many commits in your pr. should have rebase first
the title of pr also should change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kegechen

Sorry for the delay due to some urgent tasks,I'm going to start working on this again.Will update the progress

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

Successfully merging this pull request may close these issues.

4 participants