Skip to content

refactor, remove winston, implement file-rotation instead #9584

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

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

davidfirst
Copy link
Member

@davidfirst davidfirst commented Feb 26, 2025

Currently, we use Winston solely for file rotation, while our primary logger is Pino. However, loading Winston during Bit’s bootstrap is costly, adding 94 files to the total (~1,850).

When we initially switched from Winston to Pino, Pino didn't offer a file rotation solution. Now it does - pino-roll, which I tested but ended up not using for the following reasons:

  1. The file rotation changes the active log file. To maintain a single log file, we have to use the symlink option, which doesn't work well with the sync option that we need. (we tried hard in the past to avoid using sync without success).
  2. the file rotation doesn't happen occasionally, especially when using the extension prop.
  3. (not a blocker) there is no option to choose the filename of the symlink, it's fixed as "current.log", so we'll have to introduce a change in this regard. (it's debug.log currently).

Since Bit primarily runs as a CLI tool, which initializes fresh with each command, we don’t need real-time log rotation. Instead, we can simply check the log file status at bootstrap and rotate if needed. This is effectively how it works now, with Winston being called only once during bootstrap.

This PR removes Winston and introduces a minimal file rotation mechanism. It rotates the debug.log to a file named "debug-YYYY-MM-DD.log" if it was last modified on a previous day and keeps only the most recent 7 daily logs - executing only once at bootstrap.

@davidfirst davidfirst requested a review from zkochan February 27, 2025 14:36
Copy link
Member

@zkochan zkochan left a comment

Choose a reason for hiding this comment

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

Added one comment about a possible race condition.

Copy link
Member

@zkochan zkochan left a comment

Choose a reason for hiding this comment

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

I like the daily approach. Hopefully file size won't be an issue.

@davidfirst davidfirst merged commit 976b036 into master Feb 27, 2025
10 checks passed
@davidfirst davidfirst deleted the remove-winston branch February 27, 2025 18:02
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.

2 participants