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

Add config option to set max debug log size #24950

Closed
wants to merge 2 commits into from

Conversation

tehelsper
Copy link

This PR adds a new config option "shrinkdebugfilesize=" which accepts an integer specifying the file size of the debug log in megabytes. This option can be specified alongside "shrinkdebugfile=1" (which is the default) and when shrinking the debug log, it will now shrink it to the size specified.

The default size for debug.log shrinking is still 10 MB.

I was working on a tool to analyze IBD times and my log kept shrinking if I wanted to change settings or needed to reboot for some reason. This was causing lost data and I didn't want to let the log grow forever by setting shrinkdebugfile=0. That way I could leave it on passively and do post-analysis.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 23, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25614 (Severity-based logging, step 2 by jonatack)

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.

@laanwj
Copy link
Member

laanwj commented Apr 28, 2022

Thanks for your contribution. However, I'm not convinced that this doesn't add unnecessary complexity. In the past we've rejected features that add more fully fledged log management to bitcoind because, outside trivial usage, it's better to leave this to some dedicate external system such as logrotate.

This adds an extra command line option that adds configuration complexity, and also needs tests.

In your specific case I'd have recommended to disable the log shrinking completely for the duration of the experiment. Then you're 100% sure to not lose data, where heuristically choosing a new number does not.

@tehelsper
Copy link
Author

I see your point of view and would definitely agree if it was functionality to rotate logs or manage them in some way. I do disagree on this specific change since it only allows a user to configure a value that is hard-coded internally.

Since there is an internally hard-coded limit of 10M, it seems reasonable to allow a user to change it without any additional complexity. I would have found it more understandable if I saw this as a config option. It took some digging to even understand why my log seemed to be shrinking to a seemingly arbitrary number and having an argument in the help text indicating the default and the ability to change it may be helpful.

@tehelsper
Copy link
Author

I still need to get back and do some cleanup based on the linter and look more into automated testing. Manual testing for my test case has worked well.

@ghost
Copy link

ghost commented Apr 29, 2022

Since there is an internally hard-coded limit of 10M, it seems reasonable to allow a user to change it without any additional complexity.

Makes sense. Concept ACK.

This pull request is less complex as compared to #22350 and others

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

I would have expected the size to be used as the max debug log size, not the size to shrink to only when it's 10% higher.

Also, commit history is dirty (had a merge from master)

@laanwj
Copy link
Member

laanwj commented May 10, 2022

Since there is an internally hard-coded limit of 10M, it seems reasonable to allow a user to change it without any additional complexity.

Sure, but it's not entirely without maintenance burden. It means the code has to handle different sizes someone might configure. For example, the shrinking itself reads the entire size into memory, to clear the file and write it out again. This strategy works for a small size like 10MB, but if people set it to 1GB or more they may be in for a surprise.

@tehelsper
Copy link
Author

@laanwj Yeah, I agree a user could shoot themselves in the foot, but is that something that is typically prevented? Adding code to stop users from hurting themselves would add even more maintenance burden. Is there precedent for handing that when setting mempool size, dbcache, etc...?

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 1, 2022

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@achow101
Copy link
Member

Are you still working on this?

@tehelsper
Copy link
Author

It sounded a bit contentious. I'll go ahead and close it out.

@tehelsper tehelsper closed this Oct 12, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants