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

CS-1264 Improved Logging to debug Lambda Functions #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jwelborn-sugar
Copy link
Contributor

No description provided.

Copy link
Contributor

@rogeryang-sugarcrm rogeryang-sugarcrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the Lambda responses logged in Cloudwatch? I did a quick test on the Lambda test console and received the proper response, but I didn't see it logged

src/handlers/add-note-to-case.js Outdated Show resolved Hide resolved
}
}
} else {
body = ErrorMessages.ERROR_CANNOT_MATCH_CASE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to log why this message is thrown? For this one, we are missing the contactName from the user. I guess this question applies to all areas where this can be thrown

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My update adds a more detailed early return for the missing parameters, and an early return if we match more than one record. We'd only get here now if app.api.call returns a bean without an ID for some reason, and I'm not sure what would cause that.

src/handlers/case-status.js Outdated Show resolved Hide resolved
@jwelborn-sugar jwelborn-sugar force-pushed the CS-1264 branch 2 times, most recently from 9b7d60a to c3d9af8 Compare December 15, 2020 20:31
Comment on lines +40 to +43
let statusCode = HttpStatus.error;
let caseId = '';
let noteName = '';
let body = '';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved these to the top to use them in every early return path for consistency

Comment on lines 50 to 56
let filler = caseNumber ? '' : 'caseNumber ';
filler += contactId ? '' : 'contactId';
body = strUtils.generateMessage(ErrorMessages.TPL_MISSING_REQUIRED_PARAMETERS, filler);
return {
statusCode: HttpStatus.preconditionFailed
statusCode: HttpStatus.preconditionFailed,
caseId: caseId,
body: body
Copy link
Contributor Author

@jwelborn-sugar jwelborn-sugar Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return message will tell the user which parameters were missing.

Comment on lines 67 to 73
if (idResponse.data.records.length > 1) {
body = strUtils.generateMessage(ErrorMessages.TPL_MULTIPLE_RECORDS_MATCHED, 'Case');
return {
statusCode: statusCode,
caseId: caseId,
body: body
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could live in a case-record-utils that matched the design of our call-record-utils, but for the scope of the ticket I just added logging while trying to avoid changing the code structure.

Comment on lines -30 to -33
async function getCallRecord (contactId) {
let url = encodeURI(`Calls?filter[0][${CallsConstants.CALLS_AWS_CONTACT_ID}]=${contactId}`);
url = app.api.buildUrl(url);
const response = await app.api.call('read', url);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines are just a whitespace auto-format. I tried to split these into the second commit, but I missed this.

if (response.data) {
loggerUtils.logSugarApiResponse(response.data);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log every return from the Sugar API for debugging purposes

@jwelborn-sugar jwelborn-sugar force-pushed the CS-1264 branch 4 times, most recently from 637d54c to 99505a0 Compare December 16, 2020 15:04
CS-1264 Greatly increase logging in Lambda Functions

CS-1264 Log lambda return values
Comment on lines +53 to +57
return loggerUtils.logReturnValue({
statusCode: HttpStatus.preconditionFailed,
caseId: caseId,
body: body
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern is used to log return values to cloudwatch for debugging.

@@ -21,6 +21,6 @@
},
"scripts": {
"test": "jest --watchAll",
"lint": "eslint ./src ./__tests__"
"lint": "eslint --ext .js src/* __tests__/unit/* --fix"
Copy link
Contributor Author

@jwelborn-sugar jwelborn-sugar Dec 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this allows us to auto-fix style errors before committing with npm run lint. All of the changes in this commit were from running this command

Copy link
Contributor

@rogeryang-sugarcrm rogeryang-sugarcrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

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

Successfully merging this pull request may close these issues.

3 participants