add -debuglogsize=<n> option #14169

SuckShit commented at 11:01 AM on September 8, 2018: none

Add an option to limit maximum size of log files in MBytes, which renames itself to originname-datetimestamp.ext and creates a new log file if size reaches <n>. Default is 0, which means that the log file's size is unlimited.

add -debuglogsize=<n> option

Add an option to limit maximum size of log files in MBytes, which renames itself to originname-datetimestamp.ext and creates a new log file if size reaches <n>. Default is 0, which means that the log file's size is unlimited.

5bd58d914a

fanquake added the label Utils/log/libs on Sep 8, 2018

DrahtBot commented at 1:08 PM on September 8, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15266 (memory: Construct globals on first use by MarcoFalke)
#13088 (Log early messages with -printtoconsole by ajtowns)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

gmaxwell commented at 1:38 PM on September 8, 2018: contributor

this is pretty un-unixy, we already support external HUP signalled log rotation which is the standard thing.

unknown commented at 3:17 PM on September 8, 2018: none

nice find

unknown changes_requested

unknown commented at 3:18 PM on September 8, 2018: none

change

in src/init.cpp:373 in 5bd58d914a

 368 | @@ -369,7 +369,8 @@ void SetupServerArgs()
 369 |      gArgs.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), true, OptionsCategory::OPTIONS);
 370 |      gArgs.AddArg("-dbcache=<n>", strprintf("Set database cache size in megabytes (%d to %d, default: %d)", nMinDbCache, nMaxDbCache, nDefaultDbCache), false, OptionsCategory::OPTIONS);
 371 |      gArgs.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file. Relative paths will be prefixed by a net-specific datadir location. (-nodebuglogfile to disable; default: %s)", DEFAULT_DEBUGLOGFILE), false, OptionsCategory::OPTIONS);
 372 | -    gArgs.AddArg("-feefilter", strprintf("Tell other nodes to filter invs to us by our mempool min fee (default: %u)", DEFAULT_FEEFILTER), true, OptionsCategory::OPTIONS);
 373 | +    gArgs.AddArg("-debuglogsize=<n>", strprintf("Maximum size of log files in MBytes, which renames itself to originname-datetimestamp.ext and creates a new log file if size reaches <n>. Default is %u, which means that the log file's size is unlimited.", DEFAULT_MAX_LOG_SIZE), false, OptionsCategory::OPTIONS);
 374 | +	gArgs.AddArg("-feefilter", strprintf("Tell other nodes to filter invs to us by our mempool min fee (default: %u)", DEFAULT_FEEFILTER), true, OptionsCategory::OPTIONS);

promag commented at 10:54 PM on September 9, 2018:

Revert this change.

in src/init.cpp:392 in 5bd58d914a

 388 | @@ -388,7 +389,7 @@ void SetupServerArgs()
 389 |              "Warning: Reverting this setting requires re-downloading the entire blockchain. "
 390 |              "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), false, OptionsCategory::OPTIONS);
 391 |      gArgs.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk", false, OptionsCategory::OPTIONS);
 392 | -    gArgs.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", false, OptionsCategory::OPTIONS);
 393 | +    gArgs.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks", false, OptionsCategory::OPTIONS);

promag commented at 11:01 PM on September 9, 2018:

Why this change?

in src/logging.cpp:272 in 5bd58d914a

 267 | +                        m_fileout = fsbridge::fopen(m_file_path, "a");
 268 | +                        if (m_fileout != nullptr) {
 269 | +                            setbuf(m_fileout, nullptr); // unbuffered
 270 | +                        } 
 271 | +                    } catch (const fs::filesystem_error&) {
 272 | +                        return;

promag commented at 11:08 PM on September 9, 2018:

Why is the error ignored? Moreover the message is not logged.

in src/init.cpp:372 in 5bd58d914a

 368 | @@ -369,7 +369,8 @@ void SetupServerArgs()
 369 |      gArgs.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), true, OptionsCategory::OPTIONS);
 370 |      gArgs.AddArg("-dbcache=<n>", strprintf("Set database cache size in megabytes (%d to %d, default: %d)", nMinDbCache, nMaxDbCache, nDefaultDbCache), false, OptionsCategory::OPTIONS);
 371 |      gArgs.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file. Relative paths will be prefixed by a net-specific datadir location. (-nodebuglogfile to disable; default: %s)", DEFAULT_DEBUGLOGFILE), false, OptionsCategory::OPTIONS);
 372 | -    gArgs.AddArg("-feefilter", strprintf("Tell other nodes to filter invs to us by our mempool min fee (default: %u)", DEFAULT_FEEFILTER), true, OptionsCategory::OPTIONS);
 373 | +    gArgs.AddArg("-debuglogsize=<n>", strprintf("Maximum size of log files in MBytes, which renames itself to originname-datetimestamp.ext and creates a new log file if size reaches <n>. Default is %u, which means that the log file's size is unlimited.", DEFAULT_MAX_LOG_SIZE), false, OptionsCategory::OPTIONS);

promag commented at 11:08 PM on September 9, 2018:

Should be -debuglogmaxsize?

promag commented at 11:12 PM on September 9, 2018:

If DEFAULT_MAX_LOG_SIZE is changed then this message is wrong.

in src/logging.cpp:236 in 5bd58d914a

 231 | +                uint32_t sz = fs::file_size(m_file_path);
 232 | +                if (sz > 0 && sz + strTimestamped.size() > m_log_maxsize) {
 233 | +                    std::string originname = m_file_path.string();
 234 | +                    std::string ext;
 235 | +
 236 | +                    uint32_t i = originname.rfind(".");

practicalswift commented at 3:20 PM on September 30, 2018:

Why uint32_t and not size_t here?

in src/logging.cpp:262 in 5bd58d914a

 257 | +                        if (fs::exists(s.str())) {
 258 | +                            id++;
 259 | +                            continue;
 260 | +                        }
 261 | +                        newfilename = s.str();
 262 | +                        break;

practicalswift commented at 3:22 PM on September 30, 2018:

break at the end of while (true) looks a bit non-idiomatic. for or do instead?

DrahtBot added the label Needs rebase on Feb 4, 2019

DrahtBot commented at 7:42 PM on February 4, 2019: member

Needs rebase

practicalswift commented at 8:23 PM on February 4, 2019: contributor

Is this PR abandoned? Should it be closed? :-)

MarcoFalke commented at 9:54 PM on February 4, 2019: member

Closing for now. Let me know when you want to work on this again, so I can reopen.

MarcoFalke closed this on Feb 4, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021