Skip to content

Break diags log rotation into subroutines #13254

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.

Sign up for GitHub

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

Open
JosiahWI wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Break diags log rotation into subroutines #13254

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a25823b
Break diag log rotation into subroutines
Jun 10, 2026
fd01a92
Address Copilot review
JosiahWI Jun 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/tscore/DiagsTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,4 +274,13 @@ class Diags : public DebugInterface
{
ink_mutex_release(&tag_table_lock);
}

bool roll_diags_on_filesize(int mb_threshold);
bool roll_diags_on_interval(int interval);

bool roll_streams_on_filesize(int mb_threshold);
bool roll_streams_on_interval(int interval);

std::unique_ptr<BaseLogFile> remake_logfile(BaseLogFile const *logfile);
void swap_diagslog(BaseLogFile *new_diags);
};
294 changes: 173 additions & 121 deletions src/tscore/Diags.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
#include "tscore/Regression.h"
#include "tscore/Diags.h"
#include "ts/ts.h"

#include <memory>
#include <string_view>
Comment thread
JosiahWI marked this conversation as resolved.

int DiagsConfigState::_enabled[2] = {0, 0};
Expand Down Expand Up @@ -503,17 +505,10 @@ Diags::reseat_diagslog()
return false;
}
fflush(diags_log->m_fp);
char *oldname = ats_strdup(diags_log->get_name());
log_log_trace("in %s for diags.log, oldname=%s\n", __func__, oldname);
BaseLogFile *n = new BaseLogFile(oldname);
if (setup_diagslog(n)) {
BaseLogFile *old_diags = diags_log;
lock();
diags_log = n;
unlock();
delete old_diags;

if (std::unique_ptr<BaseLogFile> new_logfile{remake_logfile(diags_log)}; new_logfile) {
swap_diagslog(new_logfile.release());
}
ats_free(oldname);
return true;
}

Expand Down Expand Up @@ -546,53 +541,16 @@ Diags::should_roll_diagslog()
if (diags_log && diags_log->is_init()) {
if (diagslog_rolling_enabled == RollingEnabledValues::ROLL_ON_SIZE ||
diagslog_rolling_enabled == RollingEnabledValues::ROLL_ON_TIME_OR_SIZE) {
// if we can't even check the file, we can forget about rotating
struct stat buf;
if (fstat(fileno(diags_log->m_fp), &buf) != 0) {
try {
ret_val = roll_diags_on_filesize(diagslog_rolling_size);
} catch (std::runtime_error const &e) {
Comment thread
JosiahWI marked this conversation as resolved.
Outdated
return false;
}

off_t size = buf.st_size;
if (diagslog_rolling_size != -1 && size >= (static_cast<off_t>(diagslog_rolling_size) * BYTES_IN_MB)) {
fflush(diags_log->m_fp);
if (diags_log->roll()) {
char *oldname = ats_strdup(diags_log->get_name());
log_log_trace("in %s for diags.log, oldname=%s\n", __func__, oldname);
BaseLogFile *n = new BaseLogFile(oldname);
if (setup_diagslog(n)) {
BaseLogFile *old_diags = diags_log;
lock();
diags_log = n;
unlock();
delete old_diags;
}
ats_free(oldname);
ret_val = true;
}
}
}

if (diagslog_rolling_enabled == RollingEnabledValues::ROLL_ON_TIME ||
diagslog_rolling_enabled == RollingEnabledValues::ROLL_ON_TIME_OR_SIZE) {
time_t now = time(nullptr);
if (diagslog_rolling_interval != -1 && (now - diagslog_time_last_roll) >= diagslog_rolling_interval) {
fflush(diags_log->m_fp);
if (diags_log->roll()) {
diagslog_time_last_roll = now;
char *oldname = ats_strdup(diags_log->get_name());
log_log_trace("in %s for diags.log, oldname=%s\n", __func__, oldname);
BaseLogFile *n = new BaseLogFile(oldname);
if (setup_diagslog(n)) {
BaseLogFile *old_diags = diags_log;
lock();
diags_log = n;
unlock();
delete old_diags;
}
ats_free(oldname);
ret_val = true;
}
}
ret_val = roll_diags_on_interval(diagslog_rolling_interval);
}
}

Expand All @@ -619,8 +577,7 @@ Diags::should_roll_outputlog()
ink_assert(stdout_log != nullptr);
ink_assert(stderr_log != nullptr);

bool ret_val = false;
bool need_consider_stderr = true;
bool ret_val = false;

log_log_trace("%s was called\n", __func__);
log_log_trace("%s: rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n", __func__,
Expand All @@ -634,83 +591,19 @@ Diags::should_roll_outputlog()
if (stdout_log->is_init()) {
if (outputlog_rolling_enabled == RollingEnabledValues::ROLL_ON_SIZE ||
outputlog_rolling_enabled == RollingEnabledValues::ROLL_ON_TIME_OR_SIZE) {
// if we can't even check the file, we can forget about rotating
struct stat buf;
if (fstat(fileno(stdout_log->m_fp), &buf) != 0) {
try {
ret_val = roll_streams_on_filesize(outputlog_rolling_size);
} catch (std::runtime_error &e) {
Comment thread
JosiahWI marked this conversation as resolved.
Outdated
return false;
}

off_t size = buf.st_size;
if (outputlog_rolling_size != -1 && size >= static_cast<off_t>(outputlog_rolling_size) * BYTES_IN_MB) {
// since usually stdout and stderr are the same file on disk, we should just
// play it safe and just flush both BaseLogFiles
if (stderr_log->is_init()) {
fflush(stderr_log->m_fp);
}
fflush(stdout_log->m_fp);

if (stdout_log->roll()) {
char *oldname = ats_strdup(stdout_log->get_name());
log_log_trace("in %s(), oldname=%s\n", __func__, oldname);
set_std_output(StdStream::STDOUT, oldname);

// if stderr and stdout are redirected to the same place, we should
// update the stderr_log object as well
if (!strcmp(oldname, stderr_log->get_name())) {
log_log_trace("oldname == stderr_log->get_name()\n");
set_std_output(StdStream::STDERR, oldname);
need_consider_stderr = false;
}
ats_free(oldname);
ret_val = true;
}
}
}

if (outputlog_rolling_enabled == RollingEnabledValues::ROLL_ON_TIME ||
outputlog_rolling_enabled == RollingEnabledValues::ROLL_ON_TIME_OR_SIZE) {
time_t now = time(nullptr);
if (outputlog_rolling_interval != -1 && (now - outputlog_time_last_roll) >= outputlog_rolling_interval) {
// since usually stdout and stderr are the same file on disk, we should just
// play it safe and just flush both BaseLogFiles
if (stderr_log->is_init()) {
fflush(stderr_log->m_fp);
}
fflush(stdout_log->m_fp);

if (stdout_log->roll()) {
outputlog_time_last_roll = now;
char *oldname = ats_strdup(stdout_log->get_name());
log_log_trace("in %s, oldname=%s\n", __func__, oldname);
set_std_output(StdStream::STDOUT, oldname);

// if stderr and stdout are redirected to the same place, we should
// update the stderr_log object as well
if (!strcmp(oldname, stderr_log->get_name())) {
log_log_trace("oldname == stderr_log->get_name()\n");
set_std_output(StdStream::STDERR, oldname);
need_consider_stderr = false;
}
ats_free(oldname);
ret_val = true;
}
}
ret_val = roll_streams_on_interval(outputlog_rolling_interval);
}
}

// This assertion has to be true since log rolling for traffic.out is only ever enabled
// (and useful) when traffic_server is NOT running in stand alone mode. If traffic_server
// is NOT running in stand alone mode, then stderr and stdout SHOULD ALWAYS be pointing
// to the same file (traffic.out).
//
// If for some reason, someone wants the feature to have stdout pointing to some file on
// disk, and stderr pointing to a different file on disk, and then also wants both files to
// rotate according to the (same || different) scheme, it would not be difficult to add
// some more config options in records.yaml and said feature into this function.
if (ret_val) {
ink_assert(!need_consider_stderr);

@JosiahWI JosiahWI Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Instead of asserting that this flag was set, I am asserting at an earlier point that the condition for setting the flag is met. The code was originally designed to be a little easier to change if more output stream flexibility was needed. Since that obviously hasn't happened for a long time, and was only a small amount of code, I have removed the extra logic flow and this flag entirely, simplifying the code.

}

return ret_val;
}

Expand Down Expand Up @@ -810,3 +703,162 @@ Diags::rebind_std_stream(StdStream stream, int new_fd)
}
return false;
}

bool
Diags::roll_diags_on_filesize(int mb_threshold)
{
// if we can't even check the file, we can forget about rotating
struct stat buf;
if (fstat(fileno(diags_log->m_fp), &buf) != 0) {
throw std::runtime_error{"could not stat diags log"};
}

off_t size = buf.st_size;
if (mb_threshold == -1 || size < static_cast<off_t>(mb_threshold) * BYTES_IN_MB) {
return false;
}

fflush(diags_log->m_fp);

if (!diags_log->roll()) {
return false;
}

if (std::unique_ptr<BaseLogFile> new_logfile{remake_logfile(diags_log)}; new_logfile) {
swap_diagslog(new_logfile.release());
}
return true;
}

bool
Diags::roll_diags_on_interval(int interval)
{
time_t now = time(nullptr);
if (interval == -1 || (now - diagslog_time_last_roll) < interval) {
return false;
}

fflush(diags_log->m_fp);

if (!diags_log->roll()) {
return false;
}

diagslog_time_last_roll = now;
if (std::unique_ptr<BaseLogFile> new_logfile{remake_logfile(diags_log)}; new_logfile) {
swap_diagslog(new_logfile.release());
}
return true;
}

bool
Diags::roll_streams_on_filesize(int mb_threshold)
{
// if we can't even check the file, we can forget about rotating
struct stat buf;
if (fstat(fileno(stdout_log->m_fp), &buf) != 0) {
throw std::runtime_error{"could not stat output log"};
}

off_t size = buf.st_size;
if (mb_threshold == -1 || size < static_cast<off_t>(mb_threshold) * BYTES_IN_MB) {
return false;
}

// since usually stdout and stderr are the same file on disk, we should just
// play it safe and just flush both BaseLogFiles
if (stderr_log->is_init()) {
fflush(stderr_log->m_fp);
}
fflush(stdout_log->m_fp);

if (!stdout_log->roll()) {
return false;
}

char *oldname = ats_strdup(stdout_log->get_name());
log_log_trace("in %s(), oldname=%s\n", __func__, oldname);
set_std_output(StdStream::STDOUT, oldname);

// This assertion has to be true since log rolling for traffic.out is only ever enabled
// (and useful) when traffic_server is NOT running in stand alone mode. If traffic_server
// is NOT running in stand alone mode, then stderr and stdout SHOULD ALWAYS be pointing
// to the same file (traffic.out).
//
// If for some reason, someone wants the feature to have stdout pointing to some file on
// disk, and stderr pointing to a different file on disk, and then also wants both files to
// rotate according to the (same || different) scheme, it would not be difficult to add
// some more config options in records.yaml and said feature into this function.
ink_assert(0 == strcmp(stdout_log->get_name(), stderr_log->get_name()));
log_log_trace("oldname == stderr_log->get_name()\n");
set_std_output(StdStream::STDERR, oldname);
Comment thread
JosiahWI marked this conversation as resolved.
Outdated

ats_free(oldname);
return true;
}

bool
Diags::roll_streams_on_interval(int interval)
{
time_t now = time(nullptr);
if (interval == -1 || (now - outputlog_time_last_roll) < interval) {
return false;
}

// since usually stdout and stderr are the same file on disk, we should just
// play it safe and just flush both BaseLogFiles
if (stderr_log->is_init()) {
fflush(stderr_log->m_fp);
}
fflush(stdout_log->m_fp);

if (!stdout_log->roll()) {
return false;
}

outputlog_time_last_roll = now;
char *oldname = ats_strdup(stdout_log->get_name());
log_log_trace("in %s, oldname=%s\n", __func__, oldname);
set_std_output(StdStream::STDOUT, oldname);

// This assertion has to be true since log rolling for traffic.out is only ever enabled
// (and useful) when traffic_server is NOT running in stand alone mode. If traffic_server
// is NOT running in stand alone mode, then stderr and stdout SHOULD ALWAYS be pointing
// to the same file (traffic.out).
//
// If for some reason, someone wants the feature to have stdout pointing to some file on
// disk, and stderr pointing to a different file on disk, and then also wants both files to
// rotate according to the (same || different) scheme, it would not be difficult to add
// some more config options in records.yaml and said feature into this function.
ink_assert(0 == strcmp(stdout_log->get_name(), stderr_log->get_name()));
log_log_trace("oldname == stderr_log->get_name()\n");
set_std_output(StdStream::STDERR, oldname);
Comment thread
JosiahWI marked this conversation as resolved.
Outdated

ats_free(oldname);
return true;
}

std::unique_ptr<BaseLogFile>
Diags::remake_logfile(BaseLogFile const *logfile)
{
char *oldname = ats_strdup(logfile->get_name());
log_log_trace("in %s for diags.log, oldname=%s\n", __func__, oldname);
auto new_logfile = std::make_unique<BaseLogFile>(oldname);
ats_free(oldname);

if (!setup_diagslog(new_logfile.get())) {
return nullptr;
}

return new_logfile;
}

void
Diags::swap_diagslog(BaseLogFile *new_diags)
{
BaseLogFile *old_diags = diags_log;
lock();
diags_log = new_diags;
unlock();
delete old_diags;
}