getsentry / raven-node

A standalone (Node.js) client for Sentry
https://sentry.io/
BSD 2-Clause "Simplified" License
454 stars 135 forks source link

Raven.captureException does not process the request properly in koa app. #375

Closed chrisjensen closed 7 years ago

chrisjensen commented 7 years ago

What is the current behavior?

Raven.captureException does not process the request properly in koa app.

Minimum example at this gist.

In the gist, you can see four ways I've tried to capture the request (lines 29 to 32)

The relavent lines are:

Raven.captureException(
        err,
        {
//              req: this.request,
//              request: this.request,
//              request: this.req,
            req: this.req,
        },

Test this server by sending a post request to http://localhost:3000/error with some body included

Here's the information that's recorded about the request in my sentry dashboard from those different configurations

Request Attachment Method Body Headers? HTTP Method URL Other
req: this.req "\<unavailable>" Yes POST http://localhost:3000/error Sentry says: Discarded invalid parameter 'req'
request: this.req (none) Yes POST /error
request: this.request (none) (none) (none) (none) Discarded invalid value for parameter 'request'
req: this.request Full Body (none) GET (should be POST) http://\<no host>/error Discarded invalid parameter 'req'

What is the expected behavior?

At least one of these ways of attaching the request should cause the full request to be saved, ie:

Request Attachment Method Body Headers HTTP Method URL Other
??? Full Body Yes POST http://localhost:3000/error
kamilogorek commented 7 years ago

Koa itself doesn't provide parsers, you have to attach them yourself, that's why body is always empty.

Use https://github.com/koajs/bodyparser and this.request to report errors.

Also your implementation is using generators, which will be dropped in v3 of Koa and it's written in a way that it'll never reach catch clause (you can verify it using --inspect flag and Chrome Devtools).

screen shot 2017-09-21 at 13 05 49

Working example:

'use strict';

const PORT = 3000;

const Koa = require('koa');
const Router = require('koa-router');
const bodyParser = require('koa-bodyparser');

const Raven = require('raven');
Raven.config('__DSN__').install();

const app = new Koa();
const router = new Router();

function* errors(next) {
  let err;

  try {
    yield next;
  } catch (e) {
    err = e;
    this.body = { message: err.message };
  } finally {
    Raven.captureException(err, { req: this.request }, (sendErr, eventId) => {
      if (sendErr) console.error('Failed to send captured exception to Sentry', err);
    }); 
  }
}

app.use(bodyParser());
app.use(errors)

router.post('/', function (ctx, next) {
  throw new Error('oh hi');
});

app
  .use(router.routes())
  .use(router.allowedMethods());

app.listen(PORT);
console.log(`Server listening on port: ${PORT}`);
chrisjensen commented 7 years ago

Thanks for the help. It turns out we are using bodyparser in our codebase, but it was being applied to the stack after our error handler. (I didn't implement the original codebase, so not entirely familiar with all it's ins and outs).

However, moving it up does not change the results. I still get the same 4 outcomes documented above, and am still only able to see a body passed to sentry by using req: this.request. I've verified that moving bodyparser up causes typeof this.request.body === 'object' before it is passed to captureException.

We've got an old codebase, so we're using koa 0.5.5 if that's relevant. (On node 7.6.0)

(The results I've posted are from using that exact code that I posted. It does reach the catch because err was populated and passed to sentry in those examples)

kamilogorek commented 7 years ago

Can you paste your package.json with relevant libraries versions please? (koa, koa-router, koa-bodyparser)?

chrisjensen commented 7 years ago

Thanks for the help, here's the relevant parts of package.json (not the full list, but all the packages that are likely to influence the outcome of tests)

"dependencies": {
    "bluebird": "^3.4.6",
    "co": "^4.6.0",
    "co-bcryptjs": "^0.2.0",
    "co-redis": "^2.0.0",
    "co-s3": "^1.0.0",
    "koa": "^1.1.1",
    "koa-bodyparser": "^2.0.1",
    "koa-cors": "0.0.16",
    "koa-formidable": "^1.0.0",
    "koa-generic-session": "^1.10.2",
    "koa-logger": "^1.3.0",
    "koa-lusca": "^2.1.0",
    "koa-mount": "^1.3.0",
    "koa-passport": "^1.2.0",
    "koa-redis": "^1.0.1",
    "koa-render": "^0.2.1",
    "koa-request": "^1.0.0",
    "koa-roles": "^1.0.1",
    "koa-router": "^5.2.3",
    "koa-static-folder": "^0.1.6",
    "lodash": "^3.10.1",
    "raven": "^2.1.0",
    "request": "^2.72.0",
    "request-promise": "^1.0.2",
    "thunkify-wrap": "^1.0.4",
  },
kamilogorek commented 7 years ago

As I already mentioned before, bodyparser has to be attached before any other middleware that uses request body. I was able to reproduce my fix using exactly your gist and your dependencies list just by adding those two lines:


 const app = require('koa')();
 const Router = require('koa-router');
+const bodyparser = require('koa-bodyparser')

 // Configure Sentry
 const Raven = require('raven');
@@ -38,6 +39,7 @@ function* errors(next) {
        }
 }

+app.use(bodyparser());
 app.use(errors);

Does it fix your issue?

chrisjensen commented 7 years ago

Thank you for the time. Sorry if I wasn't clear, after discovering bodyparser was lower in our stack I moved it up to the top but found no change (even though console.log confirmed the body was being parsed)

I also tried replaced my code with the working example that you gave here and got the same results - that is, the request body is present, but the following are incorrect or missing:

HTTP Method: GET (should be POST)
HTTP Headers: Missing
URL: http://<no host>/ (literally appears as this, it has the correct path, but is missing the host component)

This is the same result as I recorded in the table at the top originally. Trying different variants of req(uest): this.req(uest) yields different portions correct, but I can't find any that get's all of these correct.

I've also tried pulling down the latest version of koa, koa-router and koa-bodyparser

"koa": "^2.3.0",
"koa-bodyparser": "^4.2.0",
"koa-router": "^7.2.1",

which also yields the same results.

Not sure if you can access it, but this is the issue that gets recorded using the demo code you supplied: https://sentry.io/agency/raisely/issues/356951070/

kamilogorek commented 7 years ago

@chrisjensen can you test it now using latest ref as a package version?

'raven': 'git@github.com:getsentry/raven-node.git#5d8556aba2682c8add539a77a8b8f35ae3116797'

I want to make sure it fixes your issue before releasing it.

As for the payload, use { req: this.request }

chrisjensen commented 7 years ago

Thanks and sorry for the slow reply. That's fixed the issue, thank you.