Skip to content
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

Detect API Gateway errors #347

Open
jeffggardner opened this issue Sep 16, 2019 · 6 comments
Open

Detect API Gateway errors #347

jeffggardner opened this issue Sep 16, 2019 · 6 comments

Comments

@jeffggardner
Copy link

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 added a commit to jeffggardner/iopipe-js-core that referenced this issue Sep 17, 2019
@jeffggardner
Copy link
Author

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

@mrickard
Copy link
Contributor

@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
Copy link
Author

PR submitted and I fixed the package name :)

@kolanos
Copy link
Contributor

kolanos commented Sep 17, 2019

@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:
iopipe/iopipe-python#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.

@kolanos kolanos closed this as completed Sep 17, 2019
@kolanos kolanos reopened this Sep 17, 2019
@jeffggardner
Copy link
Author

@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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants