Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Could you please review changes to winston-syslog. #3

Merged
merged 4 commits into from

3 participants

@thurmda

I made some changes that provide my use case the expected functionality, however I don't understand the majority of the connect method. I provided inline comments accompanying the commit in github. I also added nodeunit test simply because i'm unsure how to do the same with vows.

Here are a few items that I just don't get about the connect method:
1) If syslogd is listening on localhost:514 how would this socket ever bind?
2) If you want to send messages why would you bind and listen on a port?
3) What's the point of binding and not handling 'message' events?

PS If I didn't follow coding conventions It's simply an oversight, just point me in the right direction and I'll update.

@thurmda

Without this change messages appear in log files as ": [object Object]"

@thurmda

Tracking messages that are 'in flight' = they have been sent but the send(... callback) hasn't fired yet.

@thurmda

A node process will hang due to this unclosed socket. This close method makes 6 graceful attempts to close the socket before forcefully closing.

@thurmda

I might be missing something critical here, but for my uses case (sending syslog messages to the local syslogd) the majority of this method is causing problems for me. Here are a few items that I just don't get...

1) If syslogd is listening on localhost:514 how would this socket ever bind?
2) If you want to send messages why would you bind and listen on a port?
3) What's the point of binding and not handling 'message' events?

@thurmda

I'm not familiar with testing async stuff in vows. These nodeunit tests demonstrate what I think are useful tests. I'm interested to know how to do the same in vows...

@thurmda

I don't know what's going on here but I think the issue is in winston. After setting the log levels 'debug' no longer works?...

@thurmda

called after each message has actually been sent.

@thurmda

This handles the 'closed' event raised by the newly added Syslog.prototype.close (this is called by winston.remove()). The 'closed' event fires after all messages have been sent.

thurmda added some commits
@thurmda thurmda util.inspect -> JSON.stringify to keep on single line to make syslog …
…happy.
8c9c9aa
@thurmda thurmda Glossy takes a app_id parameter to help identify the \"App\" that is …
…logging messages. Adding support for this. You can pass an app_id in options when calling the constructor or let it default to the process.title
f9b706e
@thurmda thurmda cleaning up unused code 6e875c7
@thurmda thurmda commented on the diff
lib/winston-syslog.js
((5 lines not shown))
//
// Setup connection state
//
this.connected = false;
this.retries = 0;
this.queue = [];
+ this.inFlight = 0;
@thurmda
thurmda added a note

used to track calls that have been made but not yet completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thurmda thurmda commented on the diff
lib/winston-syslog.js
@@ -87,6 +88,10 @@ util.inherits(Syslog, winston.Transport);
winston.transports.Syslog = Syslog;
//
+// Expose the name of this Transport on the prototype
+//
+Syslog.prototype.name = 'Syslog';
@thurmda
thurmda added a note

FOllowing convention of other winston sub libs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thurmda thurmda commented on the diff
lib/winston-syslog.js
((5 lines not shown))
date: new Date(),
- message: data
+ message: JSON.stringify(data)
@thurmda
thurmda added a note

With data vs JSON.stringify(data) objects appear in syslogs as [object]...

@squeeks
squeeks added a note

Should you really be serialising your message as JSON? Unless you're explicitly using BSD/RFC 3164 style messages, using syslog's own structured data format is probably a better idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thurmda thurmda commented on the diff
lib/winston-syslog.js
@@ -109,8 +114,9 @@ Syslog.prototype.log = function (level, msg, meta, callback) {
syslogMsg = this.producer.produce({
severity: level,
host: this.localhost,
+ app_id: this.app_id || process.title,
@thurmda
thurmda added a note

without this this field in syslogs appears as 'undefined'. This allows for more meaningful logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thurmda thurmda commented on the diff
lib/winston-syslog.js
@@ -151,7 +159,24 @@ Syslog.prototype.log = function (level, msg, meta, callback) {
callback(null, true);
};
-
+//
+// ### function close ()
+// Closes the socket used by this transport freeing the resource.
+//
+Syslog.prototype.close = function () {
@thurmda
thurmda added a note

Handling the close event fired when transport is removed. Without this my tests hang due to the open socket, also kinda nice to free resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thurmda thurmda commented on the diff
lib/winston-syslog.js
((14 lines not shown))
- //
@thurmda
thurmda added a note

I might be missing something major here but... I don't get most of this connect method.

For my uses case (sending syslog messages to the local syslogd) the majority of this method is causing problems for me. Here are a few items that I just don't get...

1) If syslogd is listening on localhost:514 how would this socket ever bind?
2) If you want to send messages why would you bind and listen on a port?
3) What's the point of binding and not handling 'message' events?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davehorton

ditto....I was expecting to be able to log to syslogd on localhost, the connect method as currently written seems to exclude that use case. Am I missing something? Was there a resolution/workaround?

...Oh, my bad -- I see those changes are here; I had done an npm install to get it before and apparently they never made it over there (though the version numbers in package.json are the same the code is different)

@thurmda thurmda merged commit 6e875c7 into winstonjs:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 13, 2012
  1. @thurmda
Commits on Feb 17, 2012
  1. @thurmda
  2. @thurmda

    Glossy takes a app_id parameter to help identify the \"App\" that is …

    thurmda authored
    …logging messages. Adding support for this. You can pass an app_id in options when calling the constructor or let it default to the process.title
Commits on Feb 25, 2012
  1. @thurmda

    cleaning up unused code

    thurmda authored
This page is out of date. Refresh to see the latest.
Showing with 93 additions and 65 deletions.
  1. +33 −65 lib/winston-syslog.js
  2. +60 −0 test/nodeunitTests.js
View
98 lib/winston-syslog.js
@@ -31,13 +31,13 @@ var levels = Object.keys({
//
var Syslog = exports.Syslog = function (options) {
options = options || {};
-
//
// Setup connection state
//
this.connected = false;
this.retries = 0;
this.queue = [];
+ this.inFlight = 0;
@thurmda
thurmda added a note

used to track calls that have been made but not yet completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
//
// Merge the options for the target Syslog server.
@@ -45,6 +45,7 @@ var Syslog = exports.Syslog = function (options) {
this.host = options.host || 'localhost';
this.port = options.port || 514;
this.path = options.path || null;
+ this.app_id = options.app_id || null;
this.protocol = options.protocol || 'udp4';
this.isDgram = /^udp|unix/.test(this.protocol);
@@ -87,6 +88,10 @@ util.inherits(Syslog, winston.Transport);
winston.transports.Syslog = Syslog;
//
+// Expose the name of this Transport on the prototype
+//
+Syslog.prototype.name = 'Syslog';
@thurmda
thurmda added a note

FOllowing convention of other winston sub libs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+//
// ### function log (level, msg, [meta], callback)
// #### @level {string} Target level to log to
// #### @msg {string} Message to log
@@ -109,8 +114,9 @@ Syslog.prototype.log = function (level, msg, meta, callback) {
syslogMsg = this.producer.produce({
severity: level,
host: this.localhost,
+ app_id: this.app_id || process.title,
@thurmda
thurmda added a note

without this this field in syslogs appears as 'undefined'. This allows for more meaningful logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
date: new Date(),
- message: data
+ message: JSON.stringify(data)
@thurmda
thurmda added a note

With data vs JSON.stringify(data) objects appear in syslogs as [object]...

@squeeks
squeeks added a note

Should you really be serialising your message as JSON? Unless you're explicitly using BSD/RFC 3164 style messages, using syslog's own structured data format is probably a better idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
});
//
@@ -130,6 +136,7 @@ Syslog.prototype.log = function (level, msg, meta, callback) {
function onError (logErr) {
if (logErr) { self.queue.push(syslogMsg) }
self.emit('logged');
+ self.inFlight--;
}
//
@@ -137,7 +144,8 @@ Syslog.prototype.log = function (level, msg, meta, callback) {
//
if (self.isDgram) {
buffer = new Buffer(syslogMsg);
- if (self.protocol.match(/^udp/)) {
+ if (self.protocol.match(/^udp/)) {
+ self.inFlight++;
self.socket.send(buffer, 0, buffer.length, self.port, self.host, onError);
}
else {
@@ -151,7 +159,24 @@ Syslog.prototype.log = function (level, msg, meta, callback) {
callback(null, true);
};
-
+//
+// ### function close ()
+// Closes the socket used by this transport freeing the resource.
+//
+Syslog.prototype.close = function () {
@thurmda
thurmda added a note

Handling the close event fired when transport is removed. Without this my tests hang due to the open socket, also kinda nice to free resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ var self = this,
+ max = 6,
+ attempt = 0;
+ (function _close(){
+ if(attempt>=max || (self.queue.length === 0 && self.inFlight <= 0)){
+ self.socket.close();
+ self.emit('closed');
+ }else{
+ attempt++
+ setTimeout(_close, 200 * attempt);
+ }
+ }());
+}
//
// ### function connect (callback)
// #### @callback {function} Continuation to respond to when complete.
@@ -165,6 +190,7 @@ Syslog.prototype.connect = function (callback) {
// If the socket already exists then respond
//
if (this.socket) {
+ return callback(null);// Only writing on the socket...
return this.socket.readyState === 'open'
? callback(null)
: callback(true);
@@ -191,72 +217,14 @@ Syslog.prototype.connect = function (callback) {
function onError (logErr) {
if (logErr) { self.emit('error', logErr) }
self.emit('logged');
+ self.inFlight--;
}
//
// Indicate to the callee that the socket is not ready. This
// will enqueue the current message for later.
//
- callback(true);
+ callback();
+
- //
@thurmda
thurmda added a note

I might be missing something major here but... I don't get most of this connect method.

For my uses case (sending syslog messages to the local syslogd) the majority of this method is causing problems for me. Here are a few items that I just don't get...

1) If syslogd is listening on localhost:514 how would this socket ever bind?
2) If you want to send messages why would you bind and listen on a port?
3) What's the point of binding and not handling 'message' events?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- // Listen to the appropriate events on the socket that
- // was just created.
- //
- this.socket.on(readyEvent, function () {
- //
- // When the socket is ready, write the current queue
- // to it.
- //
- if (self.isDgram) {
- var buffer = new Buffer(self.queue.join(''));
- if (self.protocol.match(/^udp/)) {
- self.socket.send(buffer, 0, buffer.length, self.port, self.host, onError);
- }
- else {
- self.socket.send(buffer, 0, buffer.length, self.path, onError);
- }
- }
- else {
- self.socket.write(self.queue.join(''), 'utf8', onError);
- }
-
- self.emit('logged');
- self.queue = [];
- self.retries = 0;
- self.connected = true;
- }).on('error', function (ex) {
- //
- // TODO: Pass this error back up
- //
- }).on('end', function (ex) {
- //
- // Nothing needs to be done here.
- //
- }).on('close', function (ex) {
- //
- // Attempt to reconnect on lost connection(s), progressively
- // increasing the amount of time between each try.
- //
- var interval = Math.pow(2, self.retries);
- self.connected = false;
-
- setTimeout(function () {
- self.retries++;
- self.socket.connect(self.port, self.host);
- }, interval * 1000);
- }).on('timeout', function () {
- if (self.socket.readyState !== 'open') {
- self.socket.destroy();
- }
- });
-
- if (self.protocol.match(/^udp/)) {
- return this.socket.bind(this.port, this.host);
- }
- else if (self.protocol.match(/^unix/)){
- return this.socket.bind(this.path);
- }
-
- this.socket.connect(this.port, this.host);
};
View
60 test/nodeunitTests.js
@@ -0,0 +1,60 @@
+/*
+ *Nodeunit tests (tests some async behaviors)
+ *Dan Thurman
+ *
+ *
+*/
+var winston = require('winston'),
+ helpers = require('winston/test/helpers'),
+ Syslog = require('../lib/winston-syslog').Syslog;
+
+exports.basicTypeChecking = function(test){
+ var transport = new Syslog();
+ test.expect(5);
+ test.ok(transport instanceof Syslog);
+ test.ok(transport.log instanceof Function);
+ test.ok(transport.connect instanceof Function);
+ test.ok(transport.close instanceof Function);
+
+ var logger = new winston.Logger({transports: [transport]});
+ test.ok(logger.transports.Syslog instanceof Syslog);
+ test.done();
+};
+
+exports.sendsMessages = function(test){
+ var transport = new Syslog(),
+ logger = new winston.Logger({
+ transports: [transport],
+ levels: winston.config.syslog.levels
+ });
+// var levels = Object.keys(winston.config.syslog.levels),
+ var levels = ['info', 'notice', 'warning', 'error', 'alert', 'crit', 'emerg']; //debug doesn't work after setting levels...?
+ test.expect(levels.length);
+
+ logger.transports.Syslog.on('logged', function(){
+ test.ok(1);
+ });
+
+ levels.forEach(function(level){
+ logger.log(level, 'Test '+level+' Message ' + Date.now());
+ });
+
+ transport.on('closed', function(){
+ test.done();
+ });
+ logger.remove(winston.transports.Syslog);
+};
+
+exports.releasesResources = function(test){
+ test.expect(1);
+ var transport = new winston.transports.Syslog(),
+ logger = new winston.Logger({
+ transports: [transport]
+ });
+ logger.log('debug', 'Test message to actually use socket');
+ transport.on('closed', function(){
+ test.ok(1);
+ test.done()
+ });
+ logger.remove(winston.transports.Syslog);
+}
Something went wrong with that request. Please try again.