numo-labs / aws-lambda-helper

:lollipop: Collection of helper methods for lambda
GNU General Public License v3.0
22 stars 2 forks source link

Can we add a method to helper to simplify error handling in our lambdas? #25

Closed nelsonic closed 8 years ago

nelsonic commented 8 years ago

Can be as simple as: (though we can always add more if we need formatting for Kibana)

helper.error = function (err, event, context) {
  if (err) {
    context.fail(err); // halt processing ?
  } else {
    return; // no error so continue processing;
  }
}

this would allow use to 1-line error handling in our lambda. e.g:

exports.handler = function (event, context) {
  api_request(params, function (err, data) {
    helper.error(err, event, context); // error handled or continue. no if/else in the body of lambda.
    // continue processing data
  });
};
nikhilaravi commented 8 years ago

@nelsonic I agree with the error handling in one line suggestion!! Does the error helper need the event?

nelsonic commented 8 years ago

@nikhilaravi good question. I only put the event in there in case we want to log it as part of the fail message... but we can easily remove it.

Kumjami commented 8 years ago

Hi @nelsonic and @nikhilaravi. Here is my first "contribution". I would call it error interceptor, or something like that, to make it clear that it only response in case of error. I mean, by reading the code you don't know if its doing something or not when there isn't error.

exports.handler = function (event, context) {
  api_request(params, function (err, data) {
     // option 1
     err ? helper.error(err, event, context) : continue; // you know how the system reacts (the inside "if" statement becomes unnecessary)
     // option 2
     helper.errorInterceptor(err, event, context); // in this case it's clear that this will intercept errors, nothing else

     // continue processing data
  });
};

I don't know how do you usually do things so... sorry in advance if this doesn't mean something to you.

Kumjami commented 8 years ago

Btw, to implement it, I would suggest to create a private npm module where we could have a single instance of our "error factory" so we could just add the dependency to all the repositories where we would like to use it instead of duplicating code. Then we would have a single repository to update for adding new log info, updating format or whatever. But I don't know if there would be necessary then to update every lambda function dependencies.

Note: It is not necessary to create a private npm module, you can use git url in package.json dependencies, but its less stylish in my opinion :P

nelsonic commented 8 years ago

@Kumjami, yes, I agree that we need an intuitive name for the method.

The error handler does not need to be private NPM project, aws-lambda-helper is public because it contains no "sensative" or "proprietary" information. (we try to make as much as possible public...)

We are trying to avoid conditional execution ("branching") in our Lambdas so case 1 in your example above would not improve the situation.

Would you agree that failOnError describes the functionality more accurately?

Also, can you clarify what the advantage of a "factory" in this situation?

Kumjami commented 8 years ago

I see a factory useful if we would have several error types depending on the error data (with different behavior). But I think that I went out of the scope here. And I really didn't mean a factory itself, I was thinking in a repository with all the possible error handlers.

About private NPM, I like the idea of doing everything public. But maybe managers don't want public code (happened to me).

I like failOnError much more than error, so it looks good to me.

Kumjami commented 8 years ago

ok! I get the idea now. in this code:

helper.error = function (err, event, context) {
  if (err) {
    context.fail(err); // halt processing ?
  } else {
    return; // no error so continue processing;
  }
}

helper makes reference to aws-lambda-helper package... that will be used by every "lambda project". Please, forget what I said before @nelsonic

nelsonic commented 8 years ago

Nah, its my fault for making the assumption that it was clear which package I was referring to ... Yes, I'm proposing adding a method to aws-lambda-helper to handle errors on a single line so we don't have to do if/else in our lambda functions...

@Kumjami do you want to add this and create a PR with tests and docs (readme) ?

Kumjami commented 8 years ago

@nelsonic ok, I will!

nelsonic commented 8 years ago

@Kumjami we prefer to use the word "closes" or "fixes" in our commit messages so that the issue it relates to only gets (automatically) closed after the PR is merged.

Kumjami commented 8 years ago

The thing is that I screwed it with git. I don't know if It has been because I have tried to do the PR directly from sourcetree or what. But It was closed, and even merged automatically, without the PR, so I had to revert changes, and redo, but the issue was closed automatically when It was merged the first time.

nelsonic commented 8 years ago

@Kumjami yeah, don't worry. We know you still don't have a dev laptop you can be productive with. hopefully soon, hey? :-)