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 rotation of log files by size. Configuration requires user to pr… #288

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

knsmathers
Copy link

…ovide only rotation by period or size - configuration by both is not allowed. Because of the buffering done by streams, the rotation will be approximate. This is especially true if log messages are coming in at a very high rate.

…vide only rotation by period or size - configuration by both is not allowed. Because of the buffering done by streams, the rotation will be approximate. This is especially true if log messages are coming in at a very high rate.
…pon initialization if it is already above the configured size threshold.
@notfalsey
Copy link

+1

@paambaati
Copy link

+1 👍

@@ -1339,7 +1380,13 @@ RotatingFileStream.prototype.write = function write(s) {
this.rotQueue.push(s);
return false;
} else {
return this.stream.write(s);
if ((this.alreadyWritten + this.stream.bytesWritten) > this.rotateFileSize) {
Copy link

Choose a reason for hiding this comment

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

By checking the file and rotating immediately, we lose the log message that caused the rotation.
Can be resolved by adding a "this.rotQueue.push(s);" before the call to rotate.

Although the function returns false, this isn't an indicator that the caller should retry - simply an indicator that the data has been buffered.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching this! Fixed!

On Sat, Feb 6, 2016 at 10:55 AM, Jim Tupper [email protected]
wrote:

In lib/bunyan.js
#288 (comment):

@@ -1339,7 +1380,13 @@ RotatingFileStream.prototype.write = function write(s) {
this.rotQueue.push(s);
return false;
} else {

  •    return this.stream.write(s);
    
  •    if ((this.alreadyWritten + this.stream.bytesWritten) > this.rotateFileSize) {
    

By checking the file and rotating immediately, we lose the log message
that caused the rotation


Reply to this email directly or view it on GitHub
https://github.com/trentm/node-bunyan/pull/288/files#r52103575.

Kevin Smathers
Product Development Engineer
Tanium, Inc.
[email protected]

@ghost
Copy link

ghost commented Feb 12, 2016

+1

@Rcomian Rcomian mentioned this pull request Feb 21, 2016
@Jokero
Copy link

Jokero commented Mar 7, 2017

Do you plan to merge the PR?

@Jokero
Copy link

Jokero commented Dec 6, 2017

@trentm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants