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

feat(node): add exponential retry mechanism with idempotency headers #4462

Merged
merged 23 commits into from
Dec 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
41d77fd
feat(node): add basic retry mechanism
michaldziuba03 Oct 9, 2023
b66965a
feat: improve retry and add basic tests
michaldziuba03 Oct 11, 2023
503257c
feat(node): add retry condition callback and more tests
michaldziuba03 Oct 12, 2023
7475e22
feat(node): regenerate idempotency key on status 422
michaldziuba03 Oct 13, 2023
6670e32
test(node): add cases for common 4xx errors
michaldziuba03 Oct 21, 2023
41296c2
fix: merge and resolve git conflicts
michaldziuba03 Nov 2, 2023
071d59f
feat(node): improve retries feature
michaldziuba03 Nov 3, 2023
1323c04
Merge branch 'next' into nv-3009-add-exponential-retry
michaldziuba03 Nov 4, 2023
d70b9f8
Merge branch 'next' into nv-3009-add-exponential-retry
michaldziuba03 Nov 5, 2023
bf68b2d
Merge branch 'next' into nv-3009-add-exponential-retry
rifont Nov 7, 2023
3474d5a
Merge branch 'next' into nv-3009-add-exponential-retry
michaldziuba03 Nov 9, 2023
a3e150d
test(node): add more tests for retries feature
michaldziuba03 Nov 9, 2023
2d548b4
Merge branch 'novuhq:next' into nv-3009-add-exponential-retry
michaldziuba03 Nov 11, 2023
208d803
Merge branch 'next' into nv-3009-add-exponential-retry
michaldziuba03 Nov 18, 2023
bbcbb80
Merge branch 'next' into nv-3009-add-exponential-retry
rifont Nov 20, 2023
024d8cd
fix: retryable to cspell
michaldziuba03 Nov 20, 2023
60a000d
Merge branch 'next' into nv-3009-add-exponential-retry
michaldziuba03 Nov 20, 2023
8479ad0
Merge branch 'next' into nv-3009-add-exponential-retry
rifont Nov 21, 2023
b773f3a
Merge branch 'next' into nv-3009-add-exponential-retry
michaldziuba03 Nov 26, 2023
01649c2
Merge branch 'next' into nv-3009-add-exponential-retry
rifont Nov 29, 2023
fe525ae
Merge branch 'next' into nv-3009-add-exponential-retry
rifont Nov 29, 2023
c75140c
Merge branch 'next' into nv-3009-add-exponential-retry
michaldziuba03 Nov 30, 2023
1e8c1b5
Merge branch 'next' into nv-3009-add-exponential-retry
rifont Dec 1, 2023
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
3 changes: 3 additions & 0 deletions .cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,9 @@
"idempotency",
"IDEMPOTENCY",
"Idempotency",
"Retryable",
"RETRYABLE",
"retryable",
"messagebird",
"Datetime",
"pubid",
Expand Down
8 changes: 6 additions & 2 deletions packages/node/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,23 @@
},
"dependencies": {
"@novu/shared": "^0.21.0",
"axios-retry": "^3.8.0",
"handlebars": "^4.7.7",
"lodash.get": "^4.4.2",
"lodash.merge": "^4.6.2"
"lodash.merge": "^4.6.2",
"uuid": "^9.0.1"
michaldziuba03 marked this conversation as resolved.
Show resolved Hide resolved
},
"devDependencies": {
"@sendgrid/mail": "^7.4.6",
"@types/jest": "29.5.2",
"@types/lodash.get": "^4.4.6",
"@types/lodash.merge": "^4.6.6",
"@types/node": "^14.6.0",
"@types/uuid": "^8.3.4",
"axios": "^1.4.0",
"codecov": "^3.5.0",
"jest": "^27.0.6",
"nock": "^13.1.3",
"npm-run-all": "^4.1.5",
"open-cli": "^6.0.1",
"rimraf": "^3.0.2",
Expand All @@ -77,7 +81,7 @@
"preset": "ts-jest",
"testEnvironment": "node",
"moduleNameMapper": {
"axios": "axios/dist/node/axios.cjs"
"^axios$": "axios/dist/node/axios.cjs"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

without that change, tests for retries don't work.

I observed strange behavior - it sends GET request to 127.0.0.1:80 even when you send POST request to baseUrl, it messes up the request config.

change to ^axios$ fixed that issue. This is also recommendation inside nock readme for axios.

I also tested manually using symlink in testing repo's node_modules and retries work as expected.

}
},
"prettier": {
Expand Down
1 change: 1 addition & 0 deletions packages/node/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,4 @@ export * from './lib/feeds/feeds.interface';
export * from './lib/topics/topic.interface';
export * from './lib/integrations/integrations.interface';
export * from './lib/messages/messages.interface';
export { defaultRetryCondition } from './lib/retry';
11 changes: 10 additions & 1 deletion packages/node/src/lib/novu.interface.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
import { AxiosInstance } from 'axios';
import { AxiosError, AxiosInstance } from 'axios';

export interface IRetryConfig {
initialDelay?: number;
waitMin?: number;
waitMax?: number;
retryMax?: number;
retryCondition?: (err: AxiosError) => boolean;
}

export interface INovuConfiguration {
backendUrl?: string;
retryConfig?: IRetryConfig;
}

export class WithHttp {
Expand Down
10 changes: 8 additions & 2 deletions packages/node/src/lib/novu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { Topics } from './topics/topics';
import { Integrations } from './integrations/integrations';
import { Messages } from './messages/messages';
import { Tenants } from './tenants/tenants';
import { makeRetryable } from './retry';

export class Novu extends EventEmitter {
private readonly apiKey?: string;
Expand All @@ -33,14 +34,19 @@ export class Novu extends EventEmitter {
constructor(apiKey: string, config?: INovuConfiguration) {
super();
this.apiKey = apiKey;

this.http = axios.create({
const axiosInstance = axios.create({
baseURL: this.buildBackendUrl(config),
headers: {
Authorization: `ApiKey ${this.apiKey}`,
},
});

if (config?.retryConfig) {
makeRetryable(axiosInstance, config);
}

this.http = axiosInstance;

this.subscribers = new Subscribers(this.http);
this.environments = new Environments(this.http);
this.events = new Events(this.http);
Expand Down
275 changes: 275 additions & 0 deletions packages/node/src/lib/retry.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,275 @@
import { AxiosError } from 'axios';
import nock from 'nock';
import { Novu, defaultRetryCondition } from '../index';
import { RETRYABLE_HTTP_CODES } from './retry';

const BACKEND_URL = 'http://example.com';
const TOPICS_PATH = '/v1/topics';
const TRIGGER_PATH = '/v1/events/trigger';

jest.setTimeout(15000);

const hasAllEqual = (arr: Array<string>) => arr.every((val) => val === arr[0]);
const hasUniqueOnly = (arr: Array<string>) =>
Array.from(new Set(arr)).length === arr.length;

class NetworkError extends Error {
constructor(public code: string) {
super('Network Error');
}
}

class HttpError extends Error {
readonly response: { status: number };

constructor(status: number) {
super('Http Error');
this.response = { status };
}
}

describe('Novu Node.js package - Retries and idempotency-key', () => {
afterEach(() => {
nock.cleanAll();
nock.enableNetConnect();
});

const novu = new Novu('fake-key', {
backendUrl: BACKEND_URL,
retryConfig: {
retryMax: 3,
waitMax: 0.5,
waitMin: 0.2,
},
});

it('should retry trigger and generate idempotency-key only once for request', async () => {
const idempotencyKeys: string[] = [];

nock(BACKEND_URL)
.post(TRIGGER_PATH)
.times(3)
.reply(function (_url, _body) {
idempotencyKeys.push(this.req.getHeader('idempotency-key') as string);

return [500, { message: 'Server Exception' }];
});

nock(BACKEND_URL)
.post(TRIGGER_PATH)
.reply(201, { acknowledged: true, transactionId: '1003' });

const result = await novu.trigger('fake-workflow', {
to: { subscriberId: '123' },
payload: {},
});

// all idempotency keys are supposed to be same.
expect(hasAllEqual(idempotencyKeys)).toEqual(true);
expect(result.status).toEqual(201);
expect(result.request.headers['idempotency-key']).toBeDefined();
});

it('should generate different idempotency-key for each request', async () => {
nock(BACKEND_URL)
.post(TRIGGER_PATH)
.reply(500, { message: 'Server Exception' });

nock(BACKEND_URL)
.post(TRIGGER_PATH)
.times(10)
.reply(201, { acknowledged: true, transactionId: '1003' });

const idempotencyKeys: string[] = [];

for (let i = 0; i < 10; i++) {
const result = await novu.trigger('fake-workflow', {
to: { subscriberId: '123' },
payload: {},
});

idempotencyKeys.push(result.request?.headers['idempotency-key']);
}

expect(hasUniqueOnly(idempotencyKeys)).toEqual(true);
});

it('should retry on status 422 and regenerate idempotency-key for every retry', async () => {
const idempotencyKeys: string[] = [];

nock(BACKEND_URL)
.post(TRIGGER_PATH)
.times(3)
.reply(function () {
idempotencyKeys.push(this.req.getHeader('idempotency-key') as string);

return [422, { message: 'Unprocessable Content' }];
});

nock(BACKEND_URL)
.post(TRIGGER_PATH)
.reply(201, { acknowledged: true, transactionId: '1003' });

const result = await novu.trigger('fake-workflow', {
to: { subscriberId: '123' },
payload: {},
});

// idempotency key should be regenerated for every retry for http status 422.
expect(hasUniqueOnly(idempotencyKeys)).toEqual(true);
expect(result.status).toEqual(201);
expect(result.request.headers['idempotency-key']).toBeDefined();
});

it('should retry getting topics list', async () => {
nock(BACKEND_URL)
.get(TOPICS_PATH)
.times(3)
.reply(500, { message: 'Server Exception' });

nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]);

const result = await novu.topics.list({});

expect(result.status).toEqual(200);
expect(result.request.headers['idempotency-key']).toBeUndefined();
});

it('should fail after reaching max retries', async () => {
nock(BACKEND_URL)
.get(TOPICS_PATH)
.times(4)
.reply(500, { message: 'Server Exception' });

nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]);

await expect(novu.topics.list({})).rejects.toMatchObject({
response: { status: 500 },
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great job 👏


const NON_RECOVERABLE_ERRORS: Array<[number, string]> = [
[400, 'Bad Request'],
[401, 'Unauthorized'],
[403, 'Forbidden'],
[404, 'Not Found'],
[405, 'Method not allowed'],
[413, 'Payload Too Large'],
[414, 'URI Too Long'],
[415, 'Unsupported Media Type'],
];

test.each<[number, string]>(NON_RECOVERABLE_ERRORS)(
'should not retry on non-recoverable %i error',
async (status, message) => {
nock(BACKEND_URL).get(TOPICS_PATH).times(3).reply(status, { message });
nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]);

await expect(novu.topics.list({})).rejects.toMatchObject({
response: { status },
});
}
);

rifont marked this conversation as resolved.
Show resolved Hide resolved
it('should retry on various errors until it reach successful response', async () => {
nock(BACKEND_URL)
.get(TOPICS_PATH)
.reply(429, { message: 'Too many requests' });

nock(BACKEND_URL)
.get(TOPICS_PATH)
.reply(408, { message: 'Request Timeout' });

nock(BACKEND_URL)
.get(TOPICS_PATH)
.replyWithError(new NetworkError('ECONNRESET'));

nock(BACKEND_URL)
.get(TOPICS_PATH)
.replyWithError(new NetworkError('ETIMEDOUT'));

nock(BACKEND_URL)
.get(TOPICS_PATH)
.replyWithError(new NetworkError('ECONNREFUSED'));

nock(BACKEND_URL)
.get(TOPICS_PATH)
.reply(504, { message: 'Gateway timeout' });

nock(BACKEND_URL)
.get(TOPICS_PATH)
.reply(422, { message: 'Unprocessable Content' });

nock(BACKEND_URL).get(TOPICS_PATH).reply(200, [{}, {}]);

const novuClient = new Novu('fake-key', {
backendUrl: BACKEND_URL,
retryConfig: {
initialDelay: 0,
waitMin: 0.2,
waitMax: 0.5,
retryMax: 7,
},
});

const result = await novuClient.topics.list({});
expect(result.status).toEqual(200);
});

describe('defaultRetryCondition function', () => {
test.each<[number, string]>(NON_RECOVERABLE_ERRORS)(
'should return false when HTTP status is %i',
(status) => {
const err = new HttpError(status);
expect(defaultRetryCondition(err as AxiosError)).toEqual(false);
}
);

test.each<number>(RETRYABLE_HTTP_CODES)(
'should return true when HTTP status is %i',
(status) => {
const err = new HttpError(status);
expect(defaultRetryCondition(err as AxiosError)).toEqual(true);
}
);

it('should return true when HTTP status is 500', () => {
const err = new HttpError(500);
expect(defaultRetryCondition(err as AxiosError)).toEqual(true);
});

it('should return true when network code is ECONNRESET', () => {
const err = new NetworkError('ECONNRESET');
expect(defaultRetryCondition(err as AxiosError)).toEqual(true);
});

it('shoud return false on unknown error', () => {
const err = new Error('Unexpected error');
expect(defaultRetryCondition(err as AxiosError)).toEqual(false);
});
});

describe('hasAllEqual helper function', () => {
it('should return true when all items are equal', () => {
const arr = ['a', 'a', 'a', 'a'];
expect(hasAllEqual(arr)).toEqual(true);
});

it('should return false when items are not equal', () => {
const arr = ['a', 'b', 'b', 'b'];
expect(hasAllEqual(arr)).toEqual(false);
});
});

describe('hasUniqueOnly helper function', () => {
it('should return true when all items are unique', () => {
const arr = ['a', 'b', 'c', 'd'];
expect(hasUniqueOnly(arr)).toEqual(true);
});

it('should return false when items are not unique', () => {
const arr = ['a', 'a', 'c', 'd'];
expect(hasUniqueOnly(arr)).toEqual(false);
});
});
});
Comment on lines +219 to +275
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent tests 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

hey @michaldziuba03 , just a few spellcheck issues to resolve then we are good to merge 🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rifont done

Loading
Loading