Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions src/Service/Logger.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,54 @@ export default class LoggerService extends DependencyAwareClass {
return this.winston;
}

/**
* While handling an error, lambda wrapper should
* recognise axios errors and trim down the information.
*
* Keep the following keys:
* - message.config
* - message.message
* - message.response?.status
* - message.response?.data
*
* @param {object} error
*/
static processAxiosError(error) {
const processed = {
config: error.config,
message: error.message,
};

// It's pretty common for axios errors
// to not have.response e.g.when there's
// a network error or timeout.
// These errors will have .request but not .response.
if (error.response) {
processed.response = {
status: error.response.status,
data: error.response.data,
};
}

return processed;
}

/**
* Transform the original message
* before it is passed to the winston logger
*
* @param {string|object} message
*/
processMessage(message = '') {
let processed = message;

if (processed && processed.isAxiosError) {
processed = this.constructor.processAxiosError(processed);
}

return processed;
}

/**
* Log Error Message
*
Expand All @@ -130,7 +178,7 @@ export default class LoggerService extends DependencyAwareClass {
Epsagon.setError(error);
}

this.logger.log('error', message, { error });
this.logger.log('error', message, { error: this.processMessage(error) });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the naming is a bit confusing. We have a method that takes error and message, but its error that we pass to another method called processMessage

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree. How would you name the method?

Copy link
Copy Markdown
Contributor

@tomisaacmarshall tomisaacmarshall Dec 14, 2020

Choose a reason for hiding this comment

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

🤣 erm .... well ... maybe just have a processError method. And check it's an error first.

Like, would we ever want to process a string really?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we need the same functionality for logger.info as it is reasonable a function might call it with an axios error (I know this happens in a couple of places)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, am aware of that ... was envisaging the check would happen there as well.

this.label('error', true);
this.metric('error', 'error', true);
}
Expand All @@ -150,7 +198,7 @@ export default class LoggerService extends DependencyAwareClass {
* @param message string
*/
info(message) {
this.logger.log('info', message);
this.logger.log('info', this.processMessage(message));
}

/**
Expand Down
27 changes: 2 additions & 25 deletions src/Wrapper/LambdaWrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,33 +25,10 @@ import ResponseModel from '../Model/Response.model';
export const handleError = (di, error) => {
const logger = di.get(DEFINITIONS.LOGGER);

let errorBody = error;

// While handling an error, lambda wrapper should
// recognise axios errors and trim down the information
if (errorBody.isAxiosError) {
// only keep error.config, error.response.status, error.response.data
errorBody = {
config: error.config,
message: error.message,
};

// It's pretty common for axios errors
// to not have.response e.g.when there's
// a network error or timeout.
// These errors will have .request but not .response.
if (error.response) {
errorBody.response = {
status: error.response.status,
data: error.response.data,
};
}
}

if (error.raiseOnEpsagon || !error.code || error.code >= 500) {
logger.error(errorBody);
logger.error(error);
} else {
logger.info(errorBody);
logger.info(error);
}

const responseDetails = {
Expand Down
78 changes: 78 additions & 0 deletions tests/unit/Service/Logger.service.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ const getLogger = (event = getEvent, context = getContext) => new LoggerService(
describe('Service/LoggerService', () => {
const context = { invokedFunctionArn: 'my-function' };

const axiosResponses = {
UNDEFINED: undefined,
EMPTY: {},
HTTP_417: {
status: 417,
data: { data: 1 },
extra: 2,
},
};

afterEach(() => jest.clearAllMocks());

describe('constructor', () => {
Expand Down Expand Up @@ -55,4 +65,72 @@ describe('Service/LoggerService', () => {
expect(Winston.createLogger).toHaveBeenCalledTimes(1);
});
});

describe('error', () => {
Object.entries(axiosResponses).forEach(([key, axiosResponse]) => {
it(`Trims down the axios error: ${key}`, () => {
const logger = getLogger();
const log = jest.fn();

jest.spyOn(logger, 'logger', 'get').mockReturnValue({ log });

const error = {
isAxiosError: true,
raiseOnEpsagon: true,
config: {
url: 'http://localhost:9999',
method: 'get',
},
extra: 1,
response: axiosResponse,
message: 'some-message',
};

logger.error(error);

const loggerCall = log.mock.calls[0][2].error;

expect(loggerCall).toMatchSnapshot();
expect('extra' in loggerCall).toEqual(false);

if (axiosResponse) {
expect('extra' in loggerCall.response).toEqual(false);
}
});
});
});

describe('info', () => {
Object.entries(axiosResponses).forEach(([key, axiosResponse]) => {
it(`Trims down the axios error: ${key}`, () => {
const logger = getLogger();
const log = jest.fn();

jest.spyOn(logger, 'logger', 'get').mockReturnValue({ log });

const error = {
isAxiosError: true,
raiseOnEpsagon: true,
config: {
url: 'http://localhost:9999',
method: 'get',
},
extra: 1,
response: axiosResponse,
message: 'some-message',
};

logger.info(error);

const loggerCall = log.mock.calls[0][1];

expect(loggerCall).toMatchSnapshot();
expect('extra' in loggerCall).toEqual(false);

if (axiosResponse) {
expect('extra' in loggerCall.response).toEqual(false);
}
});
});
});
});
81 changes: 81 additions & 0 deletions tests/unit/Service/__snapshots__/Logger.service.test.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Service/LoggerService error Trims down the axios error: EMPTY 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
"response": Object {
"data": undefined,
"status": undefined,
},
}
`;

exports[`Service/LoggerService error Trims down the axios error: HTTP_417 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
"response": Object {
"data": Object {
"data": 1,
},
"status": 417,
},
}
`;

exports[`Service/LoggerService error Trims down the axios error: UNDEFINED 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
}
`;

exports[`Service/LoggerService info Trims down the axios error: EMPTY 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
"response": Object {
"data": undefined,
"status": undefined,
},
}
`;

exports[`Service/LoggerService info Trims down the axios error: HTTP_417 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
"response": Object {
"data": Object {
"data": 1,
},
"status": 417,
},
}
`;

exports[`Service/LoggerService info Trims down the axios error: UNDEFINED 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
}
`;
42 changes: 0 additions & 42 deletions tests/unit/Wrapper/LambdaWrapper.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,48 +58,6 @@ describe('Wrapper/LambdaWrapper', () => {
});
});
});

describe('Axios Errors', () => {
const axiosResponses = {
UNDEFINED: undefined,
EMPTY: {},
HTTP_417: {
status: 417,
data: { data: 1 },
extra: 2,
},
};

Object.entries(axiosResponses).forEach(([key, axiosResponse]) => {
it(`Trims down the axios error: ${key}`, () => {
const di = getMockedDi();
const logger = di.get(DEFINITIONS.LOGGER);

const error = {
isAxiosError: true,
raiseOnEpsagon: true,
config: {
url: 'http://localhost:9999',
method: 'get',
},
extra: 1,
response: axiosResponse,
message: 'some-message',
};

handleError(di, error);

const loggerCall = logger.error.mock.calls[0][0];

expect(loggerCall).toMatchSnapshot();
expect('extra' in loggerCall).toEqual(false);

if (axiosResponse) {
expect('extra' in loggerCall.response).toEqual(false);
}
});
});
});
});

describe('LambdaWrapper', () => {
Expand Down
40 changes: 0 additions & 40 deletions tests/unit/Wrapper/__snapshots__/LambdaWrapper.test.js.snap
Original file line number Diff line number Diff line change
@@ -1,45 +1,5 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Wrapper/LambdaWrapper handleError Axios Errors Trims down the axios error: EMPTY 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
"response": Object {
"data": undefined,
"status": undefined,
},
}
`;

exports[`Wrapper/LambdaWrapper handleError Axios Errors Trims down the axios error: HTTP_417 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
"response": Object {
"data": Object {
"data": 1,
},
"status": 417,
},
}
`;

exports[`Wrapper/LambdaWrapper handleError Axios Errors Trims down the axios error: UNDEFINED 1`] = `
Object {
"config": Object {
"method": "get",
"url": "http://localhost:9999",
},
"message": "some-message",
}
`;

exports[`Wrapper/LambdaWrapper handleError Generates a response object 1`] = `
Object {
"body": "{\\"data\\":{},\\"message\\":\\"unknown error\\"}",
Expand Down