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

[@sentry/node] AWS Lambda and other Serverless solutions support #1449

Closed
vietbui opened this Issue Jul 28, 2018 · 50 comments

Comments

Projects
None yet
@vietbui
Copy link

vietbui commented Jul 28, 2018

  • @sentry/node version 4.0.0-beta.11
  • I'm using hosted Sentry

What is the current behavior?

I'm using @sentry/node to capture exception on AWS lambda function.

    .catch(err => {
      Sentry.captureException(err)
      context.fail()
    })

However, it kills the process when context.fail() is called and the exception does not end up in Sentry.

I could do a workaround like:

    .catch(err => {
      Sentry.captureException(err)
      setTimeout(() => context.fail(), 1000)
    })

What is the expected behavior?

It would be nice if I can do something like:

    .catch(err => {
      Sentry.captureException(err, () => context.fail())
    })

Or something globally handle callback.

@vietbui vietbui changed the title [raven-node] Callback after capture exception (or event, message) [raven-node] Callback after capturing exception (or event, message) Jul 29, 2018

@kamilogorek

This comment has been minimized.

Copy link
Member

kamilogorek commented Aug 2, 2018

This may help I guess https://blog.sentry.io/2018/06/20/how-droplr-uses-sentry-to-debug-serverless (it's using old raven version, which had a callback, but I'm mostly pointing to a callbackWaitsForEmptyEventLoop flag.

There's no official way yet, as we're still trying things out in beta, but it's doable with this code:

import { init, getDefaultHub } from '@sentry/node';

init({
  dsn: 'https://my-dsn.com/1337'
});

exports.myHandler = async function(event, context) {
  // your code

  await getDefaultHub().getClient().captureException(error, getDefaultHub().getScope());
  context.fail();
}
@vietbui

This comment has been minimized.

Copy link
Author

vietbui commented Aug 6, 2018

@kamilogorek Thank you for the pointer. I'll give it a try and play back the learnings.

@vietbui

This comment has been minimized.

Copy link
Author

vietbui commented Aug 7, 2018

@kamilogorek You suggestion works. I'm looking forward to a more official way.

@vietbui vietbui changed the title [raven-node] Callback after capturing exception (or event, message) [@sentry/node] Callback after capturing exception (or event, message) Aug 7, 2018

@HazAT

This comment has been minimized.

Copy link
Member

HazAT commented Sep 7, 2018

@vietbui
In 4.0.0-rc.1 we introduced a function on the client called close, you call it like this:

import { getCurrentHub } from '@sentry/node';

getCurrentHub().getClient().close(2000).then(result => {
      if (!result) {
        console.log('We reached the timeout for emptying the request buffer, still exiting now!');
      }
      global.process.exit(1);
})

close will wait until all requests are sent, it will always resolve (result = false timeout was reached), up until the timeout is reached (in this example 2000ms).
This is our official API.
While the prev approach will still work, the close method works for all cases.

@HazAT HazAT closed this Sep 7, 2018

@vietbui

This comment has been minimized.

Copy link
Author

vietbui commented Sep 13, 2018

@HazAT Nice one. Thanks for all the hard work.

@vietbui

This comment has been minimized.

Copy link
Author

vietbui commented Sep 21, 2018

In 4.0.3 I call it like this in my lambda function:

try {
  ...
} catch (err) {
  await getCurrentHub().getClient().captureException(err, getCurrentHub().getScope())
  throw err
}

getDefaultHub() is no longer available.

@kamilogorek

This comment has been minimized.

Copy link
Member

kamilogorek commented Sep 24, 2018

@vietbui it's called getCurrentHub now, as we had to unify our API with other languages SDKs.

@vietbui

This comment has been minimized.

Copy link
Author

vietbui commented Sep 26, 2018

@kamilogorek Thanks for the clarification. There is a problem with getCurrentHub approach as somehow the scope I set up did not end up in Sentry.

In the end I took a different approach as suggested by @HazAT to capture exception in my lambda functions:

try {
  ...
} catch (err) {
  Sentry.captureException(err)
  await new Promise(resolve => Sentry.getCurrentHub().getClient().close(2000).then(resolve))
  throw err
}

And it works perfectly.

@albinekb

This comment has been minimized.

Copy link

albinekb commented Oct 3, 2018

Is this the recommended way to wait/force sentry to send events?

@kamilogorek

This comment has been minimized.

Copy link
Member

kamilogorek commented Oct 3, 2018

@Andriy-Kulak

This comment has been minimized.

Copy link

Andriy-Kulak commented Nov 11, 2018

@albinekb yes – https://docs.sentry.io/learn/draining/?platform=browser

This solution does not work for me for some reason. It only works the first time in production when there is a cold start time and does not work after that. here is example code

'use strict'

const Sentry =  require('@sentry/node')
Sentry.init({
  dsn: 'xxx',
  environment: process.env.STAGE
});

module.exports.createPlaylist = async (event, context, callback) => {
  context.callbackWaitsForEmptyEventLoop = false
  if(!event.body) {
    Sentry.captureException(error)
    await new Promise(resolve => Sentry.getCurrentHub().getClient().close(2000).then(resolve))
    return {
      statusCode: 500,
      headers: { 'Content-Type': 'text/plain' },
      body: 'Missing body parameters'
    }
  }
  return {
    statusCode: 200,
  }
};
@albinekb

This comment has been minimized.

Copy link

albinekb commented Nov 12, 2018

@Andriy-Kulak Thats also stated in the docs:

After shutdown the client cannot be used any more so make sure to only do that right before you shut down the application.

So I don't know how we can handle this in lambda where we don't know when the application will be killed. Best would be to drain sentry per request like we could with the old API?

@LinusU

This comment has been minimized.

Copy link

LinusU commented Nov 12, 2018

@HazAT could we reopen this, please? I think it's important to have a way to work with this on Lambda, which is becoming an increasingly common target to deploy to.

This is currently blocking me from upgrading to the latest version...

Personally, I would prefer being able to get a Promise/callback when reporting an error. Having a way to drain the queue without actually closing it afterward would be the next best thing...

What was the rationale of removing the callback from captureException?

@Andriy-Kulak

This comment has been minimized.

Copy link

Andriy-Kulak commented Nov 12, 2018

@albinekb it does not work at all if I remove the following line

await new Promise(resolve => Sentry.getCurrentHub().getClient().close(2000).then(resolve))

@LinusU what is the solution and sentry or raven solution are you using?

For me basically the following works with sentry/node @4.3.0, but I have to make lambda manually wait some period of time (in this case I put 2 seconds) for sentry to do what it needs to do. Which I am not sure why it needs to be there because we are awaiting for sentry to finish the captureException request. If I don't have the waiting period afterwards, then sentry does not seem to send the error.

'use strict'

const Sentry =  require('@sentry/node')
Sentry.init({
  dsn: 'xxx',
  environment: process.env.STAGE
});

module.exports.createPlaylist = async (event, context, callback) => {
  context.callbackWaitsForEmptyEventLoop = false
  if(!event.body) {
    const error = new Error('Missing body parameters in createPlaylist')
    await Sentry.captureException(error)
    await new Promise(resolve => {setTimeout(resolve, 2000)})
    return {
      statusCode: 500,
      headers: { 'Content-Type': 'text/plain' },
      body: 'Missing body parameters'
    }
  }
  return {
    statusCode: 200,
  }
};
@dwelch2344

This comment has been minimized.

Copy link

dwelch2344 commented Nov 12, 2018

We're also getting burned on this on Lambda. We started with the new libs and are totally boxed out, considering to go back to Raven. We're writing tests right now to attempt to close the hub and then reinitialize, which would be a workable workaround if it holds water. But still hacky / likely to cause problems under load.

Personally I'd prefer some sort of flush() that returns a promise – hard to find a downside. Think it'd ever happen?

@LinusU

This comment has been minimized.

Copy link

LinusU commented Nov 12, 2018

what is the solution and sentry or raven solution are you using?

I'm using the following express error handler:

