-
Notifications
You must be signed in to change notification settings - Fork 157
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
Transport refactor #107
Merged
+1,901
−1,705
Merged
Transport refactor #107
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
fa75415
refactoring transport to use third-party file rotation component
9ba0633
setting some more sane defaults
07c31d0
emitting rotate event
407f0cb
Documentation update
e569e02
adding missing dependency
b028bc3
removing support for nodejs versions < 4. Adding node 6 and 8 to trav…
9e1c68d
adding winston 3.0.0-rc1 compatibility
9af7adc
adding callback to test
2e50195
version bump for rc
a3e62a9
ignoring test files
f3ba4b3
updating for winston3 changes in formatting
834b346
version bump for rc.2
4962304
adding triple-beam reference
e130267
temporary fix until upstream file-stream-rotator has patch in place
35f0733
incorporating file-stream-rotator fix to continue last file if below …
16c3586
prevent bubbling of events from the underlying stream
c36fb29
move cleanup to _final() method and update documentation relating to …
4c674e8
version bump for @next
8e82e5e
implementing custom method to close stream for tests
3ed1b1e
version bump to support 2.x release
0a2bff2
winston@next peer dependency is now rc2
7b36f34
Merge branch 'master' into transport-refactor
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
module.exports = exports = { | ||
extends: 'xo', | ||
env: { | ||
node: true, | ||
mocha: true | ||
}, | ||
rules: { | ||
indent: ['error', 4], | ||
camelcase: ['error', {properties: 'never'}] | ||
} | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
# Logs | ||
logs | ||
*.log | ||
*.log.* | ||
*log* | ||
test.js | ||
|
||
# Runtime data | ||
pids | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,260 @@ | ||
'use strict'; | ||
|
||
var fs = require('fs'); | ||
var os = require('os'); | ||
var path = require('path'); | ||
var util = require('util'); | ||
var semver = require('semver'); | ||
var zlib = require('zlib'); | ||
var winston = require('winston'); | ||
var compat = require('winston-compat'); | ||
var MESSAGE = require('triple-beam').MESSAGE; | ||
var PassThrough = require('stream').PassThrough; | ||
var Transport = semver.major(winston.version) === 2 ? compat.Transport : require('winston-transport'); | ||
|
||
var loggerDefaults = { | ||
json: false, | ||
colorize: false, | ||
eol: os.EOL, | ||
logstash: null, | ||
prettyPrint: false, | ||
label: null, | ||
stringify: false, | ||
depth: null, | ||
showLevel: true, | ||
timestamp: function () { | ||
return new Date().toISOString(); | ||
} | ||
}; | ||
|
||
var DailyRotateFile = function (options) { | ||
options = options || {}; | ||
Transport.call(this, options); | ||
|
||
function throwIf(target /* , illegal... */) { | ||
Array.prototype.slice.call(arguments, 1).forEach(function (name) { | ||
if (options[name]) { | ||
throw new Error('Cannot set ' + name + ' and ' + target + ' together'); | ||
} | ||
}); | ||
} | ||
|
||
function getMaxSize(size) { | ||
if (size && typeof size === 'string') { | ||
var _s = size.toLowerCase().match(/^((?:0\.)?\d+)([k|m|g])$/); | ||
if (_s) { | ||
return size; | ||
} | ||
} else if (size && Number.isInteger(size)) { | ||
var sizeK = Math.round(size / 1024); | ||
return sizeK === 0 ? '1k' : sizeK + 'k'; | ||
} | ||
return null; | ||
} | ||
|
||
this.options = Object.assign({}, loggerDefaults, options); | ||
|
||
if (options.stream) { | ||
throwIf('stream', 'filename', 'maxsize'); | ||
this.logStream = new PassThrough(); | ||
this.logStream.pipe(options.stream); | ||
} else { | ||
this.filename = options.filename ? path.basename(options.filename) : 'winston.log'; | ||
this.dirname = options.dirname || path.dirname(options.filename); | ||
|
||
var self = this; | ||
|
||
this.logStream = require('file-stream-rotator').getStream({ | ||
filename: path.join(this.dirname, this.filename), | ||
frequency: 'custom', | ||
date_format: options.datePattern ? options.datePattern : 'YYYY-MM-DD', | ||
verbose: false, | ||
size: getMaxSize(options.maxSize), | ||
max_logs: options.maxFiles | ||
}); | ||
|
||
this.logStream.on('rotate', function (oldFile, newFile) { | ||
self.emit('rotate', oldFile, newFile); | ||
}); | ||
|
||
if (options.zippedArchive) { | ||
this.logStream.on('rotate', function (oldFile) { | ||
var gzip = zlib.createGzip(); | ||
var inp = fs.createReadStream(oldFile); | ||
var out = fs.createWriteStream(oldFile + '.gz'); | ||
inp.pipe(gzip).pipe(out).on('finish', function () { | ||
fs.unlinkSync(oldFile); | ||
}); | ||
}); | ||
} | ||
} | ||
}; | ||
|
||
module.exports = DailyRotateFile; | ||
|
||
util.inherits(DailyRotateFile, Transport); | ||
|
||
DailyRotateFile.prototype.name = 'dailyRotateFile'; | ||
|
||
var noop = function () {}; | ||
if (semver.major(winston.version) === 2) { | ||
DailyRotateFile.prototype.log = function (level, msg, meta, callback) { | ||
callback = callback || noop; | ||
var options = Object.assign({}, this.options, { | ||
level: level, | ||
message: msg, | ||
meta: meta | ||
}); | ||
|
||
var output = compat.log(options) + options.eol; | ||
this.logStream.write(output); | ||
callback(null, true); | ||
}; | ||
} else { | ||
DailyRotateFile.prototype.normalizeQuery = compat.Transport.prototype.normalizeQuery; | ||
DailyRotateFile.prototype.log = function (info, callback) { | ||
callback = callback || noop; | ||
|
||
this.logStream.write(info[MESSAGE] + this.options.eol); | ||
this.emit('logged', info); | ||
callback(null, true); | ||
}; | ||
} | ||
|
||
DailyRotateFile.prototype.close = function () { | ||
var self = this; | ||
if (this.logStream) { | ||
this.logStream.end(function () { | ||
self.emit('finish'); | ||
}); | ||
} | ||
}; | ||
|
||
DailyRotateFile.prototype.query = function (options, callback) { | ||
if (typeof options === 'function') { | ||
callback = options; | ||
options = {}; | ||
} | ||
|
||
if (!this.filename) { | ||
throw new Error('query() may not be used when initializing with a stream'); | ||
} | ||
|
||
var self = this; | ||
var results = []; | ||
var row = 0; | ||
options = self.normalizeQuery(options); | ||
|
||
var logFiles = (function () { | ||
var fileRegex = new RegExp(self.filename.replace('%DATE%', '.*'), 'i'); | ||
return fs.readdirSync(self.dirname).filter(function (file) { | ||
return path.basename(file).match(fileRegex); | ||
}); | ||
})(); | ||
|
||
if (logFiles.length === 0 && callback) { | ||
callback(null, results); | ||
} | ||
|
||
(function processLogFile(file) { | ||
if (!file) { | ||
return; | ||
} | ||
var logFile = path.join(self.dirname, file); | ||
var buff = ''; | ||
|
||
var stream = fs.createReadStream(logFile, { | ||
encoding: 'utf8' | ||
}); | ||
|
||
stream.on('error', function (err) { | ||
if (stream.readable) { | ||
stream.destroy(); | ||
} | ||
if (!callback) { | ||
return; | ||
} | ||
return err.code === 'ENOENT' ? callback(null, results) : callback(err); | ||
}); | ||
|
||
stream.on('data', function (data) { | ||
data = (buff + data).split(/\n+/); | ||
var l = data.length - 1; | ||
|
||
for (var i = 0; i < l; i++) { | ||
if (!options.start || row >= options.start) { | ||
add(data[i]); | ||
} | ||
row++; | ||
} | ||
|
||
buff = data[l]; | ||
}); | ||
|
||
stream.on('close', function () { | ||
if (buff) { | ||
add(buff, true); | ||
} | ||
|
||
if (options.order === 'desc') { | ||
results = results.reverse(); | ||
} | ||
|
||
if (logFiles.length) { | ||
processLogFile(logFiles.shift()); | ||
} else if (callback) { | ||
callback(null, results); | ||
} | ||
}); | ||
|
||
function add(buff, attempt) { | ||
try { | ||
var log = JSON.parse(buff); | ||
if (check(log)) { | ||
push(log); | ||
} | ||
} catch (e) { | ||
if (!attempt) { | ||
stream.emit('error', e); | ||
} | ||
} | ||
} | ||
|
||
function check(log) { | ||
if (!log || typeof log !== 'object') { | ||
return; | ||
} | ||
|
||
var time = new Date(log.timestamp); | ||
if ((options.from && time < options.from) || (options.until && time > options.until)) { | ||
return; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
function push(log) { | ||
if (options.rows && results.length >= options.rows && options.order !== 'desc') { | ||
if (stream.readable) { | ||
stream.destroy(); | ||
} | ||
return; | ||
} | ||
|
||
if (options.fields) { | ||
var obj = {}; | ||
options.fields.forEach(function (key) { | ||
obj[key] = log[key]; | ||
}); | ||
log = obj; | ||
} | ||
|
||
if (options.order === 'desc') { | ||
if (results.length >= options.rows) { | ||
results.shift(); | ||
} | ||
} | ||
results.push(log); | ||
} | ||
})(logFiles.shift()); | ||
}; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little concerned about this function name. Streams already expose a
close
event and this feels ripe for confusion. For my part (not a stream expert by any stretch), I had to go and look at the core NodeJSstream
andwinston-transport
source code to verify that this wasn't overwriting a pre-existingclose()
function.If this is specifically for
winston-daily-rotate-rotate
(WDRF) would it be possible to sidestep any such confusion and call it something WDRF-specific? Something likeendRotation()
orendLogging()
orcloseTransport()
?@indexzero This raises an interesting question. Is there a set of functions (hooks?) defined in
winston-transport
that cover setup and teardown? Or at least a convention by which Transports can name basic functionality like this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericdrobinson
FWIW, I did the same thing.
However, I think it does have meaning to
winston
. In the area I explored with the rotation bug -- Inwinston-transport
-- theunpipe
handler calls close on theTransport
if it is defined: (https://github.com/winstonjs/winston-transport/blob/master/index.js#L49-L62)Perhaps this just needs some clarity in the documentation -- maybe this is also the "most correct" place for transports to call
this.end()" and do additional cleanup if necessary since
_final()` is not an option in node versions < 8.0. I'll add more thoughts about that to thethread about _final()main PR conversation