Skip to content

fix: improve retry logging #82

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

Merged
merged 5 commits into from
Jun 14, 2024
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
46 changes: 0 additions & 46 deletions src/hooks/custom/LogRetryHook.ts

This file was deleted.

82 changes: 82 additions & 0 deletions src/hooks/custom/LoggerHook.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { AfterErrorContext, AfterErrorHook, AfterSuccessContext, AfterSuccessHook } from "../types";

/**
* Represents a hook that logs status and information that the request will be retried
* after encountering a 5xx error.
*/
export class LoggerHook implements AfterSuccessHook, AfterErrorHook {
private retriesCounter: Map<string, number> = new Map();

/**
* Log retries to give users visibility into requests.
* @param response - The response object received from the server.
* @param error - The error object representing the encountered error.
* @param operationID - The unique identifier for the operation being logged.
*/
private logRetries(
response: Response | null,
error: unknown,
operationID: string
): void {
if (response && response.status >= 500) {
console.warn(
"Failed to process a request due to API server error with status code %d. " +
"Attempting retry number %d after sleep.",
response.status,
this.retriesCounter.get(operationID)
);
if (response.statusText) {
console.warn("Server message - %s", response.statusText);
}
} else if (error) {
console.info(
`Failed to process a request due to connection error - ${(error as Error).message}. ` +
`Attempting retry number ${this.retriesCounter.get(operationID)} after sleep.`
);
}
}

/**
* Handles successful responses, resetting the retry counter for the operation.
* Logs a success message indicating that the document was successfully partitioned.
* @param hookCtx - The context object containing information about the request.
* @param response - The response object received from the server.
* @returns The response object.
*/
afterSuccess(hookCtx: AfterSuccessContext, response: Response): Response {
this.retriesCounter.delete(hookCtx.operationID);
// NOTE: In case of split page partition this means - at least one of the splits was partitioned successfully
console.info("Successfully partitioned the document.");
return response;
}

/**
* Executes after an error occurs during a request.
* @param hookCtx - The context object containing information about the request.
* @param response - The response object received from the server.
* @param error - The error object representing the encountered error.
* @returns An object containing the updated response and error.
*/
afterError(
hookCtx: AfterErrorContext,
response: Response | null,
error: unknown
): { response: Response | null; error: unknown } {
const currentCount = this.retriesCounter.get(hookCtx.operationID) || 0;
this.retriesCounter.set(hookCtx.operationID, currentCount + 1);
this.logRetries(response, error, hookCtx.operationID);

if (response && response.status === 200) {
console.info("Successfully partitioned the document.");
} else {
console.error("Failed to partition the document.");
if (response) {
console.error(`Server responded with ${response.status} - ${response.statusText}`);
}
if (error) {
console.error(`Following error occurred - ${(error as Error).message}`);
}
}
return { response, error };
}
}
4 changes: 0 additions & 4 deletions src/hooks/custom/SplitPdfHook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,6 @@ export class SplitPdfHook

this.clearOperation(operationID);

console.info("Successfully processed the request.")

return new Response(body, {
headers: headers,
status: response.status,
Expand All @@ -241,7 +239,6 @@ export class SplitPdfHook
const responses = await this.awaitAllRequests(operationID);

if (!responses?.length) {
console.error("Failed to process the request.");
this.clearOperation(operationID);
return { response, error };
}
Expand All @@ -257,7 +254,6 @@ export class SplitPdfHook
});

this.clearOperation(operationID);
console.info("Successfully processed the request.");

return { response: finalResponse, error: null };
}
Expand Down
10 changes: 7 additions & 3 deletions src/hooks/registration.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Hooks } from "./types";

import { LogRetryHook } from "./custom/LogRetryHook";
import { LoggerHook } from "./custom/LoggerHook";
import { SplitPdfHook } from "./custom/SplitPdfHook";
import { HttpsCheckHook } from "./custom/HttpsCheckHook";

Expand All @@ -16,10 +16,13 @@ export function initHooks(hooks: Hooks) {
// Hooks are registered per SDK instance, and are valid for the lifetime of the SDK instance

// Initialize hooks
const logErrorHook = new LogRetryHook();
const loggerHook = new LoggerHook();
const splitPdfHook = new SplitPdfHook();
const httpsCheckHook = new HttpsCheckHook();

// NOTE: logger_hook should stay registered last as logs the status of
// request and whether it will be retried which can be changed by e.g. split_pdf_hook

// Register SDK init hooks
hooks.registerSDKInitHook(httpsCheckHook);
hooks.registerSDKInitHook(splitPdfHook);
Expand All @@ -29,8 +32,9 @@ export function initHooks(hooks: Hooks) {

// Register after success hooks
hooks.registerAfterSuccessHook(splitPdfHook);
hooks.registerAfterSuccessHook(loggerHook)

// Register after error hooks
hooks.registerAfterErrorHook(splitPdfHook);
hooks.registerAfterErrorHook(logErrorHook);
hooks.registerAfterErrorHook(loggerHook);
}
5 changes: 3 additions & 2 deletions test/integration/SplitPdfHook.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,9 @@ describe("SplitPdfHook integration tests check splitted file is same as not spli
});
} catch (e) {
if (!expectedOk) {
expect(consoleErrorSpy).toHaveBeenCalledTimes(2);
expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to process the request.");
expect(consoleErrorSpy).toHaveBeenCalledTimes(3);
expect(consoleErrorSpy).toHaveBeenCalledWith("Failed to partition the document.");
expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Server responded with"));
expect(consoleWarnSpy).toHaveBeenCalledWith(
"Attempted to interpret file as pdf, but error arose when splitting by pages. Reverting to non-split pdf handling path."
);
Expand Down
Loading