search

winstonjs / winston-loggly

A Loggly transport for winston
http://github.com/indexzero/winston-loggly
MIT License
85 stars 110 forks source link

Add support for host #30

Closed chris-rudmin closed 8 years ago

adamhadani commented 9 years ago

:+1: a bit surprised native support for including a host/ip isn't already available for this integration, absolutely needed in a load-balanced/distributed environment, esp. if underlying 'loggly' node.js library supports this argument already

Phylodome commented 9 years ago

:+1:

rosshettel commented 8 years ago

:+1: Yeah, this would be great to have!

adamhadani commented 8 years ago

@indexzero anything stopping merging this PR in?

srt32 commented 8 years ago

:+1:

bradfol commented 8 years ago

Hey all, I was tripped up by the README on this. The host option does not allow a hostname to be sent with the logs. It is actually the destination for the logs to be sent to.

Setting it to os.hostname() as I did will silently break communication with Loggly, which is bad.

The default is logs-01.loggly.com, and I'm not aware of a use case to change that. https://github.com/nodejitsu/node-loggly/blob/master/lib/loggly/client.js#L52

I would suggest reverting this, as it is breaks the transport silently. At a minimum, I can submit a PR to correct the README.

indexzero commented 8 years ago

@bradfol thanks for finding this. An update to tests with a subsequent PR would be very helpful.

chris-rudmin commented 8 years ago

@bradfol @indexzero I misunderstood the purpose of host. Thanks for caching this. I would suggest undoing my PR, as there is not really a good reason to configure this. Apologies for troubles caused.

bradfol commented 8 years ago

@indexzero @chris-rudmin Thanks guys. I've opened PR #35 to revert this.