app.use((err: any, req: express.Request, res: express.Response, next: express.NextFunction) => {
  let status = (err.status || err.statusCode || 500) as number

  if (process.env.NODE_ENV === 'test') {
    return next(err)
  }

  if (status < 400 || status >= 500) {
    Raven.captureException(err, () => next(err))
  } else {
    next(err)
  }
})

I'm then using scandium to deploy the Express app to Lambda

edit: this is with Raven "raven": "^2.6.3",

@LinusU

This comment has been minimized.

Copy link

LinusU commented Nov 13, 2018

The dream API would be something like this 😍

Sentry.captureException(err: Error): Promise<void>
@kamilogorek

This comment has been minimized.

Copy link
Member

kamilogorek commented Nov 13, 2018

@LinusU https://github.com/getsentry/sentry-javascript/blob/master/packages/core/src/baseclient.ts#L145-L152 🙂

You have to use client instance directly to get it though. The reason for this is that decided that the main scenario is a "fire and forget" type of behavior, thus it's not an async method. Internally however, we do have async API which we use ourselves.

@LinusU

This comment has been minimized.

Copy link

LinusU commented Nov 13, 2018

Seems that what I actually want is something more like:

const backend = client.getBackend()
const event = await backend.eventFromException(error)
await client.processEvent(event, finalEvent => backend.sendEvent(finalEvent))

In order to skip all the queueing and buffering...

I get that the design is tailored to "fire and forgot", and for running in a long-running server, and it's probably quite good at that since it does a lot of buffering, etc. The problem is that this is the exact opposite that you want for Lambda, App Engine, and other "serverless" architectures, which are becoming more and more common.

Would it be possible to have a special method that sends the event as fast as possible, and returns a Promise that we can await? That would be perfect for the serverless scenarios!

class Sentry {
  // ...

  async unbufferedCaptureException(err: Error): Promise<void> {
    const backend = this.client.getBackend()
    const event = await backend.eventFromException(error)
    await this.client.processEvent(event, finalEvent => backend.sendEvent(finalEvent))
  }

  // ...
}
@kamilogorek

This comment has been minimized.

Copy link
Member

kamilogorek commented Nov 13, 2018

@LinusU we'll most likely create a specific serverless package for this scenario. We just need to find some time, as it's the end of the year and things are getting crowdy now. Will keep you posted!

@LinusU

This comment has been minimized.

Copy link

LinusU commented Nov 13, 2018

we'll most likely create a specific serverless package for this scenario

That would be amazing! 😍

@kamilogorek kamilogorek changed the title [@sentry/node] Callback after capturing exception (or event, message) [@sentry/node] AWS Lambda and other Serverless solutions support Nov 13, 2018

@kamilogorek

This comment has been minimized.

Copy link
Member

kamilogorek commented Nov 20, 2018

@mtford90

when exactly would I use this better solution? As far as I know it's not possible to know when the lambda will be shutdown - plus it seems silly to wait for an arbitrary amount of time for shutdown to allow sentry to do its thing - especially on expensive high memory/cpu lambda functions.

(talking about draining)

It's meant to be used as the last thing before closing down the server process. Timeout in drain method is maximum time that we'll wait before shutting down the process, which doesn't mean that we will always use up that time. If the server is fully responsive, it'll send all the remaining events right away.

There's no way to know this per se, but there's a way to tell the lambda when it should be shut down using handler's callback argument.

Also @LinusU, I re-read your previous comment, specifically this part:

Would it be possible to have a special method that sends the event as fast as possible, and returns a Promise that we can await? That would be perfect for the serverless scenarios!

This is how we implemented our buffer. Every captureX call on the client, will add it to the buffer, that's correct, but it's not queued in any way, it's executed right away and this pattern is only used so that we can get the information if everything was successfully sent through to Sentry.

public async add(task: Promise<T>): Promise<T> {
if (this.buffer.indexOf(task) === -1) {
this.buffer.push(task);
}
task.then(async () => this.remove(task)).catch(async () => this.remove(task));
return task;
}

This means that if you do something like this in AWS Lambda (assuming you want to use default client, which is the simplest case):

