Add TCP support with option {tcp: true}
#2
Conversation
| // https://github.com/t0xa/gelfj/pull/61 | ||
| this.tcpClient.write(JSON.stringify(this.convert(data)) + '\0', callback) | ||
| } else if (this.tcpConnecting) | ||
| setTimeout(retrySend, 10) |
There was a problem hiding this comment.
This may result in race conditions and your data will be sent out of order. Eg:
- say
tcpConnectingistrueandtcpConnectedisfalse _sendTcpis called with some data, sosetTimeoutwill retry in 10ms- in that time,
tcpConnectedis set totrue - then let's say
_sendTcpis called with new data – that will now be sent first - then when the
setTimeouttriggers, the first packet of data will be sent second
One way to avoid this is to create a queue – it could just be an array of bound callbacks – and then call each of these on connect.
I guess the other thing to do is just ignore it – and live with the fact that out of order messages may occur 😸
There was a problem hiding this comment.
If I put the messages through convert before delaying them then they'd be timestamped in log order, even if they get sent out of order. That'd probably be OK since UDP doesn't guarantee delivery order either. What do you think?
There was a problem hiding this comment.
Ah yeah – great point. No idea if the Graylog server reorders them like it does with UDP, but in any case it's a pretty easy solution.
|
Cool! Thanks for looking into this – I haven't looked at or used this library in years. Everything seems pretty reasonable – just some minor comments – lemme know what you think. |
|
Also, would you be able to add a paragraph to the README? Basically something along the lines of this PR description would be perfect. |
|
I moved the Thanks for looking this over! |
| if (callback == null) callback = function() {} | ||
|
|
||
| var send = this.tcp ? this._sendTcp : this._sendUdp | ||
| send.call(this, this.convert(data), callback) |
There was a problem hiding this comment.
Hmmm – I'm just looking at this and it looks like you're changing the signature for this?
send used to be able to accept a Buffer and an array of Buffers (as well as a message object which would be converted) – see https://github.com/mhart/gelfling/blob/master/gelfling.js#L25
Now you're calling this.convert(data) which will break in those cases.
There was a problem hiding this comment.
Hmm, that's true. I'll have to rethink that.
This is something I'm using and thought it might be welcome upstream.
Passing the option
{tcp: true}establishes a TCP connection with the givenhostandport. Gzipping is disabled with TCP because it isn't supported properly by graylog. TCP GELF messages are NUL-delimited, and the gzip stream contains NULs, which confuses the server.If this is used with gelf-stream and bunyan, an
on('error', ...)handler must be bound to the gelf-stream stream because bunyan does not capture them (e.g.ECONNREFUSED). Disabling the stream (removing from bunyan) must also be handled manually, presumably in the error handler. The gelfling stream will attempt to connect any time a message is written to it and the connection is not open. That seemed more or less in keeping with the behavior of the UDP messaging.