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 9 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.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
205 changes: 205 additions & 0 deletions packages/node/src/lib/retry.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
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<string>) => arr.every((val) => val === arr[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Consider the case where every array item except the first is the same, like [1,2,2,2,2]. This function will currently return true, because it's only comparing entries against the first item, not all of the entries.

So we'll need to instead verify that the number of unique entries in the array is equal to the number of total entries. new Set(Array<any>) constructs a Set comprising unique only values. We can then transform the Set to an Array for a length comparison. See https://stackoverflow.com/a/14438954 for reference.

We should also use the convention of is/has (is for singular test against a primitive, has for plural test, like an array) prefix for a boolean function, to provide a better description for the result.

Lastly, it's generally a good idea to also add tests for helper functions. If these helper functions aren't working as expected, how can we be sure that the feature under test is too?

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 disagree with second sentence but I understand where problem is. Function allEqual (currently renamed to hasAllEqual) works well when our goal is to check if all items are equal (it iterates over all elements and compares them to the first element, it also compares first element with itself). This is the goal when we check in tests if idempotency-keys are equal between retries (in the scope of same request).

I agree this function is used in wrong way when we check if idempotency-keys are unique between requests - we expect allEqual to return false. Negation of this function does not guarantee that all items are unique, so this expectation will pass in case of [1,2,2,2,2] scenario, but shouldn't.

I decided to let them coexist :)


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 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(allEqual(idempotencyKeys)).toBeTruthy();
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(allEqual(idempotencyKeys)).toEqual(false);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new test case - I want to make sure that idempotency key is different between requests

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch 🎉


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 (_url, _body) {
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(allEqual(idempotencyKeys)).toBeFalsy();
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: 1,
waitMax: 1,
retryMax: 7,
},
});

const result = await novuClient.topics.list({});
expect(result.status).toEqual(200);
});
});
94 changes: 94 additions & 0 deletions packages/node/src/lib/retry.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
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'];
const IDEMPOTENCY_KEY = 'Idempotency-Key'; // header key

const DEFAULT_RETRY_MAX = 0;
const DEFAULT_WAIT_MIN = 1;
const DEFAULT_WAIT_MAX = 30;

Copy link
Contributor Author

@michaldziuba03 michaldziuba03 Nov 3, 2023

Choose a reason for hiding this comment

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

new global variables - for default values as suggested in review + I decided to move string value for header key to IDEMPOTENCY_KEY variable (to avoid "magic strings" as it's used in multiple places)

export function makeRetryable(
axios: AxiosInstance,
config?: INovuConfiguration
) {
axios.interceptors.request.use((axiosConfig) => {
if (
axiosConfig.method &&
NON_IDEMPOTENT_METHODS.includes(axiosConfig.method)
) {
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;
});

const retryConfig = config?.retryConfig || {};
const retries = retryConfig.retryMax || DEFAULT_RETRY_MAX;
const minDelay = retryConfig.waitMin || DEFAULT_WAIT_MIN;
const maxDelay = retryConfig.waitMax || DEFAULT_WAIT_MAX;
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();
}
},
});
}

const RETRYABLE_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;
}

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

if (err.response && RETRYABLE_HTTP_CODES.includes(err.response.status)) {
return true;
}

return false;
}
rifont marked this conversation as resolved.
Show resolved Hide resolved
Loading