Skip to content
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

cannot add loggly transport to the newest winston #32

Open
piotr-s-brainhub opened this issue Jan 5, 2018 · 34 comments
Open

cannot add loggly transport to the newest winston #32

piotr-s-brainhub opened this issue Jan 5, 2018 · 34 comments
Assignees

Comments

@piotr-s-brainhub
Copy link

@piotr-s-brainhub piotr-s-brainhub commented Jan 5, 2018

Node: 8.6.0
npm: 5.2.0
OS: Mac OS X 10.12.6
winston: 3.0.0-rc1
winston-loggly-bulk: 2.0.2

I'm trying to use winston-loggly-bulk as in the README.md and it doesn't work.

const winston = require('winston');
require('winston-loggly-bulk');

const options = {
  inputToken: process.env.LOGGLY_TOKEN,
  subdomain: process.env.LOGGLY_ORG,
};
winston.add(winston.transports.Loggly, options);

I obtain:

/Users/foo/programming/my-logger/node_modules/winston-transport/legacy.js:17
    throw new Error('Invalid transport, must be an object with a log method.');
    ^

Error: Invalid transport, must be an object with a log method.
    at new LegacyTransportStream (/Users/foo/programming/my-logger/node_modules/winston-transport/legacy.js:17:11)
    at DerivedLogger.add (/Users/foo/programming/my-logger/node_modules/winston/lib/winston/logger.js:277:7)
    at Object.winston.(anonymous function) [as add] (/Users/foo/programming/my-logger/node_modules/winston/lib/winston.js:84:34)
    at Object.<anonymous> (/Users/foo/programming/my-logger/1.js:8:9)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Function.Module.runMain (module.js:665:10)
@piotr-s-brainhub
Copy link
Author

@piotr-s-brainhub piotr-s-brainhub commented Jan 5, 2018

Fixed in winston-loggly repo winstonjs#53

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jan 5, 2018

Is it possible you are using the new keyword when instantiating the winston transport? If so, see the last troubleshooting step in our documentation https://www.loggly.com/docs/node-js-logs/. You might have to add "true" as the third parameter in your add function.

@piotr-s-brainhub
Copy link
Author

@piotr-s-brainhub piotr-s-brainhub commented Jan 5, 2018

for winston.add(winston.transports.Loggly, options, true); I have the same error

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jan 5, 2018

Okay thanks I think we need to have an engineer test this out

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jan 8, 2018

Hi @piotr-s-brainhub, with winston 3.0.0-rc1, I was able to reproduce the issue reported by you.

JFYI, You are using winston's latest version i.e. 3.0.0-rc1 which is currently not a stable version of winston but a release candidate and is also not tested by us. The current stable version that we are compatible with and tested is winston 2.4.0. I will recommend you to check with winston 2.4.0 for now.

cc: @mostlyjason

@dhaspden
Copy link

@dhaspden dhaspden commented Feb 2, 2018

I have the issue described in the original post. I upgraded winston to version 2.4.0 and my build broke. When I remove the Loggly transport my build succeeds. Not sure what the issue is but something between 2.3.1 and 2.4.0 changed that caused this library to stop working for me.

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Feb 21, 2018

Hi @dhaspden, I released a new beta package for winston-loggly-bulk library which internally install node-loggly-bulk. Can you please give a try with that beta package and let me know if the issue is resolved for you? If not, then please report me back with your complete stacktrace of the error you are getting.

Use below command to install the new beta package:

npm install winston-loggly-bulk@beta

I'll wait for your response.

cc: @mchaudhary

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Mar 8, 2018

Hi @piotr-s-brainhub and @dhaspden, did you get some time to test the above beta package that I published some time before?

I want to listen from you to see if the new release resolves your issue. Waiting for your reply.

@dhaspden
Copy link

@dhaspden dhaspden commented Mar 8, 2018

Hi @Shwetajain148 I'll carve some time out at lunch to give it a shot. Standby.

@piotr-s-brainhub
Copy link
Author

@piotr-s-brainhub piotr-s-brainhub commented Mar 8, 2018

I don't have enough time to check it.
In our project we forked https://github.com/winstonjs/winston-loggly because they hadn't merged my PR winstonjs#53 (no response at all) but this fork is a private repo.

@dhaspden
Copy link

@dhaspden dhaspden commented Mar 9, 2018

Okay sorry about the delay. I tried the beta version and got mixed results. Firstly I tried using the transport as normal... The first set of errors is using winston@2.4.0, the latest stable version.

// log is created using new winston.Logger();
log.add(winston.transports.Loggly, {
  json: true,
  level: process.env.LOGGLY_LOG_LEVEL,
  subdomain: process.env.LOGGLY_SUBDOMAIN,
  token: process.env.LOGGLY_KEY
});

When I do this I get the following error:

  var instance = created ? transport : (new (transport)(options));
                                        ^

TypeError: transport is not a constructor
    at exports.Logger.Logger.add (/path/to/project/node_modules/winston/lib/winston/logger.js:481:41)
    at getLogger (/path/to/project/config/log.config.js:42:9)
    at Object.<anonymous> (/path/to/project/index.js:9:43)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Module.require (module.js:587:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/path/to/project/bin/index.js:4:21)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Function.Module.runMain (module.js:684:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3
[nodemon] app crashed - waiting for file changes before starting...

I was able to get it working by using the instance directly from the module...

const loggly = require('winston-loggly-bulk');
log.add(loggly.Loggly, {
  json: true,
  level: process.env.LOGGLY_LOG_LEVEL,
  subdomain: process.env.LOGGLY_SUBDOMAIN,
  token: process.env.LOGGLY_KEY
});

Using winston@3.0.0-rc1 I received the following error...

/Users/dhaspden/Documents/code/rankhigherlms/server/node_modules/winston-transport/legacy.js:17
    throw new Error('Invalid transport, must be an object with a log method.');
    ^

Error: Invalid transport, must be an object with a log method.
    at new LegacyTransportStream (/path/to/project/node_modules/winston-transport/legacy.js:17:11)
    at DerivedLogger.add (/path/to/project/node_modules/winston/lib/winston/logger.js:277:7)
    at getLogger (/path/to/project/config/log.config.js:36:9)
    at Object.<anonymous> (/path/to/project/index.js:9:43)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Module.require (module.js:587:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/path/to/project/bin/index.js:4:21)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Function.Module.runMain (module.js:684:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3
[nodemon] app crashed - waiting for file changes before starting...

I hope that is detailed enough. I can provide more details if you need them. Thanks!

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Mar 12, 2018

Hi @dhaspden, Thanks for your input. Let's first talk about the first error that you mentioned with latest stable version winston@2.4.0. I would like to tell you that if you are instantiating the Loggly transporter using new keyword then you must add that Loggly transporter into winston by passing 3 parameters like below-

winston.add(transportsObject, null, true);

Please refer the Advanced Logging Section of the Loggly NodeJs document here- https://www.loggly.com/docs/node-js-logs/.

Below is a sample code snippet to be more clear-

var winston  = require('winston');
require('winston-loggly-bulk');

var transportsObject = new winston.transports.Loggly({
    inputToken: "TOKEN",
    subdomain: "SUBDOMAIN",
    tags: ["nodejs"],
    json: true
});
 
winston.add(transportsObject, null, true);

winston.log('info', 'hey there..');

Note: For winston@3.0.0-rc, I would like to tell you that it's not tested by us yet and we are curently compatible with winston@2.4.0.

@shahidcodes
Copy link

@shahidcodes shahidcodes commented Mar 28, 2018

For some reason in node v8.9.4 the require("winston-loggly-bulk"); didn't expose Loggly to the winston.transaports. So we have to pass Loggly directly to winston.add.

var winston = require('winston');
var {Loggly} = require('winston-loggly-bulk');

winston.add(new Loggly({
    token: "[TOKEN]",
    subdomain: "[SUBDOMAIN]",
    tags: ["Winston-NodeJS"],
    json: true
}));


winston.log('info', "Hello World from Node.js!");

PS: Used version

{
"winston": "^2.4.1",
"winston-loggly-bulk": "^2.0.2"
}
@edorivai
Copy link

@edorivai edorivai commented Apr 10, 2018

Following works for us, we use typescript.

import { Loggly } from 'winston-loggly-bulk';

winston.add(Loggly, { ...options });
@Shwetajain148 Shwetajain148 self-assigned this May 22, 2018
@sabry2010
Copy link

@sabry2010 sabry2010 commented Jun 20, 2018

Node: 10.4.1
npm: 6.1.0
OS: ubuntu 14.04
winston: ^3.0.0

when I try to run node server.js in terminal I’m getting this error

/node_modules/winston-transport/legacy.js:18
throw new Error('Invalid transport, must be an object with a log method.');
^

Error: Invalid transport, must be an object with a log method.
at new LegacyTransportStream (/node_modules/winston-transport/legacy.js:18:11)
at DerivedLogger.add (/node_modules/winston/lib/winston/logger.js:245:9)
at Object.winston.(anonymous function).args [as add] (/node_modules/winston/lib/winston.js:101:55)
at Object. (/server.js:8:8)
at Module._compile (internal/modules/cjs/loader.js:702:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
at Module.load (internal/modules/cjs/loader.js:612:32)
at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
at Function.Module._load (internal/modules/cjs/loader.js:543:3)
at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)

can any person help me to fix this issue?

@OtterCode
Copy link

@OtterCode OtterCode commented Jun 20, 2018

@sabry2010 Hello! I'm currently working on an official fix for this. I've got it 90%, just cleaning up some tests. It should be finished today or tomorrow. I'll keep you posted.

@sabry2010
Copy link

@sabry2010 sabry2010 commented Jun 21, 2018

@OtterCode okay thank you waiting you

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jun 22, 2018

Hi @sabry2010, @OtterCode, Sorry for the delay in fix here, I was busy with some critical issues. JFYI, I am almost done with the initial support with winston 3.0.0 and going to share the details very soon to setup Loggly with the latest winston version.

Stay tuned. Thanks!

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jun 22, 2018

Hi @piotr-s-brainhub, @dhaspden, @shahidcodes, @edorivai, @sabry2010, @OtterCode Good news here!

Since winston-v3.0.0 changes, we have also made some modification to be compatible with winston@3. We created a different branch winston-loggly-bulk@3.x to keep the new changes there. Please go through the instructions in the README file for some new syntax changes.

See the usage here- https://github.com/loggly/winston-loggly-bulk/tree/3.x#usage
See a sample example to create your Loggly logger- https://github.com/loggly/winston-loggly-bulk/tree/3.x#sample-working-code-snippet

You can setup the winston-loggly-bulk library version 3.0.0-rc1 with winston 3.0.0 compatible using the below command-

npm install https://github.com/loggly/winston-loggly-bulk.git#3.x --save

Note: Please keep in mind that winston@3 has increased the nodejs runtime version to minimum V6 which was before this. Now you must have node V6 installed to setup this library or you have to upgrade your node version.

Do setup the library on new version and let me know your feedback/issues so that I can work on them. Please feel free to report bugs. PRs are always welcome.

Thanks!

@OtterCode
Copy link

@OtterCode OtterCode commented Jun 22, 2018

@Shwetajain148 Did you end up using my PR?

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jun 22, 2018

Cool thanks for sharing @Shwetajain148 can you test the new branch @OtterCode? Btw, @OtterCode is writing a blog on winston 3 support.

@sabry2010
Copy link

@sabry2010 sabry2010 commented Jun 22, 2018

@Shwetajain148 I follow all instructions and still I get issues

loggly is a legacy winston transport. Consider upgrading:

  • Upgrade docs: https://github.com/winstonjs/winston/blob/master/UPGRADE-3.0.md
    Error: Loggly Error (403): Forbidden
    at Request._callback (/node_modules/node-loggly-bulk/lib/loggly/common.js:201:31)
    at Request.self.callback (/node_modules/request/request.js:185:22)
    at Request.emit (events.js:182:13)
    at Request. (/node_modules/request/request.js:1157:10)
    at Request.emit (events.js:182:13)
    at IncomingMessage. (node_modules/request/request.js:1079:12)
    at Object.onceWrapper (events.js:273:13)
    at IncomingMessage.emit (events.js:187:15)
    at endReadableNT (_stream_readable.js:1081:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)
@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jun 23, 2018

Hi @sabry2010, Please correct your Loggly Customer Token. The library throws 403 Forbidden in case of wrong customer token used. You can find the customer token at- https://SUBDOMAIN.loggly.com/tokens

Replace: SUBDOMAIN with your Loggly account subdomain.

Hopefully, this will resolve your issue.

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jun 23, 2018

Hi @OtterCode, I did not use your PR. In-fact, I just saw it now.

Hi @mostlyjason, sorry it's a bit confusing. Are you asking me or @OtterCode to test the new branch? BTW, great to know that he's writing a blog.

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jun 25, 2018

@Shwetajain148, @OtterCode is going to try to merge his changes from #39 into your branch. I'm then hoping you'll be able to test it. If you have more changes, can you add them tonight or hold until he's done so we don't create any merge conflicts?

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jun 27, 2018

@OtterCode, Can you confirm when you are going to merge your changes into my branch?

@OtterCode
Copy link

@OtterCode OtterCode commented Jun 27, 2018

@Shwetajain148 The pull request has been up for a while. You can merge it if it looks good to you.

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jun 28, 2018

He submitted #42 but I think he needs to fix some merge conflicts first

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jun 28, 2018

Fixed the merge conflicts and added #43. @Shwetajain148 can you test?

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jun 29, 2018

@mostlyjason @OtterCode, Yes, I'll test and will let you know how it goes. Thanks.

@trieloff
Copy link

@trieloff trieloff commented Jul 2, 2018

I've been trying both @Shwetajain148 and @OtterCode and @mostlyjason's #43 with an empty project:

$ mkdir scratch
$ cd scratch
$ npm init
This utility will walk you through creating a package.json file.
It only covers the most common items, and tries to guess sensible defaults.

See `npm help json` for definitive documentation on these fields
and exactly what they do.

Use `npm install <pkg>` afterwards to install a package and
save it as a dependency in the package.json file.

Press ^C at any time to quit.
package name: (scratch)
version: (1.0.0)
description:
entry point: (index.js)
test command:
git repository:
keywords:
author:
license: (ISC)
About to write to /Users/lars/Code/scratch/package.json:

{
  "name": "scratch",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC"
}


Is this OK? (yes)
$ npm install --save "mostlyjason/winston-loggly-bulk#ottercode"
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN scratch@1.0.0 No description
npm WARN scratch@1.0.0 No repository field.

+ winston-loggly-bulk@3.0.0-rc1
added 80 packages from 113 contributors and audited 119 packages in 5.777s
found 0 vulnerabilities

$ npm install --save winston
npm WARN scratch@1.0.0 No description
npm WARN scratch@1.0.0 No repository field.

+ winston@3.0.0
updated 1 package and audited 163 packages in 1.676s
found 0 vulnerabilities

$ micro scratch.js
$ node --version
v8.8.1
$ node scratch.js
loggly is a legacy winston transport. Consider upgrading:
- Upgrade docs: https://github.com/winstonjs/winston/blob/master/UPGRADE-3.0.md

After the initial debug message it just sits there for another 20 seconds.

My scratch.js looks like this:

var winston  = require('winston');
var {Loggly} = require('winston-loggly-bulk');

const l = new Loggly({
  token: "<secret>",
  subdomain: "trieloff"
});

winston.add(l);

winston.log('info', "Hello World from Node.js!");

So I'd say it does not work for me. Going through the test cases I could not find an example where the transport is called with winston (it's always tested standalone) and it looks like Winston assumes the implementation to be a legacy implementation.

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jul 3, 2018

Hi @trieloff, I just replicated all the steps that you performed and I didn't see any problem my event logged to Loggly successfully. Did you search for your event in your Loggly account? I think you are getting confused with the termination of the command once the event has been logged because there is no other event to be logged. This is default library behavior to not let any open handler keep on running.

Also, I noticed that you ran the command to install the winston package which is not a required step here since when you run the command to install the winston-loggly-bulk package, it internally fetches the winston package.

If you setup your scratch.js file to continue logging events at a specified interval then you will notice that your app is keep on running and events are being sent to Loggly. if you don't mind can you give a try with the below code snippet-

var winston = require('winston');
var { Loggly } = require('winston-loggly-bulk');

const l = new Loggly({
    token: "<secret>",
    subdomain: "trieloff",
    json: true
});

winston.add(l);

setInterval(function () {
    sendMultipleLogs();
}, 5 * 1000);

function sendMultipleLogs() {
    for (i = 0; i < 5; i++) {
        winston.log('info', "Hello World from Node.js!");
    }
}

The above code will log 5 events in each 5 seconds. In anyway, if you log a single event, it will be logged to Loggly without any issue. Please let me know if you see any error/issue.

Thanks.

@OtterCode
Copy link

@OtterCode OtterCode commented Jul 3, 2018

@Shwetajain148 It's a legacy transport because the Loggly.log function accepts too many arguments. There's an undocumented update in Winston 3.x that requires the log function to be written with only two arguments: info, and callback.

For the current winston-loggly-bulk, this means that there's a huge amount of code that needs cutting, since Winston now takes responsibility for it. It still works in legacy mode for the moment, but it's something that needs doing sooner or later.

@kimown
Copy link

@kimown kimown commented Sep 2, 2018

Debugger attached.
loggly is a legacy winston transport. Consider upgrading: 
- Upgrade docs: https://github.com/winstonjs/winston/blob/master/UPGRADE-3.0.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.