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 4 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
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.20.0-alpha.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/retries';
michaldziuba03 marked this conversation as resolved.
Show resolved Hide resolved
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 { makeRetriable } from './retries';
michaldziuba03 marked this conversation as resolved.
Show resolved Hide resolved

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) {
makeRetriable(axiosInstance, config);
michaldziuba03 marked this conversation as resolved.
Show resolved Hide resolved
}

this.http = axiosInstance;

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

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

jest.setTimeout(15000);

const allEqual = (arr: Array<any>) => arr.every((val) => val === arr[0]);

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

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: 1,
waitMin: 1,
},
});

it('should retry trigger', async () => {
// prepare backend api mock endpoints:
const idepotencyKeys: string[] = [];

nock(BACKEND_URL)
.post(TRIGGER_PATH)
.times(3)
.reply(function (_url, _body) {
idepotencyKeys.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: {},
});

expect(allEqual(idepotencyKeys)).toBeTruthy();
expect(result.status).toEqual(201);
expect(result.request.headers['idempotency-key']).toBeDefined();
});

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

nock(BACKEND_URL)
.post(TRIGGER_PATH)
.times(3)
.reply(function (_url, _body) {
idepotencyKeys.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: {},
});

expect(allEqual(idepotencyKeys)).toBeFalsy();
expect(result.status).toEqual(201);
expect(result.request.headers['idempotency-key']).toBeDefined();
});

it('should retry getting topics list', async () => {
rifont marked this conversation as resolved.
Show resolved Hide resolved
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();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is great. Could we add another test using the same format with times(4) and verify that a 500 is received?


it('should retry on various errors until it reach successfull response', async () => {
michaldziuba03 marked this conversation as resolved.
Show resolved Hide resolved
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: 1,
waitMax: 1,
retryMax: 7,
},
});

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

Choose a reason for hiding this comment

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

Nice test 🙌

96 changes: 96 additions & 0 deletions packages/node/src/lib/retries.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import { AxiosError, AxiosInstance } from 'axios';
import axiosRetry, { isNetworkError } from 'axios-retry';
import { v4 as uuid } from 'uuid';
import { INovuConfiguration } from './novu.interface';

const NON_IDEMPOTENT_METHODS = ['post', 'patch'];

export function makeRetriable(
axios: AxiosInstance,
config?: INovuConfiguration
) {
axios.interceptors.request.use((axiosConfig) => {
// don't attach idempotency key for idempotent methods
if (
axiosConfig.method &&
!NON_IDEMPOTENT_METHODS.includes(axiosConfig.method)
michaldziuba03 marked this conversation as resolved.
Show resolved Hide resolved
) {
return axiosConfig;
}

const idempotencyKey = axiosConfig.headers['Idempotency-Key'];
// that means intercepted request is retried, so don't generate new idempotency key
if (idempotencyKey) {
return axiosConfig;
}

axiosConfig.headers['Idempotency-Key'] = uuid();

return axiosConfig;
});
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'm using axios interceptor to attach idempotency key header to each request.

Idempotency key is attached only to POST and PATCH methods because they are not idempotent by definition.


const retryConfig = config?.retryConfig || {};
const retries = retryConfig.retryMax || 0;
const minDelay = retryConfig.waitMin || 1;
const maxDelay = retryConfig.waitMax || 30;
michaldziuba03 marked this conversation as resolved.
Show resolved Hide resolved
const initialDelay = retryConfig.initialDelay || minDelay;
const retryCondition = retryConfig.retryCondition || defaultRetryCondition;

function backoff(retryCount: number) {
if (retryCount === 1) {
return initialDelay;
}

const delay = retryCount * minDelay;
if (delay > maxDelay) {
return maxDelay;
}

return delay;
}

axiosRetry(axios, {
retries,
retryCondition,
retryDelay(retryCount) {
return backoff(retryCount) * 1000; // return delay in milliseconds
},
onRetry(_retryCount, error, requestConfig) {
if (
error.response?.status === 422 &&
requestConfig.headers &&
requestConfig.method &&
NON_IDEMPOTENT_METHODS.includes(requestConfig.method)
) {
requestConfig.headers['Idempotency-Key'] = uuid();
}
},
});
Copy link
Contributor Author

@michaldziuba03 michaldziuba03 Oct 13, 2023

Choose a reason for hiding this comment

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

@Cliftonz regenerating idempotency key for HTTP status 422 Unprocessable Content

}

const RETRIABLE_HTTP_CODES = [408, 429, 422];
michaldziuba03 marked this conversation as resolved.
Show resolved Hide resolved

export function defaultRetryCondition(err: AxiosError): boolean {
// retry on TCP/IP error codes like ECONNRESET
if (isNetworkError(err)) {
return true;
}
Copy link
Contributor Author

@michaldziuba03 michaldziuba03 Oct 12, 2023

Choose a reason for hiding this comment

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

isNetworkError is imported from axios-retry library.

As we know HTTP is based on TCP/IP protocol and sometimes we facing issues on this network level.

TCP/IP error typically contains some code like "ECONNRESET" and isNetworkError uses under the hood library is-retry-allowed, because not every network error should be retried.


if (err.code === 'ECONNABORTED') {
return false;
}

if (!err.response) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear why we're retrying when a response is unavailable. Could you please shed some light on this one?

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 it's mistake (should return false) - if statements after this check are dependent on err.response (possibly undefined). In new changes I made it look cleaner.


if (err.response.status >= 500 && err.response.status <= 599) {
return true;
}

if (RETRIABLE_HTTP_CODES.includes(err.response.status)) {
return true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On HTTP level, I decided to retry on every 5xx error, 408 (Request Timeout) and 429 (Too Many Requests)

Copy link
Contributor

Choose a reason for hiding this comment

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

You should also retry on 422 but regen the idempotency key


return false;
}
Loading
Loading