Fix stacktrace format for rollbar #5
Conversation
lib/rollbar_transport.js
Outdated
| const logMethod = this.rollbar[rollbarLevel] || this.rollbar.log; | ||
| return logMethod.apply(this.rollbar, [message, meta, cb]); | ||
| return logMethod.apply(this.rollbar, [message, info, new Error(info.stack), cb]); |
There was a problem hiding this comment.
Creating a new error here seems wrong to me
lib/rollbar_transport.js
Outdated
| #getErrorObject(info) { | ||
| if (info.message instanceof Error) { | ||
| return info.message; | ||
| } |
There was a problem hiding this comment.
Great function 👏 ! I'd change to put the return in one line, like that:
if (info.message instanceof Error) return info.message;
| expect(rollbarMock.error).toHaveBeenCalledWith(new Error(logData.message), logData, expect.any(Function)); | ||
| }) | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I am through. 😮💨
-
The API of winston is not very clear and developer-friendly IMO. There is a lot of discussion about this topic.
[3.0.0] Error object is not parsed or printed winstonjs/winston#1338 -
When you use
logger.error(err)theerrbecomes themessage, so in general the original implementation is correct. -
However, to properly log errors rollbar expects the errors to be passed to
rollbar.error(err). This was not happening due to two reasons:
A) We use the colorizer before passing data to rollbar. That is why this this.rollbar[rollbarLevel] is not working because rollbarLevel is not error but somethingerrorsomething. The colorizer should only be used for console and papertrail.
B) We format this error before passing it to rollbar
util.format(info.message, ...(info.splat || []));
We should not do that for errors.
So if we fix A and B everything should work.
I don't really like line 34 because it suggests that we can only log errors to rollbar which is not true. And I think even though the API sucks the caller should always make sure that the error object is properly passed as the message.
@dominik1001 I've reimplemented the log function, everything should be logged correctly now. Thanks so much for the help 🙏 |
| @@ -28,24 +28,27 @@ class RollbarTransport extends TransportStream { | |||
| } | |||
|
|
|||
| log(info, callback) { | |||
| const {level, message} = info; | |||
| process.nextTick(() => { | |||
There was a problem hiding this comment.
We're changing a lot of code where I guess it's unclear why it was there in the first place. The git history doesn't tell a good story, and without unit tests it's hard to say why that has been there in the first place, so I think it's ok.
| delete meta.splat; | ||
| delete meta[Symbol.for('level')]; | ||
|
|
||
| if (level === 'error') { |
There was a problem hiding this comment.
Two things I'd still add however.
- There is no fallback, when the level is something else. I'd leave that to
rollbar.logorino warningused to work aswarnbefore, we could still keep that
No description provided.