iopipe / iopipe-js-core

Observe and develop serverless apps with confidence on AWS Lambda with Tracing, Metrics, Profiling, Monitoring, and more.
https://www.iopipe.com/
Apache License 2.0
124 stars 20 forks source link

Detect API Gateway errors #347

Open jeffggardner opened 5 years ago

jeffggardner commented 5 years ago

Description When using the "Lambda Proxy Integration" of AWS API Gateway, the lambda must return a response per this document: https://docs.aws.amazon.com/apigateway/latest/developerguide/set-up-lambda-proxy-integrations.html#api-gateway-simple-proxy-for-lambda-output-format

So instead of throwing an error or calling context.fail(err); our lambdas return a response like:

exports.handler = async (event, context) => {
try {
   <code here throws exception>
  } catch (err) {
    logger.error('------ There was an error');
    logger.error(err);

    response.statusCode = HttpStatus.INTERNAL_SERVER_ERROR;
    response.body = 'There was an error';

    return response; 
  } 
}

Unfortunately, iopipe reports errors in our process as successful transactions as it is expecting to trap an error or for context.fail(err); to be thrown.

Steps to reproduce the issue:

  1. Create a lambda and an API gateway endpoint using "Lambda Proxy Integration"
  2. Have the lambda send a properly formatted API Gateway reponse
  3. Check iopipe dashboard for errors

Describe the results you received: While the transactions appear in iopipe, they are not recognized as errors.

Describe the results you expected: I would like to see them as errors.

p.s. I have filed issues in other iopipe repos but I will close them if needed

Additional information you deem important (e.g. issue happens only occasionally):

jeffggardner commented 5 years ago

I put together an idea on how you might address this: https://github.com/jeffggardner/iopipe-js-core/tree/fix-347

mrickard commented 5 years ago

@jeffggardner Great--thanks for supplying this! Would you mind making it a PR against this repo?

The only thing that jumps out as a potential integration issue is that the change to the package name would interfere with our tooling. But I'll pull this down and run some tests. Thank you!

jeffggardner commented 5 years ago

PR submitted and I fixed the package name :)

kolanos commented 5 years ago

@jeffggardner There's a couple ways to handle this, I think. In the Python agent there is a new feature in the Event Info plugin to collect response meta data, such as status codes. This hasn't made it's way to the Node plugin yet, but would allow you to configure alerts for specific status codes. Python PR is here: https://github.com/iopipe/iopipe-python/pull/341

Another option is that the Python agent provides an context.iopipe.error() method to report handled errors. All it does is send the report to IOpipe, it doesn't return any values to Lambda. Unfortunately, context.iopipe.fail() does call the Lambda callback, so it doesn't behave the same way. There is a context.iopipe.sendReport(), though, which could be used to send an error back to IOpipe without calling the callback. We could add an context.iopipe.error() method which would just call sendReport() with an error to make the APIs consistent.

Edit: Accidentally closed the issue when commenting. Re-opened.

jeffggardner commented 5 years ago

@mrickard I'm under a bit of a time crunch so my plan is to deploy my changes to our private repos and to at least keep moving forward unless you think my recommendation is something you feel could be implemented in the next week or so. LMK

mrickard commented 5 years ago

@jeffggardner We're looking at leveraging our event-info plugin for customizable responses. Since you're under a time crunch, I'd suggest keep moving forward on your side, and we'll keep you updated with progress on our side.