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
```
41 changes: 23 additions & 18 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,24 +28,29 @@ class RollbarTransport extends TransportStream {
}

log(info, callback) {
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 rollbarLevel = level === 'warn' ? 'warning' : level;

const cb = err => {
if (err) {
this.emit('error', err);
return callback(err);
}
this.emit('logged');
return callback(null, true);
};

const logMethod = this.rollbar[rollbarLevel] || this.rollbar.log;
return logMethod.apply(this.rollbar, [message, meta, cb]);
setImmediate(() => {
this.emit('logged', info);
});

const level = info[Symbol.for('level')];
const message = info.message;
const meta = Object.assign({}, info);
delete meta.level;
delete meta.message;
delete meta.splat;
delete meta[Symbol.for('level')];

if (level === 'error') {

Choose a reason for hiding this comment

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

Two things I'd still add however.

  1. There is no fallback, when the level is something else. I'd leave that to rollbar.log or ino
  2. warning used to work as warn before, we could still keep that

Copy link
Author

Choose a reason for hiding this comment

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

added fallback to info 👍

this.rollbar.error(message, meta, callback);
} else if (level === 'warn') {
this.rollbar.warning(message, meta, callback);
} else if (level === 'info') {
this.rollbar.info(message, meta, callback);
} else if (level === 'debug') {
this.rollbar.debug(message, meta, callback);
} else {
this.rollbar.info(message, meta, callback);
}
}
}

Expand Down
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.6",
"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"
}
}
81 changes: 81 additions & 0 deletions test/rollbar_transport.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
const RollbarTransport = require('../lib/rollbar_transport');
const rollbar = require('rollbar');

jest.mock('rollbar');

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

function createLogData(level, message, payload = {}) {
const logData = { level, message, ...payload };
logData[Symbol.for('level')] = level;
return logData;
}

beforeEach(() => {
rollbarMock = {
log: jest.fn(),
warning: jest.fn(),
error: jest.fn(),
info: jest.fn(),
debug: 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 = createLogData('error', 'test');
transport.log(logData, function() {});

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

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

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

it('logs with log level info', async () => {
const logData = createLogData('info', 'test');
transport.log(logData, function() {});

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

it('logs with log level debug', async () => {
const logData = createLogData('debug', 'test');
transport.log(logData, function() {});

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

it('logs additional payload as meta', () => {
const logData = createLogData('error', 'test', { foo: 'bar' });
transport.log(logData, function() {});

expect(rollbarMock.error).toHaveBeenCalledWith(logData.message, { foo: 'bar' }, expect.any(Function));
})

it('logs with log level info by default', async () => {
const logData = createLogData('unknown', 'test');
transport.log(logData, function() {});

expect(rollbarMock.info).toHaveBeenCalledWith(logData.message, {}, 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