Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

feat: Allow to configure stacktrace for captureMessage calls #388

Merged
merged 1 commit into from
Oct 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
feat: Allow to configure stacktrace for captureMessage calls
  • Loading branch information
kamilogorek committed Oct 6, 2017
commit 19a8caa079590a78e912e81515af8962f7573ac5
4 changes: 4 additions & 0 deletions docs/config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,10 @@ Those configuration options are documented below:

Controls how many requests can be maximally queued before bailing out and emitting an error. Defaults to `100`.

.. describe:: stacktrace

Attack stack trace to `captureMessage` calls by generatic "synthetic" error object and extracting all frames.

Environment Variables
---------------------

Expand Down
28 changes: 26 additions & 2 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ extend(Raven.prototype, {
this.sampleRate = typeof options.sampleRate === 'undefined' ? 1 : options.sampleRate;
this.maxReqQueueCount = options.maxReqQueueCount || 100;
this.parseUser = options.parseUser;
this.stacktrace = options.stacktrace || false;

if (!this.dsn) {
utils.consoleAlert('no DSN provided, error reporting disabled');
Expand Down Expand Up @@ -229,7 +230,7 @@ extend(Raven.prototype, {
but also want to provide convenience for passing a req object and having us parse it out
so we only parse a `req` property if the `request` property is absent/empty (and hence we won't clobber)
parseUser returns a partial kwargs object with a `request` property and possibly a `user` property
*/
*/
kwargs.request = this._createRequestObject(
this._globalContext.request,
domainContext.request,
Expand Down Expand Up @@ -318,8 +319,31 @@ extend(Raven.prototype, {
} else {
kwargs = kwargs || {};
}

var eventId = this.generateEventId();
this.process(eventId, parsers.parseText(message, kwargs), cb);

if (this.stacktrace) {
var ex;
// Generate a "synthetic" stack trace
try {
throw new Error(message);
Copy link
Member

Choose a reason for hiding this comment

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

do you have to throw the error to get the stack?

Copy link
Contributor Author

@kamilogorek kamilogorek Oct 5, 2017

Choose a reason for hiding this comment

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

No, it's just more explicit, as we wrap it in try clause. Also this is how we have it written in raven-js and I'm trying to write as unified code as possible.

Copy link
Member

@mitsuhiko mitsuhiko Oct 5, 2017

Choose a reason for hiding this comment

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

I'm mostly asking because throwing an exception vs just creating an error should have noticable performance differences. And since message logging is potentially more common than crashes it might be a concern here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm surprised with this, but you're right : o

throw new Error(i)
608.497162ms
575.068622ms
574.888692ms
540.633688ms
540.919783ms
550.634267ms
577.011109ms
541.462129ms
568.272018ms
573.879691ms

avg: 565.1267161000001ms

new Error(i)
410.160736ms
412.978303ms
423.740643ms
434.467241ms
420.66899ms
426.131056ms
430.911071ms
426.683749ms
429.369119ms
432.251688ms

avg: 424.73625960000004ms
var start = process.hrtime()
var avg = 0 
var n = 10
var sample = 100000

console.log('\nthrow new Error(i)')

for (var j = 0; j < n; j++) {
  for (var i = 0; i < sample; i++) {
    try {
      throw new Error(i)
    } catch (e) {}
  }

  end = process.hrtime(start)[1]
  start = process.hrtime()

  avg += end 

  console.log('' + (end/1000000) + 'ms')
}
console.log('\navg: ' + (avg/n/1000000) + 'ms')

console.log('\nnew Error(i)')

avg = 0 

for (var j = 0; j < n; j++) {
  for (var i = 0; i < sample; i++) {
    try {
      new Error(i)
    } catch (e) {}
  }

  end = process.hrtime(start)[1]
  start = process.hrtime()

  avg += end 

  console.log('' + (end/1000000) + 'ms')
}
console.log('\navg: ' + (avg/n/1000000) + 'ms')
```js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests are failing without throw, in the end it is required ¯_(ツ)_/¯

} catch (ex1) {
ex = ex1;
}

utils.parseStack(
ex,
function(frames) {
// We trim last frame, as it's our `throw new Error(message)` call itself, which is redundant
kwargs.stacktrace = {
frames: frames.slice(0, -1)
};
this.process(eventId, parsers.parseText(message, kwargs), cb);
}.bind(this)
);
} else {
this.process(eventId, parsers.parseText(message, kwargs), cb);
}

return eventId;
},
Expand Down
15 changes: 15 additions & 0 deletions test/raven.client.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,21 @@ describe('raven.Client', function() {

client.captureMessage('Hey!');
});

it('should allow for attaching stacktrace', function(done) {
var dsn = 'https://public:private@app.getsentry.com:8443/269';
var client = new raven.Client(dsn, {
stacktrace: true
});
client.send = function mockSend(kwargs) {
kwargs.message.should.equal('wtf?');
kwargs.should.have.property('stacktrace');
var stack = kwargs.stacktrace;
stack.frames[stack.frames.length - 1].context_line.should.match(/captureMessage/);
done();
};
client.captureMessage('wtf?');
});
});

describe('#captureException()', function() {
Expand Down