-
Notifications
You must be signed in to change notification settings - Fork 844
Options to roll empty logs and log trimming #5700
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
Conversation
| .. ts:cv:: CONFIG proxy.config.output.logfile.rolling_allow_empty INT 0 | ||
| :reloadable: | ||
|
|
||
| While rolling default behaviour is to rename, close and re-open the log file *only* when/if there is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've been using American english... so, behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
| Set :ts:cv:`proxy.config.output.logfile.rolling_max_count` (yaml: `rolling_max_count`) to 1 | ||
| which will lead to keeping only one rolled log file at any moment (rolled will be trimmed on every roll). | ||
|
|
||
| Set :ts:cv:`proxy.config.output.logfile.rolling_allow_empty` (yaml: `rolling_allow_empty`) to 1 (default: 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra whitespace?
allow_empty`) to 1
^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| Set :ts:cv:`proxy.config.output.logfile.rolling_allow_empty` (yaml: `rolling_allow_empty`) to 1 (default: 0) | ||
| which will allow logs to be open and rolled even if there was nothing to be logged during the previous period | ||
| (i.e. no requests to the traffic server). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to |TS|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
| -------------------------------------------------- | ||
|
|
||
| If for security reasons logs need to be purged to make sure no log entry remains on the box | ||
| for more then a specified period of time, we could achieve this by setting the following variables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
achieve this by setting the following variables ==>
achieve this by setting the rolling interval, the maximum number of rolled log files, and forcing |TS| to roll even when there is no traffic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
|
[approve ci autest] |
2 similar comments
|
[approve ci autest] |
|
[approve ci autest] |
proxy/logging/LogObject.cc
Outdated
| long time_now = LogUtils::timestamp(); | ||
|
|
||
| if (logfile.roll(time_now - log_object->get_rolling_interval(), time_now) == 0) { | ||
| if (logfile.roll(time_now - log_object->get_rolling_interval(), time_now)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at LogFile::roll(), return value of 1 means successful rotation? And this line should be looking for the error condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, great catch! Did not mean to change this line, likely did not restore this line properly after my experiments.
proxy/logging/LogFile.cc
Outdated
| std::sort(rolled.begin(), rolled.end(), [](const RolledFile a, const RolledFile b) { return a._mtime > b._mtime; }); | ||
| if (rolling_max_count < rolled.size()) { | ||
| for (auto it = rolled.begin() + rolling_max_count; it != rolled.end(); it++) { | ||
| RolledFile file = *it; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a const reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, changing it to a const reference
| A default value of 0 means auto-deletion will not try to limit the number of output logs. | ||
| See :doc:`../logging/rotation.en` for an use-case for this option. | ||
|
|
||
| .. ts:cv:: CONFIG proxy.config.output.logfile.rolling_allow_empty INT 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would set this to be on by default and not have a configuration option for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me, other solutions like i.e. multilog have similar behavior by default.
But from traffic server logging code it seems to me that someone has gone great lengths to make sure no empty log files are rolled, so having an option to enable/disable this behavior made more sense to me.
Added 2 options: - proxy.config.log.rolling_allow_empty - ability to roll empty logs (i.e. rolling logs without traffic) - proxy.config.log.rolling_max_count - trimming logs to a certain number of rolled files on each rolling More info in records.config.en.rst and rotation.en.rst.
|
@bryancall, @dyrock, @mlibbey is there anything else I could address ? |
|
This has merge conflicts, and we'll need a new 8.1.x PR. Also, feel free to recommend / suggest other PRs that this depends on, such that it makes the back porting easier. |
|
Created PR #6553 to resolve the cherry-pick/merge conflicts. |
|
This was back ported and merged into 8.1.0 release. |
Added 2 options:
proxy.config.log.rolling_allow_empty- ability to roll empty logs(i.e. rolling logs without traffic)
proxy.config.log.rolling_max_count- trimming logs to a certainnumber of rolled files on each rolling
More info and use-cases in records.config.en.rst and rotation.en.rst.