Skip to content

Conversation

@chris-rudmin
Copy link
Contributor

No description provided.

@adamhadani
Copy link
Contributor

👍
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
Copy link

👍

@rosshettel
Copy link

👍 Yeah, this would be great to have!

@adamhadani
Copy link
Contributor

@indexzero anything stopping merging this PR in?

@srt32
Copy link

srt32 commented Oct 20, 2015

👍

indexzero added a commit that referenced this pull request Oct 20, 2015
@indexzero indexzero merged commit 9e27212 into winstonjs:master Oct 20, 2015
@bradfol
Copy link
Contributor

bradfol commented Nov 11, 2015

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.

bradfol added a commit to bradfol/winston-loggly that referenced this pull request Nov 11, 2015
@bradfol bradfol mentioned this pull request Nov 11, 2015
@indexzero
Copy link
Member

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

@chris-rudmin
Copy link
Contributor Author

@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 added a commit to bradfol/winston-loggly that referenced this pull request Nov 12, 2015
This reverts commit 9e27212, reversing
changes made to f1f1743.

# Conflicts:
#	README.md
#	lib/winston-loggly.js
@bradfol
Copy link
Contributor

bradfol commented Nov 12, 2015

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

indexzero added a commit that referenced this pull request Nov 13, 2015
Revert "Merge pull request #30 from chris-rudmin/master"
OtterCode pushed a commit to OtterCode/winston-loggly that referenced this pull request Jun 22, 2018
Error message support in winston-loggly-bulk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants