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

Better error message and error handling for data plane gateway #916

Merged
merged 14 commits into from
Jan 8, 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
5 changes: 4 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module.exports = {
project: './tsconfig.json',
tsconfigRootDir: __dirname,
},
plugins: ['unused-imports', 'import'],
plugins: ['formatjs', 'unused-imports', 'import'],
rules: {
// Want to make sure imports and exports are always formatted correctly
'unused-imports/no-unused-imports': 'error',
Expand Down Expand Up @@ -39,6 +39,9 @@ module.exports = {
// Just do not agree with this one
'no-negated-condition': 'off',

'formatjs/enforce-placeholders': 'error',
// 'formatjs/no-literal-string-in-jsx': 'error',
Comment on lines +42 to +43
Copy link
Member Author

Choose a reason for hiding this comment

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

Keeping this off for right now. Will make a stand alone PR to fix all of these while turning it on.


'react/iframe-missing-sandbox': 'error',
'react/jsx-no-leaked-render': 'error',
'no-constant-binary-expression': 'error',
Expand Down
5 changes: 4 additions & 1 deletion .licensee.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@
"react-universal-interface": "0.6.2",

"// MIT and CC 3.0 License": "0.0.0",
"spdx-ranges": "2.1.1"
"spdx-ranges": "2.1.1",

"// Public Domain": "0.0.0",
"jsonify": "0.0.1"
},
"ignore": [{ "prefix": "@estuary" }]
}
563 changes: 512 additions & 51 deletions package-lock.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@
"@vitejs/plugin-react": "^4.1.1",
"@vitest/ui": "^0.34.6",
"eslint-config-kentcdodds": "^20.3.1",
"eslint-plugin-formatjs": "^4.11.3",
"eslint-plugin-react": "^7.32.2",
"eslint-plugin-unused-imports": "^2.0.0",
"husky": "^8.0.3",
Expand Down
23 changes: 20 additions & 3 deletions src/components/shared/Error/Message.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,28 @@
import { Box, Stack, Typography } from '@mui/material';
import { FormattedMessage, useIntl } from 'react-intl';
import { logRocketConsole, retryAfterFailure } from 'services/shared';
import {
logRocketConsole,
logRocketEvent,
retryAfterFailure,
} from 'services/shared';
import { CustomEvents } from 'services/types';
import Instructions from './Instructions';
import { ErrorDetails } from './types';

interface Props {
error?: ErrorDetails;
}

const FALLBACK = 'error.fallBack';