import * as Sentry from '@sentry/browser';

Sentry.init({ dsn: '__YOUR_DSN__' });

exports.handler = (event, context, callback) => {
    try {
      // do something
    catch (err) {
      Sentry.getCurrentHub()
        .getClient()
        .captureException(err)
        .then((status) => {
          // request status
          callback(null, 'Hello from Lambda');
        })
    }
};

You can be sure that it was sent right away and there was no timing/processing overhead.

@jviolas

This comment has been minimized.

Copy link

jviolas commented Nov 20, 2018

@kamilogorek
Does this mean something like this should work in a async/await handler (where you don't use the callback)?

import * as Sentry from '@sentry/node';

Sentry.init({ dsn: '__YOUR_DSN__' });

exports.handler = async (event, context) => {
    try {
      // do something

      return 'Hello from Lambda';
    catch (err) {
      await Sentry.getCurrentHub().getClient().captureException(err);
      return 'Hello from Lambda with error';
    }
};
@jviolas

This comment has been minimized.

Copy link

jviolas commented Nov 22, 2018

Clearly what we're missing here is some documentation and good practice in this kind of advanced use cases. It'll be really good when it'll be documented or even a blog post can be a good start.
Otherwise, the new SDK is really simple to use and the unification is really nice.

@rreynier

This comment has been minimized.

Copy link

rreynier commented Dec 18, 2018

@kamilogorek
Does this mean something like this should work in a async/await handler (where you don't use the callback)?

import * as Sentry from '@sentry/node';

Sentry.init({ dsn: '__YOUR_DSN__' });

exports.handler = async (event, context) => {
    try {
      // do something

      return 'Hello from Lambda';
    catch (err) {
      await Sentry.getCurrentHub().getClient().captureException(err);
      return 'Hello from Lambda with error';
    }
};

Doing something as suggested above does work, except I am unable to add extra context. For example, if I do:

Sentry.configureScope(scope => {
   scope.setExtra('someExtraInformation', information);
});
await Sentry.getCurrentHub().getClient().captureException(err);

I will not actually see 'someExtraInformation' in Sentry.

Someone did suggest an alternative method at the top of this thread, and that works, but seems hacky (forcing a timeout).

Sentry.configureScope(scope => {
  scope.setExtra('someExtraInformation', information);
});
Sentry.captureException(error);
await new Promise(resolve => Sentry.getCurrentHub().getClient().close(2000).then(resolve));
@guillaumekh

This comment has been minimized.

Copy link

guillaumekh commented Dec 28, 2018

@kamilogorek @jviolas

import * as Sentry from '@sentry/node';

Sentry.init({ dsn: '__YOUR_DSN__' });

exports.handler = async (event, context) => {
   try {
     // do something

     return 'Hello from Lambda';
   catch (err) {
     await Sentry.getCurrentHub().getClient().captureException(err);
     return 'Hello from Lambda with error';
   }
};

Can this be also applied to uncaught exceptions ? It seems modifying the Sentry.Integrations.OnUncaughtException integration is the official way to do so but the documentation is pretty poor right now.

@rdsedmundo

This comment has been minimized.

Copy link

rdsedmundo commented Jan 20, 2019

+1 for this. At least having something officially documented would be good. Serverless is growing fast as of 2019, I really want to see official support from Sentry on it. One of the ideas I read here and really have liked was to having something like Sentry.flush() to send all the events that are queued.

@HazAT

This comment has been minimized.

Copy link
Member

HazAT commented Jan 22, 2019

@rdsedmundo Can you elaborate why this approach isn't working for you?

import * as Sentry from '@sentry/node';

Sentry.getCurrentHub().getClient().close(2000).then(result => {
      if (!result) {
        console.log('We reached the timeout for emptying the request buffer, still exiting now!');
      }
      global.process.exit(1);
})

This is our official approach and basically Sentry.flush().
ref: https://docs.sentry.io/error-reporting/configuration/draining/?platform=javascript

@rdsedmundo

This comment has been minimized.

Copy link

rdsedmundo commented Jan 22, 2019

@HazAT The problem with that comes when you think about AWS Lambda container reuse. Which in TL;DR terms means that a process that just served a request can serve a new brand one if it's made on a short window of time. If I close the connection with this snippet you gave, and the container is reused, I'd need to manage to create a new hub for the new request. I can easily see this getting tricky. That's why a simple await Sentry.flush() would be a better solution:

import Sentry from './sentry'; // this calls Sentry.init under the hood

export const handler = async (event, context) => {
  try {
    ...
  } catch (error) {
    Sentry.captureException(error);
    await Sentry.flush(); // could even be called on the finally block

    return formatError(error);
  }
}
@HazAT

This comment has been minimized.

Copy link
Member

HazAT commented Jan 22, 2019

@rdsedmundo I am not sure if I maybe misunderstanding something but if you do

import Sentry from './sentry'; // this calls Sentry.init under the hood

export const handler = async (event, context) => {
  try {
    ...
  } catch (error) {
    Sentry.captureException(error);
    await Sentry.getCurrentHub().getClient().close(2000);

    return formatError(error);
  }
}

It's exactly like await Sentry.flush only that you define the timeout.

The promise resolves after 2000ms for sure with false if there was still stuff in the queue.
Otherwise close will resolve with true if the queue has been drained before the timeout is reached.

Or will the container be reused before all promises are resolved? (I can't imagine that)

@LinusU

This comment has been minimized.

Copy link

LinusU commented Jan 22, 2019

@HazAT isn't the problem that close(...) will prevent the client from being used again? Lambda reuses the same Node process so the calls would be something like this, which I guess will stop working after the first call to close?

  • Sentry.init()
  • Sentry.captureException()
  • Sentry.getCurrentHub().getClient().close()
  • Sentry.captureException()
  • Sentry.getCurrentHub().getClient().close()
  • Sentry.captureException()
  • Sentry.getCurrentHub().getClient().close()
  • Sentry.captureException()
  • Sentry.getCurrentHub().getClient().close()
  • ...
@HazAT

This comment has been minimized.

Copy link
Member

HazAT commented Jan 22, 2019

No, close doesn't dispose the client, it's just here for draining the transport queue.
I agree that the name close in this context may be misleading but at least in JS/Node close doesn't do anything with the client and it's perfectly fine to still use it afterward.

Edit: If that was actually the "issue" I will update the docs to make this clear.

@rdsedmundo

This comment has been minimized.

Copy link

rdsedmundo commented Jan 22, 2019

Cool. But the documentation is wrong then:

After shutdown the client cannot be used any more so make sure to only do that right before you shut down the application.
@HazAT

This comment has been minimized.

Copy link
Member

HazAT commented Jan 22, 2019

OK, we just discussed this matter internally in the team.
You guys were right and while JavaScript right now doesn't behave the way we documented it 🙈 we will introduce a flush function which will do exactly what you expect.

So right now you can use close without any issues (not sure if we are going to change it to dispose/disable the client in the future).
But there will be a flush function which is there to just flush the queue.

I will update this issue once the feature landed.

@ondrowan

This comment has been minimized.

Copy link

ondrowan commented Jan 24, 2019

Since I got a bit lost in all of these comments, is this how Express error handler (mimicing the one from this repo) should look like?

function getStatusCodeFromResponse(error) {
    const statusCode = error.status || error.statusCode || error.status_code || (error.output && error.output.statusCode);
    return statusCode ? parseInt(statusCode, 10) : 500;
}

app.use(async (err, req, res, next) => {
    const status = getStatusCodeFromResponse(err);

    if (status >= 500) {
        Sentry.captureException(err)

        await Sentry.getCurrentHub().getClient().close(2000)
    }

    next(err)
})

It looks like it's working and it doesn't lose extra data as in @rreynier's code.

@LinusU

This comment has been minimized.

Copy link

LinusU commented Jan 25, 2019

Personally I feel that

await Sentry.getCurrentHub().getClient().captureException(err)

is cleaner than:

Sentry.captureException(err)
await Sentry.getCurrentHub().getClient().close(2000)

close really reads like it will close the client...

Full example:

import * as Sentry from '@sentry/node'

// ...

Sentry.init({ dsn: config.SENTRY_DSN })

// ...

app.use((err: any, req: express.Request, res: express.Response, next: express.NextFunction) => {
  let status = (err.status || err.statusCode || 500) as number

  if (process.env.NODE_ENV === 'test') {
    return next(err)
  }

  if (status < 400 || status >= 500) {
    Sentry.getCurrentHub().getClient().captureException(err).then(() => next(err))
  } else {
    next(err)
  }
})
@ondrowan

This comment has been minimized.

Copy link

ondrowan commented Jan 25, 2019

@LinusU I tried that and for some reason, it doesn't send extra data along with the stack trace. It basically sends just stack trace. No info about user, OS or anything.

@LinusU

This comment has been minimized.

Copy link

LinusU commented Jan 25, 2019

Aha, that's not good at all 😞

@pimterry

This comment has been minimized.

Copy link

pimterry commented Jan 31, 2019

While we wait for flush, as a more reliable workaround than both of the above options you can report and wait for the result, and include the scope, using the below snippet:

const scope = Sentry.getCurrentHub().getScope();
await Sentry.getCurrentHub().getClient().captureException(error, scope);

I'm using this, and it seems to work reliably for me, with reported errors including everything I'd expect.

I'm actually using all this with Netlify Functions, but the theory is the same with Lambda etc. I've written up a post with the full details of how to get this working, if anybody is interested: https://httptoolkit.tech/blog/netlify-function-error-reporting-with-sentry/

@zeusdeux

This comment has been minimized.

Copy link

zeusdeux commented Feb 1, 2019

I use this helper in all my lambdas currently.

@HazAT HazAT pinned this issue Feb 1, 2019

@ondrowan

This comment has been minimized.

Copy link

ondrowan commented Feb 2, 2019

@pimterry Isn't this basically the same solution as @LinusU suggested? I've tried it and it doesn't send extra data as well.

@zeusdeux

This comment has been minimized.

Copy link

zeusdeux commented Feb 2, 2019

This approach has worked out for me thus far @ondrowan

@pimterry

This comment has been minimized.

Copy link

pimterry commented Feb 2, 2019

@ondrowan it's the same, but manually grabbing and including the current scope. That should be enough to get you working exceptions though I think. With the previous version, I was getting unlabelled events, and now with this change my exceptions come through with all the extra normal details.

@kamilogorek

This comment has been minimized.

Copy link
Member

kamilogorek commented Feb 14, 2019

@vietbui @albinekb @Andriy-Kulak @LinusU @dwelch2344 @jviolas @rreynier @guillaumekh @rdsedmundo @ondrowan @pimterry @zeusdeux not sure who's still interested in this use-case, so excuse me if I shouldn't call you.

Starting 4.6.0, there's no more client/hub dance. You can just call any our captureX method and then use Sentry.flush() to await the response once everything is sent to the server. All scope/extra data should be preserved without any dev interaction.

Here's an example with succeeded/timed-out requests.

image

Hope it helps! :)

@LinusU

This comment has been minimized.

Copy link

LinusU commented Feb 14, 2019

Nice!

Are there still plans on making a minimal package just for capturing exceptions from Lambda and other serverless solutions? I think that that would still be a really nice addition ❤️

@kamilogorek

This comment has been minimized.

Copy link
Member

kamilogorek commented Feb 15, 2019

@LinusU hopefully yes, but we are swamped with other languages SDKs right now 😅

@HazAT

This comment has been minimized.

Copy link
Member

HazAT commented Feb 15, 2019

Thanks all for all the possible solutions, tl;dr for everyone coming here

Use: await Sentry.flush() to send all pending requests, this has been introduced in 4.6.x.

Closing this, please feel free to open a new issue in case anything is missing (but this thread is already super long).

Cheers 👍 🎉

@HazAT HazAT closed this Feb 15, 2019

@HazAT HazAT unpinned this issue Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment