winstonjs / winston

A logger for just about everything.
http://github.com/winstonjs/winston
MIT License
22.83k stars 1.81k forks source link

[3.0.0] Error object is not parsed or printed #1338

Closed chriswiggins closed 5 years ago

chriswiggins commented 6 years ago

Please tell us about your environment:

What is the problem?

Logging a node Error object results in an empty message:

Example:

const winston = require('winston');
const { createLogger, format, transports } = winston;

const logger = createLogger({
  transports: [
    new transports.Console()
  ]
});

let err = new Error('this is a test');
logger.log({level: 'error', message: err});

Resulting output:

% node test.js
{"level":"error","message":{}}

Also:

logger.error(new Error('hello'))

Results in:

{"level":"error"}

What do you expect to happen instead?

I Expect the message key to have at least the error message included in it. If I try a custom formatter, info also doesn't have the error object in it so it must be stripped out somewhere?

Other information

Let me know how I can help - happy to flick a PR but I don't know my way around winston@3.0.0 enough yet to find it

indexzero commented 6 years ago

We have some test coverage for this, but clearly we need more. What's going on under the covers:

  1. Your Error instance is passed along the objectMode stream pipe-chain
  2. The default format for Logger is json (see: json format code in logform)
  3. message and stack properties on Error are non-enumerable which causes JSON.stringify to output something that one does not expect.
console.log(JSON.stringify(new Error('lol nothing here')));
// '{}'

From a design perspective winston@3 introduced formats for exactly this kind of problem to increase performance. Speaking of performance, interestingly enough pino does something interesting here. Perhaps the solution is implementing something similar to asJson in the default json format.

indexzero commented 6 years ago

If anyone is looking for a quick work-around you can include enumerateErrorFormat in your logger's format for now. We will hopefully have a fix for this out before 3.0.0 next week (or shortly after in 3.0.1)

const winston = require('../');
const { createLogger, format, transports } = winston;

const enumerateErrorFormat = format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack
    }, info.message);
  }

  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack
    }, info);
  }

  return info;
});

const logger = createLogger({
  format: format.combine(
    enumerateErrorFormat(),
    format.json()
  ),
  transports: [
    new transports.Console()
  ]
});

// Error as message
console.log('Run FIRST test...');
logger.log({ level: 'error', message: new Error('FIRST test error') });

// Error as info (one argument)
console.log('\nRun SECOND test...');
const err = new Error('SECOND test error');
err.level = 'info';
logger.info(err);

// Error as info (two arguments);
console.log('\nRun THIRD test...');
logger.log('info', new Error('THIRD test error'));
sandrocsimas commented 6 years ago

@indexzero, I tried to follow your workaround, but it's not working. Do you know why?

Formatter:

const level = settings.debug ? 'debug' : 'info';
const printFormat = winston.format.printf(info => `${info.timestamp} - ${info.level}: ${info.message}`);
const enumerateErrorFormat = winston.format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack,
    }, info.message);
  }
  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack,
    }, info);
  }
  return info;
});

const consoleLogger = winston.createLogger({
  level,
  format: winston.format.timestamp(),
  transports: [
    new winston.transports.Console({
      format: winston.format.combine(
        winston.format.colorize(),
        enumerateErrorFormat(),
        printFormat,
      ),
    }),
  ],
});

Code:

try {
 // Some code throwing error
} catch (err) {
  logger.error(err);
}

Output:

2018-06-28T21:17:25.140Z - error: undefined

Info object:

{ level: '\u001b[31merror\u001b[39m', timestamp: '2018-06-28T21:17:25.140Z', [Symbol(level)]: 'error' }

Where is the message attribute in the error log?

christopher-mazaudou commented 6 years ago

@sandrocsimas I noticed that you need to give the enumerateErrorFormat function to the default formatter of the logger in order to make it work.

Formatter

const consoleLogger = winston.createLogger({
  level,
  format: winston.format.combine(
    winston.format.timestamp(),
    enumerateErrorFormat()
  ),
  transports: [
    new winston.transports.Console({
      format: winston.format.combine(
        winston.format.colorize(),
        printFormat,
      ),
    }),
  ],
});

I still don't understand why tho

SamuelMaddox commented 6 years ago

I think I'm experiencing the same bug as @sandrocsimas.

Here is my logger config:

logger.js

const winston = require('winston');
const {configure, format} = winston;
const {combine, colorize, timestamp, printf} = format;

const enumerateErrorFormat = format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack
    }, info.message);
  }

  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack
    }, info);
  }

  return info;
});

const myConsoleFormat = printf(info => {
  console.log('** Info Object: **');
  console.log(info);
  console.log('** Winston Output: **');
  return `${info.level}: ${info.message}`;
});

winston.configure({
  transports: [
    new winston.transports.Console({
      format: combine(
        colorize(),
        enumerateErrorFormat(),
        myConsoleFormat
      ),
    })
  ]
});

If I test it with this block of code:

Test A

const logger = require('winston');
try {
  throw(new Error());
} catch (err) {
  logger.error(err);
}

where new Error() doesn't contain a message value, I get this output:

Output A

** Info Object: **
{ message: 
   { message: '',
     stack: 'Error\n    at Object.<anonymous> (app.js:21:9)\n    at Module._compile (module.js:652:30)\n    at Object.Module._extensions..js (module.js:663:10)\n    at Module.load (module.js:565:32)\n    at tryModuleLoad (module.js:505:12)\n    at Function.Module._load (module.js:497:3)\n    at Module.require (module.js:596:17)\n    at require (internal/module.js:11:18)\n    at Object.<anonymous> (server.js:11:13)\n    at Module._compile (module.js:652:30)' },
  level: '\u001b[31merror\u001b[39m',
  [Symbol(level)]: 'error',
  [Symbol(message)]: '{"message":{},"level":"error"}' }
** Winston Output: **
error: [object Object]

Where error: [object Object] is exactly what I expected

However, if I test it with this block of code:

Test B

const logger = require('winston');
try {
  throw(new Error('This causes error: undefined'));
} catch (err) {
  logger.error(err);
}

Where new Error() does contain a message value, I get this output:

Output B

** Info Object: **
{ level: '\u001b[31merror\u001b[39m',
  [Symbol(level)]: 'error',
  [Symbol(message)]: '{"level":"error"}' }
** Winston Output: **
error: undefined

As you can see I get the same error: undefined that @sandrocsimas gets. I expected to get error: [object Object]

Note, if I try this block of code:

Test C

const logger = require('winston');
try {
  throw(new Error('This should work'));
} catch (err) {
  logger.log({level: 'error', message: err});
}

Where I use logger.log instead of logger.error I get the same output as Output A above

nvtuan305 commented 6 years ago

I have the same problem. I'm new in winston. I tried @indexzero solution but not working. Do you have any solution?

SamuelMaddox commented 6 years ago

@nvtuan305, did you try @indexzero's solution exactly or did you edit it a bit? If so could you provide sample code? His code should work if you're using logger.log({level: ____, message: err}); It won't work if you're doing logger.info, logger.error, or any other flavor of logger.<level>. I'm almost certain that is a bug as I specified above and it should be fixed in a later release.

dandv commented 6 years ago

Am I missing something, or is it a complete headache (or even impossible?) to get the same output one easily gets from console.log/error/warn?

try {
   // ...
  throw new Error('foo');
} catch (e) {
  console.error('Caught error:', e);  // convenient, informative
  logger.error('Caught error:', e);  // nope, the second parameter is something else (couldn't find docs)
  logger.error(`Caught error: ${e}`);  // stack lost
  logger.error(`Caught error: ${JSON.stringify(e)}`);  // Caught error: {}
}

What is the equivalent winston code to get the same output as console.error('Caught error:', error);?

And where is the documentation for the parameters taken by the convenience methods on the logger object?

SamuelMaddox commented 6 years ago

@dandv

logger.error('Caught error:', e);

This doesn't work because, unlike console.log(), the winston's logger.<level>(message) takes only one parameter called message. That message parameter is either an object or a string (someone correct me if I'm wrong but that's my understanding).

Note that you can also use logger.log({level: <level>, message: <message>}). To learn more about these two functions I would recomend reading this part of the docs: Winston Docs on Log Levels. Be sure to read through Using Logging Levels

logger.error(`Caught error: ${e}`);

I can not definitively say why this does not output the stack, but I do know this is not a problem with winston. If you try console.log(`Caught error: ${e}`) it also does not include the stack. I haven't worked with template literals much so either template literals doesn't work well with objects, or javascript's console.log recognizes the object as an error object and thus only outputs the the message property. That's my best guess.

logger.error(`Caught error: ${JSON.stringify(e)}`)

This one gets to the heart of what this bug thread is about. First you must understand some technical details about javascript. Note that if you try console.log(`Caught error: ${JSON.stringify(e)}`) you also get that same output Caught error: {}. As @indexzero explained:

message and stack properties on Error are non-enumerable which causes JSON.stringify to output something that one does not expect.

Basically, because message and stack properties are non-enumerable, JSON.stringify skips over those properties which is how you end up with an empty object {}. To understand enumerability better I recommend reading this Enumerability and ownership of properties.

Luckily, because of the way winston 3.0 was designed (props to the winston team) we have a workaround for this that @indexzero gave. I'll help explain it. First you create this function:

const enumerateErrorFormat = format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack
    }, info.message);
  }

  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack
    }, info);
  }

  return info;
});

From the docs Streams, objectMode, and info objects the info object has two properties, info.level, and info.message. That info.message property IS the error object if that is all you passed in. So we create a new object where message.stack and message.message (Think of it as Error.stack and Error.message) are now enumerable, and we include what ever other properties that may also be attached to that error object.

Next you'll create this logger which uses the enumerateErrorFormat() function above:

const logger = createLogger({
  format: format.combine(
    enumerateErrorFormat(),
    format.json()
  ),
  transports: [
    new transports.Console()
  ]
});

This will take whatever message you pass in and check if it is an error object. If it is then it will fix the enumeration problem. Then it passes message to format.json which will stringify any object (error or not). If it's not an object then it's a string and format.json effectivley does nothing, and you're home free!

Still, it would be nice if we didn't have to create that enumerateErrorFormat since error objects are commonly logged. As I understand it the winston team is working on a fix that will be released in a later version.

Some final notes. This only works if you use logger.log({level: <level>, message: <message>}) where message is the error object. Example:

try {
  throw(new Error('This should work'));
} catch (err) {
  logger.log({level: 'error', message: err});
}

There is another bug in winston where this code does not work, as I explained in my other post above:

try {
  throw(new Error('This will not work'));
} catch (err) {
  logger.error(err);
}

For some reason the info.message property is undefined when we use logger.error(err). Hopefully @indexzero can figure this one out.

nvtuan305 commented 6 years ago

@SamuelMaddox17 @indexzero Thank you! I tried using logger.log({level: 'error', message: err}); and it works

inderpreet99 commented 6 years ago

Can this please get fixed for logger.error, etc?

It is cumbersome and verbose to use logger.log, especially since with logger.error you can easily add multiple arguments.

DABH commented 6 years ago

Hey all, I'm looking into this. @indexzero : still think the best idea is to essentially add the enumerateErrorFormat functionality to the json formatter by default? Do we need to worry separately about if meta is an Error not just an object (I'm guessing people will complain if we don't also handle that case?)? Also, I'm using master, but seems like logger.error works for me with the solution by @indexzero / @SamuelMaddox17 above:

const winston = require('winston');
const format = winston.format;

const enumerateErrorFormat = format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack
    }, info.message);
  }

  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack
    }, info);
  }

  return info;
});

const logger = winston.createLogger({
  level: 'debug',
  format: format.combine(
    enumerateErrorFormat(),
    format.json()
  ),
  transports: [
    new winston.transports.Console(),
  ],
});

logger.error(new Error('whatever'));
SamuelMaddox commented 6 years ago

Upon further investigation it seems that the logger.error problem I explained above is only a problem when using the default logger. @DABH, I tried out your code and it does work for me, but when I switch it to the default logger it does not work:

const winston = require('winston');
const format = winston.format;

const enumerateErrorFormat = format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack
    }, info.message);
  }

  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack
    }, info);
  }

  return info;
});

winston.configure({
  transports: [
    new winston.transports.Console({
      level: 'debug',
      format: format.combine(
        enumerateErrorFormat(),
        format.json()
      ),
    })
  ]
});

winston.error(new Error('whatever'));

Second, I agree that enumerateErrorFormat should be added to the json format; and you're probably right about meta as well.

Finally, I'd like to note the code example given by @DABH causes the stack to not "print pretty" if you will; at least on my machine running macOS High Sierra. This is what it looks like for me:

{"message":"whatever","stack":"Error: whatever\n    at Object.<anonymous> (/Users/samuelmaddox/Desktop/winston-test/index.js:33:14)\n    at Module._compile (internal/modules/cjs/loader.js:689:30)\n    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)\n    at Module.load (internal/modules/cjs/loader.js:599:32)\n    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)\n    at Function.Module._load (internal/modules/cjs/loader.js:530:3)\n    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)\n    at startup (internal/bootstrap/node.js:266:19)\n    at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)","level":"error"}

As you can see, when outputting the error with a to JSON function the newline characters \n do not create actual new lines. This is expected behavior when taking an object and converting it to JSON, but is probably not the behavior we'd actually want from a logger, at least when logging to console.

Thank you for looking more into this @DABH

petrogad commented 6 years ago

FYI this is where I've gotten to after playing with this a bit:

import winston from 'winston';
const format = winston.format;

const printNice = format.printf(info => {
    const {level, message} = info;
    return `Logging Level: ${level} - Logging Message: ${message}`;
});

const enumerateErrorFormat = format(info => {
    if (info.message instanceof Error) {
        info.message = Object.assign({
            message: `${info.message.message}\n============\n${info.message.stack}`
        }, info.message);
    }

    if (info instanceof Error) {
        return Object.assign({
            message: `${info.message}\n============\n${info.stack}`
        }, info);
    }

    return info;
});

const logger = winston.createLogger({
    format: format.combine(
        enumerateErrorFormat(),
        format.json()
    ),
    transports: [
        new winston.transports.Console({
            format: format.combine(
                format.colorize(),
                printNice,
            ),
        })
    ]
});

export default logger;
Asing1001 commented 6 years ago

The issue is caused by this bug : https://github.com/winstonjs/winston-transport/issues/31

barisusakli commented 6 years ago

We definitely used this form in winston2.x with no problems. winston.err('some message', err); along with winston.error(err) the above enumerateErrorFormat fixes winston.error(err) but not the use case with err as second parameter.

ddemydenko commented 6 years ago

@SamuelMaddox17

logger.log({level: ____, message: err});

it works thx

SamuelMaddox commented 6 years ago

Okay, I discovered something. My comment from September 3rd is wrong. This isn't a problem with the default logger. This is a problem with where you define level and/or format. @DABH here is your old code:

const winston = require('winston');
const format = winston.format;

const enumerateErrorFormat = format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack
    }, info.message);
  }

  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack
    }, info);
  }

  return info;
});

const logger = winston.createLogger({
  level: 'debug',
  format: format.combine(
    enumerateErrorFormat(),
    format.json()
  ),
  transports: [
    new winston.transports.Console(),
  ],
});

logger.error(new Error('whatever'));

If you remove this:

const logger = winston.createLogger({
  level: 'debug',
  format: format.combine(
    enumerateErrorFormat(),
    format.json()
  ),
  transports: [
    new winston.transports.Console(),
  ],
});

And replace it with this:

const logger = winston.createLogger({
  transports: [
    new winston.transports.Console({
      level: 'debug',
      format: format.combine(
        enumerateErrorFormat(),
        format.json()
      ),
    }),
  ],
});

Then the info.message === undefined problem shows up. I believe it should be okay to specify level and format for each transport; and I'm almost certain this was allowed in Winston 2.0.

Here is your code sample with my code change so you can easily run and test:

const winston = require('winston');
const format = winston.format;

const enumerateErrorFormat = format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack
    }, info.message);
  }

  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack
    }, info);
  }

  return info;
});

const logger = winston.createLogger({
  transports: [
    new winston.transports.Console({
      level: 'debug',
      format: format.combine(
        enumerateErrorFormat(),
        format.json()
      ),
    }),
  ],
});

logger.error(new Error('whatever'));

Hopefully this helps get to the root of the problem.

crowleym commented 5 years ago

I created https://github.com/winstonjs/winston/pull/1527

This covers all options. However some tests fail so I have closed it for now. The failures are expected given the fix, but I don't believe I am in a position to make the call to amend/delete the tests.

Failing build is here https://travis-ci.org/winstonjs/winston/jobs/453012141 and its obvious why the tests now fail when you read the test code: https://github.com/winstonjs/winston/blob/c42ab7fdc51b88db180a7dd90c52ce04ddd4e054/test/logger.test.js#L668

Thoughts?

giang12 commented 5 years ago

I think the problem is in this line const info = ( msg && !(msg instanceof Error) && msg.message && msg) || { message: msg }; adding a check of instanceof Error seems to resolve the issue as @crowleym point out

danielvaijk commented 5 years ago

For anyone that is still dealing with this, this is the workaround formatter I've managed to come up with (snippet of my logger.js module):

const { format, transports, createLogger }     = require("winston");
const { combine, timestamp, colorize, printf } = format;

function devFormat() {
    const formatMessage = info => `${info.level}: ${info.timestamp} ${info.message}`;
    const formatError   = info => `${info.level}: ${info.timestamp}\n\n${info.stack}\n`;
    const format        = info => info instanceof Error ? formatError(info) : formatMessage(info);

    return combine(timestamp(), printf(format));
}

For some reason logger.error(new Error("hello")) only works if you define the formatter globally in winston.createLogger 🤔 and then you get the Error object in info in the formatter.

If you define a formatter per transport then you have to use logger.log({level: "error", message: new Error("FAILED")}) and handle the Error object via info.message instead of info to access the Error object.

I'd guess there's a bug when setting the format field in the transport options?

Those are just my 2 cents and what worked for me, I'm new to Winston and not seasoned in JavaScript so don't quote me on anything.

crowleym commented 5 years ago

My approach was an attempt to fix the root cause. But its not getting much traction from the repo owners...

danielvaijk commented 5 years ago

Yeah, I understand that. I literally spent a lot of time figuring this out because I'm new to Winston and thought it was perhaps me utilizing it incorrectly or not having grasped the concepts behind it properly enough yet.

But luckily I stumbled upon a few threads (this one included) that showed me otherwise. Hopefully they'll get this fixed so I don't have to keep using the workaround.

rgoupil commented 5 years ago

This might be caused by wintson-transport, see https://github.com/winstonjs/winston-transport/issues/31 for the issue and https://github.com/winstonjs/winston-transport/pull/34 for a PR.

indexzero commented 5 years ago

Direct logging of error objects is always a mess because of their non-enumerable properties. Personally I consider this a bad practice, but enough folks in the community are adamant about it as a requirement that we must support it.

Considering adopting https://github.com/winstonjs/logform/pull/59 as a format to support behaviors like this. On the plus side it encapsulates all of the edge cases that are so common when treating errors as log messages. On the down side it would be yet another format folks would need to opt-in to (similar to .splat())

rgoupil commented 5 years ago

@indexzero I tend to agree but direct error logging can be useful when coupled with custom logging format/printf if there is a need to display multiple Error types differently, and I don't remember Winston 2.x trying to fight this practice as it was allowed out of the box.

joepuzzo commented 5 years ago

So the proposed solution for enumerateErrorFormat works but does not support the format logger.error('some message', err). Because neither the info.message or info is instanceof Error. Also I want to point out another issue with this solution. I am currently logging an error that is returned fromsuperagent

    try {
      const response = await request
        .get('https://some/endpoint')
        .set('Authorization', bearerToken);
      logger.info('successfully received response');
      return response.body;
    } catch (e) {
      logger.error('An error was caught while getting programs');
      logger.error(e); // <<< THE ERROR LOG  
    }

If we simply use Object.assign then cool the stack and message will be set! BUT, any other info that was part of the error will also get logged. This can be very dangerous is cases where errors have sensitive data such as Authorization Headers ( which in this case gets included as part of the error object ).

But then you might say. This is not a fault of winston, its not the fault of winston that superagent adds this data to the error. I AGREE! HOWEVER, because everything is stored flat on the info object it becomes very difficult to keep the old info and not override the rest.

It would almost be nice if, when using logger.error. It assumed the second param was and error and put it on the info object as info.error and then if logging the other way, the interface would be { level: "error", error: YOUR ERROR OBJECT}

Im really just spitballing here but the new interface has definitely been a little frustrating (everything on info).

joepuzzo commented 5 years ago

Just to elaborate the point I was making, suppose you logger.error( e ) where e is of type error.

Then in your code you have winston configured as follows:

winston.configure({
    format: combine(
      timestamp(),
      enumerateErrorFormat(),
      ...
    ),
  });

the timestamp is getting shoved onto the error object 😱. Does that really make sense? Think about that.. the error object you sent in is getting a new prop dynamically.. timestamp.

I think the overall best solution to this problem would be to support the following syntax

logger.error('An error occurred when doing something important', { error: e } );

and then internally you can create an error formatter that looks for error field!

indexzero commented 5 years ago

Update on this folks:

Hoping to get this sewn up and shipped in the next couple of days. It is the next to last issue in our winston@3.2.0 tracker

indexzero commented 5 years ago

Howdy folks – please check out https://github.com/winstonjs/winston/pull/1562. This was the last item on our 3.2.0 release check list so once that PR is sewn up we will be able to release.

ezze commented 5 years ago

Until a fix is published I use the following workaround:

logger.error = item => {
  logger.log({ level: 'error', message: item instanceof Error ? item.stack : item });
};
the-vampiire commented 5 years ago

I am on the latest winston (3.2.1) and am still getting undefined when passing an error to logger.error

@ezze had a great solution but the stack trace is forced onto a new line. here is a slightly modified version that puts it in one line (so its caught by simple grep of log files)

logger.error = item => {
  const message = item instanceof Error
    ? item.stack.replace('\n', '').replace('    ', ' - trace: ')
    : item;
  logger.log({ level: 'error', message });
};

with output <Error message> - trace: <stack trace>

if there is an easier way to do this with the latest winston please let me know @indexzero. i am new to the library and was following the docs

the-vampiire commented 5 years ago

I just saw the link you posted to the PR. does this mean that to pass an error to logger.error requires a message string, then the error?

try {
  someThing();
} catch(error) {
  logger.error(error); // what I would like to do
  logger.error('special message', error); // what I believe is required?
}
SamuelMaddox commented 5 years ago

@the-vampiire There ended up being 2 problems this thread talked about. The first being what the original poster brought up, and the second being the problem I brought up which is the same as your problem. I think they only fixed the original posters problem. I've been meaning to check further to make sure and then open a new issue if that's the case. I haven't had the time to dive deeper unfortunately. In the meantime, if you use logger.log({level: 'error', message: err}); where err is an Error Object then it will work.

HRK44 commented 5 years ago

Still having this issue, lost bunch of time to figure this out, solution from @the-vampiire works well.

Why is this ticket closed?

terrisgit commented 5 years ago

Overriding logger.error is so far the best solution because it doesn’t add a timestamp property to an Error object that is passed as a single argument to error(). Most people likely expect that Error objects are immutable. If you don't also override logger.info and every other level-related method, it's a surprise when things don't work as expected. Again, unless you want your object to be modified, no matter its type, don't send it directly to a Winston logger method.

artemjackson commented 5 years ago

The feature is supported since winston@3.2.0

Example usage:

const winston = require('winston');
const { transports, format } = winston;

const print = format.printf((info) => {
  const log = `${info.level}: ${info.message}`;

  return info.stack
    ? `${log}\n${info.stack}`
    : log;
});

const logger = winston.createLogger({
  level: 'debug',
  format: format.combine(
    format.errors({ stack: true }),
    print,
  ),
  transports: [new transports.Console()],
});

const error = new Error('Ooops');

logger.error(error);
logger.error('An error occurred:', error);

cc @HRK44 @the-vampiire

OBrown92 commented 5 years ago

I also still running into the issue. When i call the error like logger.error(error); I get only undefined. Only if I call it like logger.error('Something went wrong', error) I get the complete error and can parse it.

victor-develop commented 5 years ago

I also still running into the issue. When i call the error like logger.error(error); I get only undefined. Only if I call it like logger.error('Something went wrong', error) I get the complete error and can parse it. Did you add this?

format.errors({ stack: true })
OBrown92 commented 5 years ago

Yes, still the same issue. I try to reproduce it in a gist.

marcin1990ns commented 5 years ago

@OBrown92 I have faced the same issue today. I can confirm, that it works, if format.errors({ stack: true }), is applied to logger, not to transport. In this case it's possible to use both logger.error(error); and logger.error('Something went wrong', error). However, there is a problem, when I try to apply format.errors({ stack: true }) to chosen transport. In this case I get undefined for logger.error(error);, but logger.error('Something went wrong', error) works properly.

I'm not sure if it's expected behavior or it's a bug, but I spent a lot of time to find the cause, so please fix it or mention about that somewhere in your documentation. It would be really helpul.

Anyway, I am very grateful for your work on this great project.

Atishay96 commented 5 years ago

I was facing the same issue so i wrote this package, utils-deep-clone. Check it out.

holmberd commented 5 years ago

format.errors is not a function ... well that's a surprise.

aybhalala commented 5 years ago

@holmberd Did you import the formats from winston package? Example usage: const { format } = require('winston') Or const winston = require('winston'); const { format } = winston;

holmberd commented 5 years ago

@aybhalala yepp, though it doesn't matter the error stack is passed to printf without it.

joepuzzo commented 5 years ago

Update: because there are still issues with this I have been doing the following for a while and its been working great

// Grab the default winston logger
const winston = require('winston');

const { format } = winston;
const { combine, timestamp } = format;

// Custom format function that will look for an error object and log out the stack and if 
// its not production, the error itself
const myFormat = format.printf((info) => {
  const { timestamp: tmsmp, level, message, error, ...rest } = info;
  let log = `${tmsmp} - ${level}:\t${message}`;
  // Only if there is an error
  if ( error ) {
    if ( error.stack) log = `${log}\n${error.stack}`;
    if (process.env.NODE_ENV !== 'production') log = `${log}\n${JSON.stringify(error, null, 2)}`;
  }
  // Check if rest is object
  if ( !( Object.keys(rest).length === 0 && rest.constructor === Object ) ) {
    log = `${log}\n${JSON.stringify(rest, null, 2)}`;
  }
  return log;
});

 winston.configure({
    transports: [
      new winston.transports.Console({
        level: 'debug',
        timestamp: true,
        handleExceptions: true
    }),
  ];
    format: combine(
      trace(),
      timestamp(),
      myFormat
    ),
  });

// Finally you log like this
logger.error('An error occurred!!!', { error } );
terrisgit commented 5 years ago

^^ That is the expected usage. Since mere mortals will never figure this out, it should be documented.

the-vampiire commented 5 years ago

Since mere mortals will never figure this out, it should be documented

😂 +1 to that mate

codyzu commented 5 years ago

I use winston with a 3rd party transport (@google-cloud/logging-winston) so I have a little less control over the syntax. Plus I find this just more intuitive:

const error = new Error('something bad happened');
logger.error('was doing this and', error);

When logging to the console I concatenate the stack onto the message. But the result is something like this:

ERROR was doing this andsomething bad happened Error: something bad happened <rest of the stack.>

Since winston concatenates meta.message onto the original message, there is the weird andsomething and the duplicate message that is also printed in the stack. This is described in #1660 .

It looks like #1664 is trying to fix this. In the meantime, I wrote a formatter that "undos" that concatenation: https://github.com/winstonjs/winston/issues/1660#issuecomment-512226578

ahmedkamalio commented 5 years ago

@dandv

logger.error('Caught error:', e);

This doesn't work because, unlike console.log(), the winston's logger.<level>(message) takes only one parameter called message. That message parameter is either an object or a string (someone correct me if I'm wrong but that's my understanding).

Note that you can also use logger.log({level: <level>, message: <message>}). To learn more about these two functions I would recomend reading this part of the docs: Winston Docs on Log Levels. Be sure to read through Using Logging Levels

logger.error(`Caught error: ${e}`);

I can not definitively say why this does not output the stack, but I do know this is not a problem with winston. If you try console.log(`Caught error: ${e}`) it also does not include the stack. I haven't worked with template literals much so either template literals doesn't work well with objects, or javascript's console.log recognizes the object as an error object and thus only outputs the the message property. That's my best guess.

logger.error(`Caught error: ${JSON.stringify(e)}`)

This one gets to the heart of what this bug thread is about. First you must understand some technical details about javascript. Note that if you try console.log(`Caught error: ${JSON.stringify(e)}`) you also get that same output Caught error: {}. As @indexzero explained:

message and stack properties on Error are non-enumerable which causes JSON.stringify to output something that one does not expect.

Basically, because message and stack properties are non-enumerable, JSON.stringify skips over those properties which is how you end up with an empty object {}. To understand enumerability better I recommend reading this Enumerability and ownership of properties.

Luckily, because of the way winston 3.0 was designed (props to the winston team) we have a workaround for this that @indexzero gave. I'll help explain it. First you create this function:

const enumerateErrorFormat = format(info => {
  if (info.message instanceof Error) {
    info.message = Object.assign({
      message: info.message.message,
      stack: info.message.stack
    }, info.message);
  }

  if (info instanceof Error) {
    return Object.assign({
      message: info.message,
      stack: info.stack
    }, info);
  }

  return info;
});

From the docs Streams, objectMode, and info objects the info object has two properties, info.level, and info.message. That info.message property IS the error object if that is all you passed in. So we create a new object where message.stack and message.message (Think of it as Error.stack and Error.message) are now enumerable, and we include what ever other properties that may also be attached to that error object.

Next you'll create this logger which uses the enumerateErrorFormat() function above:

const logger = createLogger({
  format: format.combine(
    enumerateErrorFormat(),
    format.json()
  ),
  transports: [
    new transports.Console()
  ]
});

This will take whatever message you pass in and check if it is an error object. If it is then it will fix the enumeration problem. Then it passes message to format.json which will stringify any object (error or not). If it's not an object then it's a string and format.json effectivley does nothing, and you're home free!

Still, it would be nice if we didn't have to create that enumerateErrorFormat since error objects are commonly logged. As I understand it the winston team is working on a fix that will be released in a later version.

Some final notes. This only works if you use logger.log({level: <level>, message: <message>}) where message is the error object. Example:

try {
  throw(new Error('This should work'));
} catch (err) {
  logger.log({level: 'error', message: err});
}

There is another bug in winston where this code does not work, as I explained in my other post above:

try {
  throw(new Error('This will not work'));
} catch (err) {
  logger.error(err);
}

For some reason the info.message property is undefined when we use logger.error(err). Hopefully @indexzero can figure this one out.

Very good explanation, I just want to add that with logger.error(Caught error: ${e}); you lose the stack because of the way that string literal works in javascript, `${e}` is exactly the same as e.toString(), so printing only the error message is the expected behavior.