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

change max size of client.log #2356

Merged
merged 1 commit into from
Mar 18, 2022
Merged

Conversation

Eskibear
Copy link
Contributor

improve the case mentioned in #2319

With 100k as max size, sometimes it creates several log files for a single session, which is not convenient for diagnose. E.g. I have to try opening several log files to find the the one containing the command to start Java language server.

This PR simply increases it to 1m, in order to mitigate the inconvenience.

@Eskibear
Copy link
Contributor Author

BTW, I also considered to create client.log per session, then it would be much clearer to inspect without comparing timestamps.
But seems there's no simple option to set in winston-daily-rotate-file. So I choose this cheap way to mitigate.

Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I've run into this as well, when I first started working on the project, but I found it was easier to just look up the logs in the console, since that doesn't get truncated. With that said, this is mainly a problem for the verbose logging, but definitely a great improvement for diagnosing problems.

Only disadvantage I can think of is users would now be uploading a log file that in the worst case is 10x larger, although the current system does risk losing relevant information.

src/log.ts Outdated Show resolved Hide resolved
@Eskibear
Copy link
Contributor Author

...uploading a log file that in the worst case is 10x larger

Exactly. So ideally, how about we rotate client.log per session.
Advantage:

  • easy to look up logs
  • usually small file size

Disadvantage:

  • creates lots of files if you frequently reload window, but that's acceptable if we combine the strategy to only keep logs for 2 days.
  • extra effort on implementation of rotation, and update the way to open corresponding client.log

Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

For the time being, I'm fine with the current implementation. As you mention, we already rotate the logs every 2 days.

Doing a simple import, I estimated about 500kb of data when logging is verbose.

@testforstephen testforstephen added this to the End March 2022 milestone Mar 18, 2022
@testforstephen testforstephen merged commit e11c4e3 into redhat-developer:master Mar 18, 2022
@Eskibear Eskibear deleted the log branch March 18, 2022 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants