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

Limit the size of the logfile #735

Closed
happysalada opened this issue Sep 10, 2021 · 20 comments
Closed

Limit the size of the logfile #735

happysalada opened this issue Sep 10, 2021 · 20 comments
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-good-first-issue Call for participation: Issues suitable for new contributors

Comments

@happysalada
Copy link
Contributor

Describe your feature request

I left the editor overnight somehow, I woke up to a 4GB logfile.
It seems something went wrong, but the logfile is filled with
2021-09-09T10:20:04.104 helix_lsp::transport [ERROR] err <-
the lines are truncated, so unfortunately I can't say much more.
How about limiting the size of the log file to 128 mb ? (just in case it gets out of control)

@happysalada happysalada added the C-enhancement Category: Improvements label Sep 10, 2021
@kirawi
Copy link
Member

kirawi commented Sep 10, 2021

Some alternative solutions may be to:

  1. Not log empty errors.
  2. Attempt to shutdown/kill the LSP process after a certain number of errors.
  3. Have less being logged when logging is not turned on via -v.

@kirawi
Copy link
Member

kirawi commented Sep 10, 2021

By the way, is this on master or 0.4.1? I believe it should have been fixed on master.

@sudormrfbin
Copy link
Member

There's some discussion on the issue of log size in #566. Raising the log level to warn or error by default would also keep the size in check.

@happysalada
Copy link
Contributor Author

it's on 0.4.1

@happysalada
Copy link
Contributor Author

also perhaps not truncate the error (in terms of line size limit)? if it was a little longer, more information could be gathered I think.

@archseer
Copy link
Member

There's no truncation happening, the error is just empty. If the LSP died we did not properly detect EOF and just kept reading 0 bytes and erroring. It's fixed on master 41f1e8e

Raising the log level to warn or error by default would also keep the size in check.

We actually do that already

0 => base_config.level(log::LevelFilter::Warn),

@sudormrfbin
Copy link
Member

Raising the log level to warn or error by default would also keep the size in check.

We actually do that already

Ah my log file was 20,000 lines long and there were INFO lines at the top from an older date, so I assumed they log level was unset.

There's no truncation happening, the error is just empty. If the LSP died we did not properly detect EOF and just kept reading 0 bytes and erroring. It's fixed on master 41f1e8e

Then I think this issue can be closed ?

@happysalada
Copy link
Contributor Author

I'll re-open if this happens again with master.
Thank you for taking the time to take a look!

@smores56
Copy link

Unfortunately, I just ran out of space on my device from a 17Gb log file. I deleted it before I read it to check the contents, so I'll make sure to do that next time, but I have only been using Helix for a week or so. Having a limit on the length seems pretty valuable.

@kirawi kirawi reopened this Oct 17, 2021
@kirawi kirawi added A-helix-term Area: Helix term improvements E-good-first-issue Call for participation: Issues suitable for new contributors labels Oct 17, 2021
@kirawi
Copy link
Member

kirawi commented Oct 17, 2021

I'm not sure if fern (the logging library we use) has built-in support for this, so we may have to roll our own logging struct.
daboross/fern#14

@smores56
Copy link

Yep, it was 2021-10-17T14:57:03.630 helix_lsp::transport [ERROR] err <- times a billion again.

@kirawi
Copy link
Member

kirawi commented Oct 17, 2021

That specific bug was already fixed in master.

There's no truncation happening, the error is just empty. If the LSP died we did not properly detect EOF and just kept reading 0 bytes and erroring. It's fixed on master 41f1e8e

Raising the log level to warn or error by default would also keep the size in check.

We actually do that already

0 => base_config.level(log::LevelFilter::Warn),

@smores56
Copy link

smores56 commented Oct 17, 2021

That specific bug was already fixed in master.

I installed from pacman, so I'll just compile from source this time.

@smores56
Copy link

Though this shouldn't be an issue for me, it still seems that having a non-truncating log file could be problematic here. fern doesn't currently have a way to auto-truncate, but it would be better IMO to add the functionality there rather than custom roll something here or change the entire logging library.

@kirawi
Copy link
Member

kirawi commented Oct 17, 2021

It wouldn't be something that drastic if we do custom roll something.

@pickfire
Copy link
Contributor

Probably we need to custom roll one ourselves, probably can use tracing-subscriber and write a custom MakeWriter. https://docs.rs/tracing-subscriber/0.2.25/tracing_subscriber/fmt/writer/trait.MakeWriter.html

@pickfire pickfire reopened this Oct 19, 2021
@archseer
Copy link
Member

Before we go overboard with the implementation, all we need to do is use https://doc.rust-lang.org/std/fs/struct.OpenOptions.html#method.truncate

@archseer
Copy link
Member

log_file generates an `OpenOptions: https://docs.rs/fern/0.6.0/fern/fn.log_file.html

@kirawi
Copy link
Member

kirawi commented Oct 19, 2021

It's not quite the same since it will completely wipe the log if you were to open helix again. This means that it doesn't prevent you from racking up a 17gb log in one session (though I guess it's only a problem if there was a bug like before). It's probably a good enough solution though.

@archseer
Copy link
Member

#895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-good-first-issue Call for participation: Issues suitable for new contributors
Projects
None yet
Development

No branches or pull requests

6 participants