-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Changes from 9 commits
9ca4a2a
18dc87e
a76b100
89a3e5a
e13cfe0
77a46b6
9c92338
25ad2d6
4673f11
4ee23bd
602fca3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
/test |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
} | ||
|
||
|
@@ -28,24 +28,27 @@ class RollbarTransport extends TransportStream { | |
} | ||
|
||
log(info, callback) { | ||
const {level, message} = info; | ||
process.nextTick(() => { | ||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two things I'd still add however.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added fallback to |
||
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); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
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)); | ||
}) | ||
}); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strong tests 💪 |
There was a problem hiding this comment.
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.