function Message({ error }: Props) {
// We fire an event AND log so we can find these but LR
// does not allow unbounded data to be passed into an event
// so the logging allows us to see what error was passed in
logRocketEvent(CustomEvents.ERROR_DISPLAYED);
logRocketConsole('Error message displayed', error);

const intl = useIntl();

const failedAfterRetry = retryAfterFailure(error?.message ?? error);

// Check if we have retried making the call or if we think the message object
Expand All @@ -25,10 +35,17 @@ function Message({ error }: Props) {
(typeof error === 'object' && error.hasOwnProperty('code')) ||
failedAfterRetry;

// There is a small chance this can happen so adding custom event to track it
// Happened before when journal data hook wasn't catching errors
const id = error.message ?? FALLBACK;
if (id === FALLBACK) {
logRocketEvent(CustomEvents.ERROR_MISSING_MESSAGE);
}
Comment on lines +38 to +43
Copy link
Member Author

Choose a reason for hiding this comment

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

Make this super safe. This spot should no longer get hit - but in case making sure we have an event just for it.

We could figure this out with the event/logging above but I want to be able to search for this and not have to manually review sessions.


// We do not need to translate messages from Supabase as they comeback readable
const message = displayErrorOnly
? error.message
: intl.formatMessage({ id: error.message });
: intl.formatMessage({ id });

if (!displayErrorOnly) {
return (
Expand Down
42 changes: 22 additions & 20 deletions src/hooks/journals/useJournalData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,10 @@ import useGatewayAuthToken from 'hooks/useGatewayAuthToken';
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { useCounter } from 'react-use';
import useSWR from 'swr';

enum ErrorFlags {
// TOKEN_PARSING_ISSUE = 'parsing jwt:', // useful for testing just add it to the onError
TOKEN_NOT_FOUND = 'Unauthenticated',
TOKEN_INVALID = 'Authentication failed',
OPERATION_INVALID = 'Unauthorized',
}
import {
dataPlaneFetcher_list,
shouldRefreshToken,
} from 'utils/dataPlane-utils';

const errorRetryCount = 2;

Expand All @@ -41,18 +38,28 @@ const useJournalsForCollection = (collectionName: string | undefined) => {
}, [gatewayConfig]);

const fetcher = useCallback(
(_url: string) => {
async (_url: string) => {
if (journalClient && collectionName) {
const journalSelector = new JournalSelector().collection(
collectionName
);
return journalClient.list(journalSelector).then((result) => {
const journals = result.unwrap();

return {
journals: journals.length > 0 ? journals : [],
};
});
const dataPlaneListResponse = await dataPlaneFetcher_list(
journalClient,
journalSelector,
'JournalData'
);

if (!Array.isArray(dataPlaneListResponse)) {
return Promise.reject(dataPlaneListResponse);
}

return {
journals:
dataPlaneListResponse.length > 0
? dataPlaneListResponse
: [],
};
Comment on lines +47 to +62
Copy link
Member Author

Choose a reason for hiding this comment

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

Made these use identical variable names to make merging these together later easier.

I was going to just have the array and response in the list call but ran into typescript issues and did not want to keep trying to wrangle those.

} else {
return null;
}
Expand All @@ -76,12 +83,7 @@ const useJournalsForCollection = (collectionName: string | undefined) => {
onError: async (error) => {
incAttempts();

const errorAsString = `${error}`;
if (
session &&
(errorAsString.includes(ErrorFlags.TOKEN_INVALID) ||
errorAsString.includes(ErrorFlags.TOKEN_NOT_FOUND))
) {
if (session && shouldRefreshToken(`${error}`)) {
await refreshAuthToken();
resetAttempts();
}
Expand Down
51 changes: 18 additions & 33 deletions src/hooks/shards/useShardsList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,10 @@ import useGatewayAuthToken from 'hooks/useGatewayAuthToken';
import { useMemo } from 'react';
import { logRocketConsole } from 'services/shared';
import useSWR from 'swr';

enum ErrorFlags {
OPERATION_INVALID = 'Unauthorized',
TOKEN_EXPIRED = 'token is expired',
TOKEN_INVALID = 'Authentication failed',
TOKEN_NOT_FOUND = 'Unauthenticated',
}
import {
dataPlaneFetcher_list,
shouldRefreshToken,
} from 'utils/dataPlane-utils';

// These status do not change often so checking every 30 seconds is probably enough
const INTERVAL = 30000;
Expand Down Expand Up @@ -41,27 +38,20 @@ const useShardsList = (catalogNames: string[]) => {
return { shards: [] };
}

const result = await shardClient.list(taskSelector);
const dataPlaneListResponse = await dataPlaneFetcher_list(
shardClient,
taskSelector,
'ShardsList'
);

// Check for an error
if (result.err()) {
// Unwrap the error, log the error, and reject the response
const error = result.unwrap_err();
logRocketConsole('ShardsList : error : ', error);
return Promise.reject(error.body);
if (!Array.isArray(dataPlaneListResponse)) {
return Promise.reject(dataPlaneListResponse);
}

try {
// No error so should be fine to unwrap
const shards = result.unwrap();
return {
shards: shards.length > 0 ? shards : [],
};
} catch (error: unknown) {
// This is just here to be safe. We'll keep an eye on it and possibly remove
logRocketConsole('ShardsList : unwrapError : ', error);
return Promise.reject(error);
}
return {
shards:
dataPlaneListResponse.length > 0 ? dataPlaneListResponse : [],
};
};

const swrKey = useMemo(
Expand All @@ -82,16 +72,11 @@ const useShardsList = (catalogNames: string[]) => {
onError: async (error: string | Error) => {
logRocketConsole('useShardsList on error', { error });

// Try fetching the error message
const errorMessage =
typeof error === 'object' ? error.message : error;

// Check if we need to refresh the access token before returning the error
if (
errorMessage &&
(errorMessage.includes(ErrorFlags.TOKEN_INVALID) ||
errorMessage.includes(ErrorFlags.TOKEN_NOT_FOUND) ||
errorMessage.includes(ErrorFlags.TOKEN_EXPIRED))
shouldRefreshToken(
typeof error === 'object' ? error.message : error
)
) {
await refreshAccess();
}
Expand Down
2 changes: 2 additions & 0 deletions src/lang/en-US.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ const Error: ResolvedIntlConfig['messages'] = {
'error.hintLabel': `Hint:`,
'error.descriptionLabel': `Description:`,
'error.tryAgain': `Try again and if the issue persists please contact support.`,

'error.fallBack': `no error details to display`,
Copy link
Member

@kiahna-tucker kiahna-tucker Jan 8, 2024

Choose a reason for hiding this comment

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

Are we comfortable with this terse and vague of a fallback message? If so, I would recommend using sentence case here and appending with a period.

Out of curiosity, why does a generic error message -- along the lines of 'error.tryAgain' and 'errorBoundry.chunkNotFetched.error.message1' -- not work in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The point is to keep it super vague so it is not relied on too much as it is simply a failsafe.

};

const ErrorBoundry: ResolvedIntlConfig['messages'] = {
Expand Down
2 changes: 2 additions & 0 deletions src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ export enum CustomEvents {
DIRECTIVE_EXCHANGE_TOKEN = 'Directive:ExchangeToken',
ERROR_BOUNDARY_DISPLAYED = 'Error_Boundary_Displayed',
ERROR_BOUNDARY_PAYMENT_METHODS = 'Error_Boundary_Displayed:PaymentMethods',
ERROR_DISPLAYED = 'Error_Displayed',
ERROR_MISSING_MESSAGE = 'Error_Missing_Message',
FIELD_SELECTION_REFRESH_AUTO = 'Field_Selection_Refresh:Auto',
FIELD_SELECTION_REFRESH_MANUAL = 'Field_Selection_Refresh:Manual',
FULL_PAGE_ERROR_DISPLAYED = 'Full_Page_Error_Displayed',
Expand Down
67 changes: 67 additions & 0 deletions src/utils/dataPlane-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import {
broker,
JournalClient,
JournalSelector,
ShardClient,
ShardSelector,
} from 'data-plane-gateway';
import { Shard } from 'data-plane-gateway/types/shard_client';
import { ResponseError } from 'data-plane-gateway/types/util';
import { logRocketConsole } from 'services/shared';

export enum ErrorFlags {
// DEBUGGING = 'parsing jwt:', // useful for testing just add it to the onError
OPERATION_INVALID = 'Unauthorized',
TOKEN_EXPIRED = 'token is expired',
TOKEN_INVALID = 'Authentication failed',
TOKEN_NOT_FOUND = 'Unauthenticated',
}

export const shouldRefreshToken = (errorMessage?: string | null) => {
return (
errorMessage &&
(errorMessage.includes(ErrorFlags.TOKEN_INVALID) ||
errorMessage.includes(ErrorFlags.TOKEN_NOT_FOUND) ||
errorMessage.includes(ErrorFlags.TOKEN_EXPIRED))
);
};

type AllowedKeys = 'ShardsList' | 'JournalData';
export function dataPlaneFetcher_list(
client: ShardClient,
selector: ShardSelector,
key: 'ShardsList'
): Promise<Shard[] | ResponseError['body']>;
export function dataPlaneFetcher_list(
client: JournalClient,
selector: JournalSelector,
key: 'JournalData'
): Promise<broker.ProtocolJournalSpec[] | ResponseError['body']>;
export async function dataPlaneFetcher_list(
client: ShardClient | JournalClient,
selector: ShardSelector | JournalSelector,
key: AllowedKeys
): Promise<Shard[] | broker.ProtocolJournalSpec[] | ResponseError['body']> {
// This can throw an error! Used within fetchers within SWR that is fine and SWR will handle it
// TODO (typing)
// I hate this but I need to get the bug finished
const result = await client.list(selector as any);

// Check for an error
if (result.err()) {
// Unwrap the error, log the error, and reject the response
const error = result.unwrap_err();
logRocketConsole(`${key} : error : `, error);
return Promise.reject(error.body);
}

try {
// No error so should be fine to unwrap
const unwrappedResponse = result.unwrap();
return await Promise.resolve(unwrappedResponse);
} catch (error: unknown) {
// This is just here to be safe. We'll keep an eye on it and possibly remove
logRocketConsole(`${key} : unwrapError : `, error);
return Promise.reject(error);
}
Comment on lines +45 to +66
Copy link
Member Author

Choose a reason for hiding this comment

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

Just snagged this from the shards hook.

}