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

Fix stacktrace format for rollbar #5

Merged
merged 11 commits into from
Jun 19, 2023
1 change: 1 addition & 0 deletions .npmignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/test
10 changes: 8 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ A [rollbar](https://rollbar.com) transport for [winston](https://github.com/wins
## Installation

``` sh
$ npm install winston
$ npm install winston-transport-rollbar-3
$ yarn add winston
$ yarn add winston-transport-rollbar-3
```

## usage
Expand Down Expand Up @@ -36,3 +36,9 @@ The Rollbar transport uses the universal [rollbar.js](https://github.com/rollbar
* **rollbarConfig**: Rollbar configuration ([more info](https://rollbar.com/docs/notifier/node_rollbar/#configuration-reference)) (mandatory, must contain rollbarAccessToken)
* **level**: Level of messages this transport should log. (default: **warn**).

## Contribute

### Test
```bash
yarn test
```
32 changes: 26 additions & 6 deletions lib/rollbar_transport.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const rollbar = require('rollbar');
class RollbarTransport extends TransportStream {
constructor(opts) {
super(opts);
if (!opts.rollbarConfig.accessToken) {
if (!opts.rollbarConfig?.accessToken) {
throw "winston-transport-rollbar requires a 'rollbarConfig.accessToken' property";
}

Expand All @@ -28,10 +28,10 @@ class RollbarTransport extends TransportStream {
}

log(info, callback) {
const {level, message} = info;
const { level, message } = info;

process.nextTick(() => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const message = util.format(info.message, ...(info.splat || []));
const meta = info;
const error = this.#getErrorObject(info);
const rollbarLevel = level === 'warn' ? 'warning' : level;

const cb = err => {
Expand All @@ -42,11 +42,31 @@ class RollbarTransport extends TransportStream {
this.emit('logged');
return callback(null, true);
};

const logMethod = this.rollbar[rollbarLevel] || this.rollbar.log;
return logMethod.apply(this.rollbar, [message, meta, cb]);
return logMethod.apply(this.rollbar, [error, info, cb]);
});
}

#getErrorObject(info) {
if (info.message instanceof Error) {
return info.message;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great function 👏 ! I'd change to put the return in one line, like that:

if (info.message instanceof Error) return info.message;


if (info.stack instanceof Error) {
return info.stack;
}

if (info.stack) {
return new Error(info.stack);
}

if (info.job?.stacktrace) {
return new Error(info.job.stacktrace);
}

return new Error(info.message);
}
}

module.exports = RollbarTransport;
9 changes: 7 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "winston-transport-rollbar-3",
"description": "An updated rollbar transport for winston",
"version": "3.2.3",
"version": "3.2.4",
"author": "Andreas Zoellner",
"license": "MIT",
"repository": {
Expand All @@ -20,7 +20,9 @@
"winston-transport": "^4.4.0"
},
"main": "./lib/rollbar_transport",
"scripts": {},
"scripts": {
"test": "jest"
},
"engines": {
"node": ">= 8.0.0"
},
Expand All @@ -30,5 +32,8 @@
"homepage": "https://github.com/zoellner/winston-rollbar#readme",
"directories": {
"lib": "lib"
},
"devDependencies": {
"jest": "^29.5.0"
}
}
99 changes: 99 additions & 0 deletions test/rollbar_transport.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
const RollbarTransport = require('../lib/rollbar_transport');
const rollbar = require('rollbar');

jest.mock('rollbar');

describe('RollbarTransport', () => {
let transport;
let rollbarMock;

function wait() {
// wait for process.nextTick
return new Promise(resolve => setTimeout(resolve, 0));
}

beforeEach(() => {
rollbarMock = {
log: jest.fn(),
warning: jest.fn(),
error: jest.fn(),
};

rollbar.mockReturnValue(rollbarMock);

transport = new RollbarTransport({
rollbarConfig: { accessToken: 'test' },
});
});

describe('#log', () => {
it('throws an error if no accessToken is provided', () => {
expect(() => new RollbarTransport({})).toThrow(
"winston-transport-rollbar requires a 'rollbarConfig.accessToken' property"
);
});

it('logs with log level error', async () => {
const logData = { level: 'error', message: 'test' };
transport.log(logData);

await wait()

expect(rollbarMock.error).toHaveBeenCalledWith(new Error(logData.message), logData, expect.any(Function));
});

it('logs with log level warn', async () => {
const logData = { level: 'warn', message: 'test' };
transport.log(logData);

await wait()

expect(rollbarMock.warning).toHaveBeenCalledWith(new Error(logData.message), logData, expect.any(Function));
})

it('logs error object if message is of type Error', async () => {
const logData = { level: 'error', message: new Error('test') };
transport.log(logData);

await wait()

expect(rollbarMock.error).toHaveBeenCalledWith(logData.message, logData, expect.any(Function));
})

it('logs error object if stack is of type Error', async () => {
const logData = { level: 'error', stack: new Error('test') };
transport.log(logData);

await wait()

expect(rollbarMock.error).toHaveBeenCalledWith(logData.stack, logData, expect.any(Function));
})

it('logs error object if stack is a string', async () => {
const logData = { level: 'error', stack: 'test' };
transport.log(logData);

await wait()

expect(rollbarMock.error).toHaveBeenCalledWith(new Error(logData.stack), logData, expect.any(Function));
})

it('logs error object if job.stacktrace is a string', async () => {
const logData = { level: 'error', job: { stacktrace: 'test' } };
transport.log(logData);

await wait()

expect(rollbarMock.error).toHaveBeenCalledWith(new Error(logData.job.stacktrace), logData, expect.any(Function));
})

it('logs error object if only message is a string', async () => {
const logData = { level: 'error', message: 'test' };
transport.log(logData);

await wait()

expect(rollbarMock.error).toHaveBeenCalledWith(new Error(logData.message), logData, expect.any(Function));
})
});
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strong tests 💪

Loading