Skip to content

feat(l10n): report whole-bundle l10n load failures to Sentry #21044

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.

Sign up for GitHub

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

Open
LZoog wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(l10n): report whole-bundle l10n load failures to Sentry #21044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
128 changes: 120 additions & 8 deletions packages/fxa-react/lib/AppLocalizationProvider.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ import { Localized } from '@fluent/react';
import fetchMock from 'fetch-mock';
import AppLocalizationProvider from './AppLocalizationProvider';

// `it` negotiates to exactly ['it', 'en'], which keeps the set of requested
// bundle paths small enough to assert on precisely.
const HASHED_BASE_DIR = '/hashed';
const HASHED_LOCALES = ['it'];

describe('<AppLocalizationProvider/>', () => {
const locales = ['en-GB', 'en-US', 'es-ES'];
const bundles = ['greetings', 'farewells'];
Expand All @@ -26,21 +31,46 @@ describe('<AppLocalizationProvider/>', () => {
}

beforeAll(() => {
// Keys must match the path `fetchMessages` builds, which has no leading
// slash. `farewells` is absent for every locale but en-US, so those
// lookups miss and Fluent falls back.
fetchMock.get(
'/static-asset-manifest.json',
`
{
"/locales/en-US/greetings.ftl": "/locales/en-US/greetings.ftl",
"/locales/en-US/farewells.ftl": "/locales/en-US/farewells.ftl",
"/locales/es-ES/greetings.ftl": "/locales/es-ES/greetings.ftl",
"/locales/en-GB/greetings.ftl": "/locales/en-GB/greetings.ftl",
}
`
JSON.stringify({
'locales/en-US/greetings.ftl': 'locales/en-US/greetings.ftl',
'locales/en-US/farewells.ftl': 'locales/en-US/farewells.ftl',
'locales/es-ES/greetings.ftl': 'locales/es-ES/greetings.ftl',
'locales/en-GB/greetings.ftl': 'locales/en-GB/greetings.ftl',
})
);
fetchMock.get('/locales/en-US/greetings.ftl', 'hello = Hello\n');
fetchMock.get('/locales/en-US/farewells.ftl', 'goodbye = Goodbye\n');
fetchMock.get('/locales/es-ES/greetings.ftl', 'hello = Hola\n');
fetchMock.get('/locales/en-GB/greetings.ftl', 'hello = Hello { $amount }');

// A well-formed manifest, served under its own baseDir so it does not
// collide with the invalid-manifest fixture above. `farewells` is
// deliberately absent from it, and `notfound` maps to a path that 404s.
Comment on lines +51 to +53
fetchMock.get(
`${HASHED_BASE_DIR}/static-asset-manifest.json`,
JSON.stringify({
'locales/it/greetings.ftl': 'locales/it/greetings.1a2b3c.ftl',
'locales/en/greetings.ftl': 'locales/en/greetings.4d5e6f.ftl',
'locales/it/notfound.ftl': 'locales/it/notfound.7a8b9c.ftl',
'locales/en/notfound.ftl': 'locales/en/notfound.7a8b9c.ftl',
})
);
fetchMock.get(
`${HASHED_BASE_DIR}/locales/it/greetings.1a2b3c.ftl`,
'hello = Ciao\n'
);
fetchMock.get(
`${HASHED_BASE_DIR}/locales/en/greetings.4d5e6f.ftl`,
'hello = Hello\n'
);
fetchMock.get(`${HASHED_BASE_DIR}/locales/it/notfound.7a8b9c.ftl`, 404);
fetchMock.get(`${HASHED_BASE_DIR}/locales/en/notfound.7a8b9c.ftl`, 404);

fetchMock.get('*', { throws: new Error() });
});

Expand Down Expand Up @@ -169,4 +199,86 @@ describe('<AppLocalizationProvider/>', () => {

expect(getByTestId('result')).toHaveTextContent('Hello ⁨$US123.00⁩');
});

describe('reportBundleError', () => {
function renderWithManifest(
bundlesToLoad: Array<string>,
reportBundleError: jest.Mock,
baseDir = HASHED_BASE_DIR
) {
return render(
<AppLocalizationProvider
baseDir={baseDir}
bundles={bundlesToLoad}
userLocales={HASHED_LOCALES}
reportBundleError={reportBundleError}
>
<main data-testid="result">
<Localized id="hello">
<div>untranslated</div>
</Localized>
</main>
</AppLocalizationProvider>
);
}

it('resolves the hashed path from the manifest and reports nothing', async () => {
const reportBundleError = jest.fn();
const { getByTestId } = renderWithManifest(
['greetings'],
reportBundleError
);
await waitUntilTranslated();

expect(getByTestId('result')).toHaveTextContent('Ciao');
expect(reportBundleError).not.toHaveBeenCalled();
});

it('reports a bundle with no entry in the manifest, once per locale', async () => {
const reportBundleError = jest.fn();
const { getByTestId } = renderWithManifest(
['farewells'],
reportBundleError
);
await waitUntilTranslated();

expect(
reportBundleError.mock.calls.map(([error]) => error.message)
).toEqual([
'No static asset mapping for l10n bundle: locales/it/farewells.ftl',
'No static asset mapping for l10n bundle: locales/en/farewells.ftl',
]);
expect(getByTestId('result')).toHaveTextContent('untranslated');
});

it('reports a bundle whose hashed path does not resolve, once per locale', async () => {
const reportBundleError = jest.fn();
renderWithManifest(['notfound'], reportBundleError);
await waitUntilTranslated();

expect(
reportBundleError.mock.calls.map(([error]) => error.message)
).toEqual([
`Fetching l10n bundle returned 404: ${HASHED_BASE_DIR}/locales/it/notfound.7a8b9c.ftl`,
`Fetching l10n bundle returned 404: ${HASHED_BASE_DIR}/locales/en/notfound.7a8b9c.ftl`,
]);
});

it('reports an unreachable manifest', async () => {
const reportBundleError = jest.fn();
renderWithManifest(['greetings'], reportBundleError, '/no-manifest');
await waitUntilTranslated();

expect(reportBundleError).toHaveBeenCalledWith(
expect.objectContaining({
message: expect.stringContaining(
'Fetching l10n static asset manifest failed: /no-manifest/static-asset-manifest.json'
),
})
);
// Without mappings the unhashed paths are requested and fail too, so a
// manifest outage costs one report plus one per negotiated locale.
expect(reportBundleError).toHaveBeenCalledTimes(3);
});
});
});
108 changes: 80 additions & 28 deletions packages/fxa-react/lib/AppLocalizationProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,45 +7,68 @@ import { LocalizationProvider, ReactLocalization } from '@fluent/react';
import React, { Component } from 'react';
import { EN_GB_LOCALES, parseAcceptLanguage } from '@fxa/shared/l10n';

type ReportError = (error: Error) => void;

function describeCause(err: unknown) {
return err instanceof Error ? err.message : String(err);
}

/**
* Gets l10n messages from server
* @param baseDir The root location where locales folders are held
* @param locale The target language
* @param bundle The target bundle (ie main)
* @param mappings A set of mappings for static resources.
* @param reportBundleError Receives whole-bundle load failures.
* @returns
*/
async function fetchMessages(
baseDir: string,
locale: string,
bundle: string,
mappings?: Record<string, string>
mappings?: Record<string, string>,
reportBundleError?: ReportError
) {
// Build the path to l10n file
const path = `locales/${locale}/${bundle}.ftl`;

// If mappings were provided see if there is one for the path. This
// will be a location where the file path contains a hash in the file
// name
const mappedPath = mappings ? mappings[path] : path;

// If we don't have mapped path, there are no l10n resources for this language.
if (!mappedPath) {
reportBundleError?.(
new Error(`No static asset mapping for l10n bundle: ${path}`)
);
return '';
}

// Fetch the file and return the messages
const resolvedPath = `${baseDir}/${mappedPath}`;
try {
// Build the path to l10n file
let path = `locales/${locale}/${bundle}.ftl`;

// If mappings were proivided see if there is one for the path. This
// will be a location where the file path contains a hash in the file
// name
if (mappings) {
path = mappings[path];
}
const response = await fetch(resolvedPath);

// If we don't have mapped path, there are no l10n resources for this language.
if (!path) {
// A non-OK body is not FTL, and handing it to Fluent yields an empty bundle.
if (!response.ok) {
reportBundleError?.(
new Error(
`Fetching l10n bundle returned ${response.status}: ${resolvedPath}`
)
);
return '';
}

// Fetch the file and return the messages
const resolvedPath = `${baseDir}/${path}`;
const response = await fetch(resolvedPath);
const messages = await response.text();

return messages;
return await response.text();
} catch (e) {
// We couldn't fetch any strings; just return nothing and fluent will fall
// back to the default locale if needed.
reportBundleError?.(
new Error(
`Fetching l10n bundle failed: ${resolvedPath} (${describeCause(e)})`
)
);
return '';
}
}
Expand All @@ -54,39 +77,62 @@ function fetchAllMessages(
baseDir: string,
locale: string,
bundles: Array<string>,
mappings?: Record<string, string>
mappings?: Record<string, string>,
reportBundleError?: ReportError
) {
return Promise.all(
bundles.map((bndl) => fetchMessages(baseDir, locale, bndl, mappings))
bundles.map((bndl) =>
fetchMessages(baseDir, locale, bndl, mappings, reportBundleError)
)
);
}

async function fetchL10nHashedMappings(mappingUrl: string) {
async function fetchL10nHashedMappings(
mappingUrl: string,
reportBundleError?: ReportError
) {
try {
// These mappigns are currently generated with grunt. See grunt task hash-static
// in fxa-settings for an example of how the mappings are generated.
const mappingsResponse = await fetch(mappingUrl);
const json = await mappingsResponse.json();
return json;
if (!mappingsResponse.ok) {
throw new Error(`Received status ${mappingsResponse.status}`);
}
return await mappingsResponse.json();
} catch (err) {
reportBundleError?.(
new Error(
`Fetching l10n static asset manifest failed: ${mappingUrl} (${describeCause(
err
)})`
)
);
return undefined;
}
}

async function createFluentBundleGenerator(
baseDir: string,
currentLocales: Array<string>,
bundles: Array<string>
bundles: Array<string>,
reportBundleError?: ReportError
) {
const mappings = await fetchL10nHashedMappings(
`${baseDir}/static-asset-manifest.json`
`${baseDir}/static-asset-manifest.json`,
reportBundleError
);
const fetched = await Promise.all(
currentLocales
.filter((l) => !EN_GB_LOCALES.includes(l))
.map(async (locale) => {
return {
[locale]: await fetchAllMessages(baseDir, locale, bundles, mappings),
[locale]: await fetchAllMessages(
baseDir,
locale,
bundles,
mappings,
reportBundleError
),
};
})
);
Expand Down Expand Up @@ -127,7 +173,11 @@ type Props = {
children: any;
// pass messages directly in, used in testing
messages?: { [key: string]: string[] };
reportError?: (error: Error) => void;
// Per-string Fluent errors, e.g. an id missing from the bundle. Defaults to
// @fluent/react's console reporter.
reportError?: ReportError;
// Failures to load a bundle at all, where no string in it can resolve.
reportBundleError?: ReportError;
};

export default class AppLocalizationProvider extends Component<Props, State> {
Expand All @@ -137,6 +187,7 @@ export default class AppLocalizationProvider extends Component<Props, State> {
bundles: ['main'],
children: React.createElement('div'),
reportError: undefined,
reportBundleError: undefined,
};

constructor(props: Props) {
Expand Down Expand Up @@ -170,7 +221,8 @@ export default class AppLocalizationProvider extends Component<Props, State> {
const bundleGenerator = await createFluentBundleGenerator(
baseDir,
currentLocales,
bundles
bundles,
this.props.reportBundleError
);
this.setState({
l10n: new ReactLocalization(
Expand Down
4 changes: 4 additions & 0 deletions packages/fxa-settings/src/components/App/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
import { AccountStateProvider } from '../../models/contexts/AccountStateContext';

import sentryMetrics from 'fxa-shared/sentry/browser';
import { flushL10nErrorReports } from '../../lib/l10n-error-reporter';
// Components
import LoadingSpinner from 'fxa-react/components/LoadingSpinner';
import { ScrollToTop } from '../Settings/ScrollToTop';
Expand Down Expand Up @@ -377,6 +378,9 @@ export const App = ({ flowQueryParams }: { flowQueryParams: QueryParams }) => {
useEffect(() => {
if (metricsEnabled || isSignedIn === false) {
sentryMetrics.enable();
// l10n bundles are fetched before this component can mount, so any
// failure there is buffered until Sentry will accept it.
flushL10nErrorReports();
} else {
sentryMetrics.disable();
}
Expand Down
Loading
Loading