From d2b4e655abed0be80b58b9677b85b8ea1d72d5e0 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Tue, 20 Feb 2024 13:36:41 +0100 Subject: [PATCH] feat: add logs to track number of http retries --- packages/api/src/utils/client/client.ts | 6 +++--- packages/api/src/utils/client/httpClient.ts | 10 +++++++++- .../src/eth1/provider/jsonRpcHttpClient.ts | 9 ++++----- packages/spec-test-util/package.json | 2 -- packages/spec-test-util/src/downloadTests.ts | 3 +-- packages/utils/src/retry.ts | 16 +++++++++++++--- yarn.lock | 19 ------------------- 7 files changed, 30 insertions(+), 35 deletions(-) diff --git a/packages/api/src/utils/client/client.ts b/packages/api/src/utils/client/client.ts index 1782ceea57fd..430d302a070c 100644 --- a/packages/api/src/utils/client/client.ts +++ b/packages/api/src/utils/client/client.ts @@ -8,10 +8,10 @@ import {HttpStatusCode} from "./httpStatusCode.js"; /* eslint-disable @typescript-eslint/no-explicit-any */ type ExtraOpts = {retryAttempts?: number}; -type ParamatersWithOptionalExtaOpts any> = [...Parameters, ExtraOpts] | Parameters; +type ParametersWithOptionalExtraOpts any> = [...Parameters, ExtraOpts] | Parameters; export type ApiWithExtraOpts> = { - [K in keyof T]: (...args: ParamatersWithOptionalExtaOpts) => ReturnType; + [K in keyof T]: (...args: ParametersWithOptionalExtraOpts) => ReturnType; }; // See /packages/api/src/routes/index.ts for reasoning @@ -71,7 +71,7 @@ export function generateGenericJsonClient< const returnType = returnTypes[routeId as keyof ReturnTypes] as TypeJson | null; return async function request( - ...args: ParamatersWithOptionalExtaOpts + ...args: ParametersWithOptionalExtraOpts ): Promise> { try { // extract the extraOpts if provided diff --git a/packages/api/src/utils/client/httpClient.ts b/packages/api/src/utils/client/httpClient.ts index 57214c1ae04a..ba2ea0c15498 100644 --- a/packages/api/src/utils/client/httpClient.ts +++ b/packages/api/src/utils/client/httpClient.ts @@ -184,11 +184,19 @@ export class HttpClient implements IHttpClient { getBody: (res: Response) => Promise ): Promise<{status: HttpStatusCode; body: T}> { if (opts.retryAttempts !== undefined) { + const routeId = opts.routeId ?? DEFAULT_ROUTE_ID; + return retry( async (_attempt) => { return this.requestWithBodyWithFallbacks(opts, getBody); }, - {retries: opts.retryAttempts, retryDelay: 200} + { + retries: opts.retryAttempts, + retryDelay: 200, + onRetry: (e, attempt) => { + this.logger?.debug("Retrying request", {routeId, attempt, lastError: e.message}); + }, + } ); } else { return this.requestWithBodyWithFallbacks(opts, getBody); diff --git a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts index 17809e047944..9655aba5fc6b 100644 --- a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts +++ b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts @@ -158,11 +158,7 @@ export class JsonRpcHttpClient implements IJsonRpcHttpClient { const routeId = opts?.routeId ?? "unknown"; const res = await retry>( - async (attempt) => { - /** If this is a retry, increment the retry counter for this method */ - if (attempt > 1) { - this.opts?.metrics?.retryCount.inc({routeId}); - } + async (_attempt) => { return this.fetchJson({jsonrpc: "2.0", id: this.id++, ...payload}, opts); }, { @@ -170,6 +166,9 @@ export class JsonRpcHttpClient implements IJsonRpcHttpClient { retryDelay: opts?.retryDelay ?? this.opts?.retryDelay ?? 0, shouldRetry: opts?.shouldRetry, signal: this.opts?.signal, + onRetry: () => { + this.opts?.metrics?.retryCount.inc({routeId}); + }, } ); return parseRpcResponse(res, payload); diff --git a/packages/spec-test-util/package.json b/packages/spec-test-util/package.json index 1b2218fff1e1..e96ee7f550f8 100644 --- a/packages/spec-test-util/package.json +++ b/packages/spec-test-util/package.json @@ -63,7 +63,6 @@ ], "dependencies": { "@lodestar/utils": "^1.15.1", - "async-retry": "^1.3.3", "axios": "^1.3.4", "rimraf": "^4.4.1", "snappyjs": "^0.7.0", @@ -71,7 +70,6 @@ "vitest": "^1.2.1" }, "devDependencies": { - "@types/async-retry": "1.4.3", "@types/tar": "^6.1.4" }, "peerDependencies": { diff --git a/packages/spec-test-util/src/downloadTests.ts b/packages/spec-test-util/src/downloadTests.ts index 9e90d5825bdf..e13478934927 100644 --- a/packages/spec-test-util/src/downloadTests.ts +++ b/packages/spec-test-util/src/downloadTests.ts @@ -6,7 +6,7 @@ import {promisify} from "node:util"; import {rimraf} from "rimraf"; import axios from "axios"; import {x as extractTar} from "tar"; -import retry from "async-retry"; +import {retry} from "@lodestar/utils"; export const defaultSpecTestsRepoUrl = "https://github.com/ethereum/consensus-spec-tests"; @@ -67,7 +67,6 @@ export async function downloadGenericSpecTests( const url = `${specTestsRepoUrl ?? defaultSpecTestsRepoUrl}/releases/download/${specVersion}/${test}.tar.gz`; await retry( - // async (bail) => { async () => { const {data, headers} = await axios({ method: "get", diff --git a/packages/utils/src/retry.ts b/packages/utils/src/retry.ts index 19ebffc16898..21d3b9a805e4 100644 --- a/packages/utils/src/retry.ts +++ b/packages/utils/src/retry.ts @@ -6,11 +6,17 @@ export type RetryOptions = { */ retries?: number; /** - * An optional Function that is invoked after the provided callback throws - * It expects a boolean to know if it should retry or not - * Useful to make retrying conditional on the type of error thrown + * An optional Function that is invoked after the provided callback throws. + * It expects a boolean to know if it should retry or not. + * Useful to make retrying conditional on the type of error thrown. */ shouldRetry?: (lastError: Error) => boolean; + /** + * An optional Function that is invoked right before a retry is performed. + * It's passed the Error that triggered it and a number identifying the attempt. + * Useful to track number of retries and errors in logs or metrics. + */ + onRetry?: (lastError: Error, attempt: number) => unknown; /** * Milliseconds to wait before retrying again */ @@ -27,10 +33,14 @@ export type RetryOptions = { export async function retry(fn: (attempt: number) => A | Promise, opts?: RetryOptions): Promise { const maxRetries = opts?.retries ?? 5; const shouldRetry = opts?.shouldRetry; + const onRetry = opts?.onRetry; let lastError: Error = Error("RetryError"); for (let i = 1; i <= maxRetries; i++) { try { + // If not the first attempt, invoke right before retrying + if (i > 1) onRetry?.(lastError, i); + return await fn(i); } catch (e) { lastError = e as Error; diff --git a/yarn.lock b/yarn.lock index c858ef47ddf0..669492e94737 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2810,13 +2810,6 @@ resolved "https://registry.npmjs.org/@types/abstract-leveldown/-/abstract-leveldown-5.0.1.tgz" integrity sha512-wYxU3kp5zItbxKmeRYCEplS2MW7DzyBnxPGj+GJVHZEUZiK/nn5Ei1sUFgURDh+X051+zsGe28iud3oHjrYWQQ== -"@types/async-retry@1.4.3": - version "1.4.3" - resolved "https://registry.yarnpkg.com/@types/async-retry/-/async-retry-1.4.3.tgz#8b78f6ce88d97e568961732cdd9e5325cdc8c246" - integrity sha512-B3C9QmmNULVPL2uSJQ088eGWTNPIeUk35hca6CV8rRDJ8GXuQJP5CCVWA1ZUCrb9xYP7Js/RkLqnNNwKhe+Zsw== - dependencies: - "@types/retry" "*" - "@types/buffer-xor@^2.0.0": version "2.0.0" resolved "https://registry.npmjs.org/@types/buffer-xor/-/buffer-xor-2.0.0.tgz" @@ -3965,13 +3958,6 @@ async-lock@^1.4.0: resolved "https://registry.yarnpkg.com/async-lock/-/async-lock-1.4.0.tgz#c8b6630eff68fbbdd8a5b6eb763dac3bfbb8bf02" integrity sha512-coglx5yIWuetakm3/1dsX9hxCNox22h7+V80RQOu2XUUMidtArxKoZoOtHUPuR84SycKTXzgGzAUR5hJxujyJQ== -async-retry@^1.3.3: - version "1.3.3" - resolved "https://registry.npmjs.org/async-retry/-/async-retry-1.3.3.tgz" - integrity sha512-wfr/jstw9xNi/0teMHrRW7dsz3Lt5ARhYNZ2ewpadnhaIp5mbALhOAP+EAdsC7t4Z6wqsDVv9+W6gm1Dk9mEyw== - dependencies: - retry "0.13.1" - async@^3.2.3: version "3.2.4" resolved "https://registry.yarnpkg.com/async/-/async-3.2.4.tgz#2d22e00f8cddeb5fde5dd33522b56d1cf569a81c" @@ -10939,11 +10925,6 @@ ret@~0.2.0: resolved "https://registry.npmjs.org/ret/-/ret-0.2.2.tgz" integrity sha512-M0b3YWQs7R3Z917WRQy1HHA7Ba7D8hvZg6UE5mLykJxQVE2ju0IXbGlaHPPlkY+WN7wFP+wUMXmBFA0aV6vYGQ== -retry@0.13.1: - version "0.13.1" - resolved "https://registry.npmjs.org/retry/-/retry-0.13.1.tgz" - integrity sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg== - retry@^0.12.0: version "0.12.0" resolved "https://registry.yarnpkg.com/retry/-/retry-0.12.0.tgz#1b42a6266a21f07421d1b0b54b7dc167b01c013b"