From dae020c93a1e602fed3e0219c4afe742572de060 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Tue, 20 Aug 2024 16:02:25 +0100 Subject: [PATCH 01/24] intial refactor using reportError method from window --- src/core/messenger.ts | 14 +++++++-- src/events/on-slot-render.ts | 36 +++++++++++++++-------- src/events/render-advert.ts | 30 ++++++++++++------- src/init/consented.ts | 12 ++++++-- src/init/consented/dynamic-ad-slots.ts | 18 ++++++++---- src/insert/fill-dynamic-advert-slot.ts | 29 ++++++++++++------ src/lib/cookies.js | 21 +++++++++---- src/lib/dfp/non-refreshable-line-items.ts | 23 +++++++++------ src/types/global.ts | 5 ++++ src/utils/report-error.spec.ts | 23 ++++++++++++--- src/utils/robust.ts | 9 ++++-- 11 files changed, 158 insertions(+), 62 deletions(-) diff --git a/src/core/messenger.ts b/src/core/messenger.ts index 9022abccf..250292383 100644 --- a/src/core/messenger.ts +++ b/src/core/messenger.ts @@ -374,9 +374,17 @@ const onMessage = async (event: MessageEvent): Promise => { respond(message.id, event.source, null, response); }) .catch((ex: Error) => { - reportError(ex, { - feature: 'native-ads', - }); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError( + ex, + 'native-ads', + ); + } else { + console.error('Error reporting is not available:', ex); + } + // reportError(ex, { + // feature: 'native-ads', + // }); respond( message.id, event.source, diff --git a/src/events/on-slot-render.ts b/src/events/on-slot-render.ts index 023799403..d2b9b91a9 100644 --- a/src/events/on-slot-render.ts +++ b/src/events/on-slot-render.ts @@ -1,7 +1,7 @@ import { isString } from '@guardian/libs'; import type { AdSize } from 'core/ad-sizes'; import { createAdSize } from 'core/ad-sizes'; -import { reportError } from 'utils/report-error'; +// import { reportError } from 'utils/report-error'; import { getAdvertById } from '../lib/dfp/get-advert-by-id'; import { emptyAdvert } from './empty-advert'; import { renderAdvert } from './render-advert'; @@ -14,23 +14,35 @@ const reportEmptyResponse = ( // let's report these and diagnose the problem in sentry. // Keep the sample rate low, otherwise we'll get rate-limited (report-error will also sample down) if (Math.random() < 1 / 10_000) { - const adUnitPath = event.slot.getAdUnitPath(); + // const adUnitPath = event.slot.getAdUnitPath(); const adTargetingKeys = event.slot.getTargetingKeys(); const adTargetingKValues = adTargetingKeys.includes('k') ? event.slot.getTargeting('k') : []; const adKeywords = adTargetingKValues.join(', '); - reportError( - new Error('dfp returned an empty ad response'), - { - feature: 'commercial', - adUnit: adUnitPath, - adSlot: adSlotId, - adKeywords, - }, - false, - ); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError( + new Error('dfp returned an empty ad response'), + 'commercial', + ); + } else { + console.error( + 'Error reporting is not available:', + new Error('dfp returned an empty ad response'), + ); + } + + // reportError( + // new Error('dfp returned an empty ad response'), + // { + // feature: 'commercial', + // adUnit: adUnitPath, + // adSlot: adSlotId, + // adKeywords, + // }, + // false, + // ); } }; diff --git a/src/events/render-advert.ts b/src/events/render-advert.ts index 8db7d7ca8..85b59ee58 100644 --- a/src/events/render-advert.ts +++ b/src/events/render-advert.ts @@ -1,7 +1,7 @@ import { adSizes } from 'core/ad-sizes'; import { $$ } from 'utils/$$'; import fastdom from 'utils/fastdom-promise'; -import { reportError } from 'utils/report-error'; +// import { reportError } from 'utils/report-error'; import type { Advert } from '../define/Advert'; import { getAdIframe } from '../lib/dfp/get-ad-iframe'; import { renderAdvertLabel } from './render-advert-label'; @@ -178,7 +178,18 @@ const addContainerClass = (adSlotNode: HTMLElement, isRendered: boolean) => { } }); }; - +//Just a local test to check function from window.guardian.modules.sentry +if (process.env.NODE_ENV !== 'production') { + try { + throw new Error('>>>>Test error for sentry'); + } catch (err) { + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError(err, 'test-feature'); + } else { + console.error('>>>>Error reporting is not available:', err); + } + } +} /** * @param advert - as defined in lib/dfp/Advert * @param slotRenderEndedEvent - GPT slotRenderEndedEvent @@ -189,7 +200,7 @@ const renderAdvert = ( slotRenderEndedEvent: googletag.events.SlotRenderEndedEvent, ): Promise => { addContentClass(advert.node); - + console.log('renderAdvert', window.guardian.modules.sentry); return getAdIframe(advert.node) .then((isRendered) => { const creativeTemplateId = @@ -227,14 +238,11 @@ const renderAdvert = ( .then(() => isRendered); }) .catch((err) => { - reportError( - err, - { - feature: 'commercial', - }, - false, - ); - + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError(err, 'commercial'); + } else { + console.error('Error reporting is not available:', err); + } return Promise.resolve(false); }); }; diff --git a/src/init/consented.ts b/src/init/consented.ts index 438be0b1d..c3b4567d5 100644 --- a/src/init/consented.ts +++ b/src/init/consented.ts @@ -19,7 +19,7 @@ import { init as setAdTestInLabelsCookie } from 'init/shared/set-adtest-in-label import { init as prepareAdVerification } from 'lib/ad-verification/prepare-ad-verification'; import { commercialFeatures } from 'lib/commercial-features'; import { adSlotIdPrefix } from 'lib/dfp/dfp-env-globals'; -import { reportError } from 'utils/report-error'; +// import { reportError } from 'utils/report-error'; import { catchErrorsWithContext } from 'utils/robust'; import { initDfpListeners } from './consented/dfp-listeners'; import { initDynamicAdSlots } from './consented/dynamic-ad-slots'; @@ -150,7 +150,15 @@ const bootCommercial = async (): Promise => { await Promise.all(promises).then(recordCommercialMetrics); } catch (error) { // report async errors in bootCommercial to Sentry with the commercial feature tag - reportError(error, tags, false); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError( + error, + 'consented-commercial', + ); + } else { + console.error('Error reporting is not available:', error); + } + // reportError(error, tags, false); } }; diff --git a/src/init/consented/dynamic-ad-slots.ts b/src/init/consented/dynamic-ad-slots.ts index 6fb1b03bf..266952dcc 100644 --- a/src/init/consented/dynamic-ad-slots.ts +++ b/src/init/consented/dynamic-ad-slots.ts @@ -5,7 +5,7 @@ import { init as initHighMerch } from 'insert/high-merch'; import { init as initMobileCrosswordsAdvert } from 'insert/mobile-crossword-banner'; import { init as initMobileSticky } from 'insert/mobile-sticky'; import { init as initLiveblogAdverts } from 'insert/spacefinder/liveblog-adverts'; -import { reportError } from 'utils/report-error'; +// import { reportError } from 'utils/report-error'; import { initArticleBodyAdverts } from './article-body-adverts'; type Modules = Array<[`${string}-${string}`, () => Promise]>; @@ -27,10 +27,18 @@ export const initDynamicAdSlots = async (): Promise => { try { await init(); } catch (error) { - reportError(error, { - feature: 'commercial', - tag: name, - }); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError( + error, + 'commercial', + ); + } else { + console.error('Error reporting is not available:', error); + } + // reportError(error, { + // feature: 'commercial', + // tag: name, + // }); } }), ); diff --git a/src/insert/fill-dynamic-advert-slot.ts b/src/insert/fill-dynamic-advert-slot.ts index e7880dc74..c7f2ebb29 100644 --- a/src/insert/fill-dynamic-advert-slot.ts +++ b/src/insert/fill-dynamic-advert-slot.ts @@ -1,6 +1,6 @@ import { log } from '@guardian/libs'; import type { SizeMapping } from 'core/ad-sizes'; -import { reportError } from 'utils/report-error'; +// import { reportError } from 'utils/report-error'; import type { Advert } from '../define/Advert'; import { createAdvert } from '../define/create-advert'; import { enableLazyLoad } from '../display/lazy-load'; @@ -29,14 +29,25 @@ const fillDynamicAdSlot = ( if (dfpEnv.adverts.has(adSlot.id)) { const errorMessage = `Attempting to add slot with exisiting id ${adSlot.id}`; log('commercial', errorMessage); - reportError( - Error(errorMessage), - { - feature: 'commercial', - slotId: adSlot.id, - }, - false, - ); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError( + Error(errorMessage), + 'commercial', + ); + } else { + console.error( + 'Error reporting is not available:', + Error(errorMessage), + ); + } + // reportError( + // Error(errorMessage), + // { + // feature: 'commercial', + // slotId: adSlot.id, + // }, + // false, + // ); return; } diff --git a/src/lib/cookies.js b/src/lib/cookies.js index 820b1ac0f..a23db10a3 100644 --- a/src/lib/cookies.js +++ b/src/lib/cookies.js @@ -42,11 +42,22 @@ const addCookie = (name, value, daysToLive, isCrossSubdomain = false) => { const expires = new Date(); if (!isValidCookieValue(name) || !isValidCookieValue(value)) { - reportError( - new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), - {}, - false, - ); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError( + new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), + 'commercial', + ); + } else { + console.error( + 'Error reporting is not available:', + new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), + ); + } + // reportError( + // new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), + // {}, + // false, + // ); } if (daysToLive) { diff --git a/src/lib/dfp/non-refreshable-line-items.ts b/src/lib/dfp/non-refreshable-line-items.ts index 212891bcd..c6e1dba0b 100644 --- a/src/lib/dfp/non-refreshable-line-items.ts +++ b/src/lib/dfp/non-refreshable-line-items.ts @@ -1,5 +1,5 @@ import { memoize } from 'lodash-es'; -import { reportError } from 'utils/report-error'; +// import { reportError } from 'utils/report-error'; export const fetchNonRefreshableLineItemIds = async (): Promise => { // When the env is CODE or local, use the CODE env's non-refreshable line items file @@ -42,14 +42,19 @@ export const fetchNonRefreshableLineItemIds = async (): Promise => { // Report an error to Sentry if we don't get an ok response // Note that in other cases (JSON parsing failure) we throw but don't report the error const error = Error('Failed to fetch non-refreshable line items'); - reportError( - error, - { - feature: 'commercial', - status: String(response.status), - }, - false, - ); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError(error, 'commercial'); + } else { + console.error('Error reporting is not available:', error); + } + // reportError( + // error, + // { + // feature: 'commercial', + // status: String(response.status), + // }, + // false, + // ); throw error; }; diff --git a/src/types/global.ts b/src/types/global.ts index c510d907c..c1203c726 100644 --- a/src/types/global.ts +++ b/src/types/global.ts @@ -468,6 +468,11 @@ declare global { notificationEventHistory?: HeaderNotification[][]; commercialTimer?: EventTimer; offlineCount?: number; + modules: { + sentry: { + reportError: (error: unknown, feature: string) => void; + }; + }; }; ootag: { queue: Array<() => void>; diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts index 8d00351bb..c126c9568 100644 --- a/src/utils/report-error.spec.ts +++ b/src/utils/report-error.spec.ts @@ -1,5 +1,5 @@ import fakeRaven from 'raven-js'; -import { reportError } from './report-error'; +// import { reportError } from './report-error'; jest.mock('raven-js', () => ({ config() { @@ -27,7 +27,12 @@ describe('report-error', () => { test('Does NOT throw an error', () => { expect(() => { - reportError(error, tags, false); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError(error, 'commercial'); + } else { + console.error('Error reporting is not available:', error); + } + // reportError(error, tags, false); }).not.toThrowError(error); expect(fakeRaven.captureException).toHaveBeenCalledWith( @@ -38,7 +43,12 @@ describe('report-error', () => { test('Does throw an error', () => { expect(() => { - reportError(error, tags); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError(error, 'commercial'); + } else { + console.error('Error reporting is not available:', error); + } + // reportError(error, tags); }).toThrowError(error); expect(fakeRaven.captureException).toHaveBeenCalledWith( @@ -48,7 +58,12 @@ describe('report-error', () => { }); test('Applies a sampling rate that prevents a sample of errors being reporting', () => { - reportError(error, tags, false, 1 / 100); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError(error, 'commercial'); + } else { + console.error('Error reporting is not available:', error); + } + // reportError(error, tags, false, 1 / 100); expect(fakeRaven.captureException).not.toHaveBeenCalled(); }); diff --git a/src/utils/robust.ts b/src/utils/robust.ts index 2df385f9e..a216c405a 100644 --- a/src/utils/robust.ts +++ b/src/utils/robust.ts @@ -3,7 +3,7 @@ For example "comments throwing an exception should not stop auto refresh" */ -import { convertError, reportError } from './report-error'; +import { convertError } from './report-error'; type ModuleFunction = () => void; type Module = [string, ModuleFunction]; @@ -27,7 +27,12 @@ const logError = ( tags?: Record, ): void => { window.console.warn('Caught error.', error.stack); - reportError(error, { module: moduleName, ...tags }, false); + if (window.guardian?.modules?.sentry?.reportError) { + window.guardian.modules.sentry.reportError(error, 'commercial'); + } else { + console.error('Error reporting is not available:', error); + } + // reportError(error, { module: moduleName, ...tags }, false); }; const catchAndLogError = ( From 757986df87aac5bb741b50fcf3fe9ee499c1d592 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Wed, 21 Aug 2024 14:43:29 +0100 Subject: [PATCH 02/24] remove ravens reportError function calls --- src/core/messenger.ts | 16 +--------- src/events/on-slot-render.ts | 38 ++++------------------- src/events/render-advert.ts | 13 ++------ src/init/consented.ts | 13 +++----- src/init/consented/dynamic-ad-slots.ts | 15 ++------- src/insert/fill-dynamic-advert-slot.ts | 23 +++----------- src/lib/cookies.js | 5 --- src/lib/dfp/non-refreshable-line-items.ts | 14 +-------- src/utils/report-error.spec.ts | 21 ++----------- src/utils/robust.ts | 13 ++------ 10 files changed, 25 insertions(+), 146 deletions(-) diff --git a/src/core/messenger.ts b/src/core/messenger.ts index 250292383..c9e96fca2 100644 --- a/src/core/messenger.ts +++ b/src/core/messenger.ts @@ -163,9 +163,6 @@ type RespondCallback = ( const LISTENERS: Listeners = {}; let REGISTERED_LISTENERS = 0; -let reportError: MessengerErrorHandler = () => { - // not set yet -}; const error405 = { code: 405, @@ -374,17 +371,7 @@ const onMessage = async (event: MessageEvent): Promise => { respond(message.id, event.source, null, response); }) .catch((ex: Error) => { - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError( - ex, - 'native-ads', - ); - } else { - console.error('Error reporting is not available:', ex); - } - // reportError(ex, { - // feature: 'native-ads', - // }); + window.guardian.modules.sentry.reportError(ex, 'native-ads'); respond( message.id, event.source, @@ -522,7 +509,6 @@ export const init = ( >, errorHandler: MessengerErrorHandler, ): void => { - reportError = errorHandler; listeners.forEach((moduleInit) => moduleInit(register, errorHandler)); persistentListeners.forEach((moduleInit) => moduleInit(registerPersistentListener, errorHandler), diff --git a/src/events/on-slot-render.ts b/src/events/on-slot-render.ts index d2b9b91a9..3d73678f0 100644 --- a/src/events/on-slot-render.ts +++ b/src/events/on-slot-render.ts @@ -6,43 +6,17 @@ import { getAdvertById } from '../lib/dfp/get-advert-by-id'; import { emptyAdvert } from './empty-advert'; import { renderAdvert } from './render-advert'; -const reportEmptyResponse = ( - adSlotId: string, - event: googletag.events.SlotRenderEndedEvent, -) => { +const reportEmptyResponse = () => { // This empty slot could be caused by a targeting problem, // let's report these and diagnose the problem in sentry. // Keep the sample rate low, otherwise we'll get rate-limited (report-error will also sample down) if (Math.random() < 1 / 10_000) { // const adUnitPath = event.slot.getAdUnitPath(); - const adTargetingKeys = event.slot.getTargetingKeys(); - const adTargetingKValues = adTargetingKeys.includes('k') - ? event.slot.getTargeting('k') - : []; - const adKeywords = adTargetingKValues.join(', '); - - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError( - new Error('dfp returned an empty ad response'), - 'commercial', - ); - } else { - console.error( - 'Error reporting is not available:', - new Error('dfp returned an empty ad response'), - ); - } - // reportError( - // new Error('dfp returned an empty ad response'), - // { - // feature: 'commercial', - // adUnit: adUnitPath, - // adSlot: adSlotId, - // adKeywords, - // }, - // false, - // ); + window.guardian.modules.sentry.reportError( + new Error('dfp returned an empty ad response'), + 'commercial', + ); } }; @@ -63,7 +37,7 @@ export const onSlotRender = ( if (event.isEmpty) { emptyAdvert(advert); - reportEmptyResponse(advert.id, event); + reportEmptyResponse(); advert.finishedRendering(false); } else { /** diff --git a/src/events/render-advert.ts b/src/events/render-advert.ts index 85b59ee58..ebfc3fc19 100644 --- a/src/events/render-advert.ts +++ b/src/events/render-advert.ts @@ -183,11 +183,7 @@ if (process.env.NODE_ENV !== 'production') { try { throw new Error('>>>>Test error for sentry'); } catch (err) { - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError(err, 'test-feature'); - } else { - console.error('>>>>Error reporting is not available:', err); - } + window.guardian.modules.sentry.reportError(err, 'test-feature'); } } /** @@ -200,7 +196,6 @@ const renderAdvert = ( slotRenderEndedEvent: googletag.events.SlotRenderEndedEvent, ): Promise => { addContentClass(advert.node); - console.log('renderAdvert', window.guardian.modules.sentry); return getAdIframe(advert.node) .then((isRendered) => { const creativeTemplateId = @@ -238,11 +233,7 @@ const renderAdvert = ( .then(() => isRendered); }) .catch((err) => { - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError(err, 'commercial'); - } else { - console.error('Error reporting is not available:', err); - } + window.guardian.modules.sentry.reportError(err, 'commercial'); return Promise.resolve(false); }); }; diff --git a/src/init/consented.ts b/src/init/consented.ts index c3b4567d5..09c469ff1 100644 --- a/src/init/consented.ts +++ b/src/init/consented.ts @@ -150,15 +150,10 @@ const bootCommercial = async (): Promise => { await Promise.all(promises).then(recordCommercialMetrics); } catch (error) { // report async errors in bootCommercial to Sentry with the commercial feature tag - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError( - error, - 'consented-commercial', - ); - } else { - console.error('Error reporting is not available:', error); - } - // reportError(error, tags, false); + window.guardian.modules.sentry.reportError( + error, + 'consented-commercial', + ); } }; diff --git a/src/init/consented/dynamic-ad-slots.ts b/src/init/consented/dynamic-ad-slots.ts index 266952dcc..e8b3421bb 100644 --- a/src/init/consented/dynamic-ad-slots.ts +++ b/src/init/consented/dynamic-ad-slots.ts @@ -23,22 +23,11 @@ const dynamicAdSlotModules: Modules = [ export const initDynamicAdSlots = async (): Promise => { await Promise.all( - dynamicAdSlotModules.map(async ([name, init]) => { + dynamicAdSlotModules.map(async ([, init]) => { try { await init(); } catch (error) { - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError( - error, - 'commercial', - ); - } else { - console.error('Error reporting is not available:', error); - } - // reportError(error, { - // feature: 'commercial', - // tag: name, - // }); + window.guardian.modules.sentry.reportError(error, 'commercial'); } }), ); diff --git a/src/insert/fill-dynamic-advert-slot.ts b/src/insert/fill-dynamic-advert-slot.ts index c7f2ebb29..4c0033f1a 100644 --- a/src/insert/fill-dynamic-advert-slot.ts +++ b/src/insert/fill-dynamic-advert-slot.ts @@ -29,25 +29,10 @@ const fillDynamicAdSlot = ( if (dfpEnv.adverts.has(adSlot.id)) { const errorMessage = `Attempting to add slot with exisiting id ${adSlot.id}`; log('commercial', errorMessage); - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError( - Error(errorMessage), - 'commercial', - ); - } else { - console.error( - 'Error reporting is not available:', - Error(errorMessage), - ); - } - // reportError( - // Error(errorMessage), - // { - // feature: 'commercial', - // slotId: adSlot.id, - // }, - // false, - // ); + window.guardian.modules.sentry.reportError( + Error(errorMessage), + 'commercial', + ); return; } diff --git a/src/lib/cookies.js b/src/lib/cookies.js index a23db10a3..c76290f02 100644 --- a/src/lib/cookies.js +++ b/src/lib/cookies.js @@ -53,11 +53,6 @@ const addCookie = (name, value, daysToLive, isCrossSubdomain = false) => { new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), ); } - // reportError( - // new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), - // {}, - // false, - // ); } if (daysToLive) { diff --git a/src/lib/dfp/non-refreshable-line-items.ts b/src/lib/dfp/non-refreshable-line-items.ts index c6e1dba0b..f6c44ab61 100644 --- a/src/lib/dfp/non-refreshable-line-items.ts +++ b/src/lib/dfp/non-refreshable-line-items.ts @@ -42,19 +42,7 @@ export const fetchNonRefreshableLineItemIds = async (): Promise => { // Report an error to Sentry if we don't get an ok response // Note that in other cases (JSON parsing failure) we throw but don't report the error const error = Error('Failed to fetch non-refreshable line items'); - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError(error, 'commercial'); - } else { - console.error('Error reporting is not available:', error); - } - // reportError( - // error, - // { - // feature: 'commercial', - // status: String(response.status), - // }, - // false, - // ); + window.guardian.modules.sentry.reportError(error, 'commercial'); throw error; }; diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts index c126c9568..f7f5a8d0e 100644 --- a/src/utils/report-error.spec.ts +++ b/src/utils/report-error.spec.ts @@ -27,12 +27,7 @@ describe('report-error', () => { test('Does NOT throw an error', () => { expect(() => { - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError(error, 'commercial'); - } else { - console.error('Error reporting is not available:', error); - } - // reportError(error, tags, false); + window.guardian.modules.sentry.reportError(error, 'commercial'); }).not.toThrowError(error); expect(fakeRaven.captureException).toHaveBeenCalledWith( @@ -43,12 +38,7 @@ describe('report-error', () => { test('Does throw an error', () => { expect(() => { - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError(error, 'commercial'); - } else { - console.error('Error reporting is not available:', error); - } - // reportError(error, tags); + window.guardian.modules.sentry.reportError(error, 'commercial'); }).toThrowError(error); expect(fakeRaven.captureException).toHaveBeenCalledWith( @@ -58,12 +48,7 @@ describe('report-error', () => { }); test('Applies a sampling rate that prevents a sample of errors being reporting', () => { - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError(error, 'commercial'); - } else { - console.error('Error reporting is not available:', error); - } - // reportError(error, tags, false, 1 / 100); + window.guardian.modules.sentry.reportError(error, 'commercial'); expect(fakeRaven.captureException).not.toHaveBeenCalled(); }); diff --git a/src/utils/robust.ts b/src/utils/robust.ts index a216c405a..a066de3f0 100644 --- a/src/utils/robust.ts +++ b/src/utils/robust.ts @@ -21,18 +21,9 @@ const catchErrors = (fn: ModuleFunction): Error | undefined => { return error; }; -const logError = ( - moduleName: string, - error: Error, - tags?: Record, -): void => { +const logError = (moduleName: string, error: Error): void => { window.console.warn('Caught error.', error.stack); - if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError(error, 'commercial'); - } else { - console.error('Error reporting is not available:', error); - } - // reportError(error, { module: moduleName, ...tags }, false); + window.guardian.modules.sentry.reportError(error, 'commercial'); }; const catchAndLogError = ( From db35e6ddc56418a1a17422f32cbcb0916f6de23f Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Thu, 22 Aug 2024 14:55:21 +0100 Subject: [PATCH 03/24] remove report-error function working for raven --- src/define/create-advert.ts | 9 ++------- src/init/consented/dfp-listeners.ts | 11 ++--------- src/init/consented/messenger.ts | 1 - src/init/consented/prepare-permutive.ts | 3 +-- src/init/consentless.ts | 1 - src/insert/spacefinder/article.spec.ts | 4 ---- src/insert/spacefinder/space-filler.ts | 3 +-- 7 files changed, 6 insertions(+), 26 deletions(-) diff --git a/src/define/create-advert.ts b/src/define/create-advert.ts index a8f72ffa2..b12a36a1c 100644 --- a/src/define/create-advert.ts +++ b/src/define/create-advert.ts @@ -1,6 +1,5 @@ import { log } from '@guardian/libs'; import type { SizeMapping } from 'core/ad-sizes'; -import { reportError } from 'utils/report-error'; import { Advert } from './Advert'; const createAdvert = ( @@ -21,13 +20,9 @@ const createAdvert = ( log('commercial', errMsg); if (!navigator.userAgent.includes('DuckDuckGo')) { - reportError( + window.guardian.modules.sentry.reportError( new Error(errMsg), - { - feature: 'commercial', - }, - false, - 1 / 100, + 'commercial', ); } diff --git a/src/init/consented/dfp-listeners.ts b/src/init/consented/dfp-listeners.ts index 60d7c8819..e743db7e8 100644 --- a/src/init/consented/dfp-listeners.ts +++ b/src/init/consented/dfp-listeners.ts @@ -1,4 +1,3 @@ -import raven from 'lib/raven'; import { onSlotLoad } from '../../events/on-slot-load'; import { onSlotRender } from '../../events/on-slot-render'; import { onSlotViewableFunction } from '../../events/on-slot-viewable'; @@ -7,14 +6,8 @@ const initDfpListeners = (): Promise => { window.googletag.cmd.push(() => { const pubads = window.googletag.pubads(); - pubads.addEventListener( - 'slotRenderEnded', - raven.wrap(onSlotRender), - ); - pubads.addEventListener( - 'slotOnload', - raven.wrap(onSlotLoad), - ); + pubads.addEventListener('slotRenderEnded', onSlotRender); + pubads.addEventListener('slotOnload', onSlotLoad); pubads.addEventListener('impressionViewable', onSlotViewableFunction()); }); return Promise.resolve(); diff --git a/src/init/consented/messenger.ts b/src/init/consented/messenger.ts index 0e5a17aac..5f8005844 100644 --- a/src/init/consented/messenger.ts +++ b/src/init/consented/messenger.ts @@ -12,7 +12,6 @@ import { init as resize } from 'core/messenger/resize'; import { init as scroll } from 'core/messenger/scroll'; import { init as type } from 'core/messenger/type'; import { init as viewport } from 'core/messenger/viewport'; -import { reportError } from 'utils/report-error'; /** * Messenger gets to skip the promise chain and run immediately. diff --git a/src/init/consented/prepare-permutive.ts b/src/init/consented/prepare-permutive.ts index 16e7c1f69..f9170444a 100644 --- a/src/init/consented/prepare-permutive.ts +++ b/src/init/consented/prepare-permutive.ts @@ -1,6 +1,5 @@ import type { Edition } from 'core/types'; import type { Config, PageConfig, Permutive, UserConfig } from 'types/global'; -import { reportError } from 'utils/report-error'; interface PermutivePageConfig { page: Pick< @@ -157,7 +156,7 @@ const runPermutive = ( page: payload, }); } catch (err) { - logger(err, { feature: 'commercial' }, false); + logger(err); } }; diff --git a/src/init/consentless.ts b/src/init/consentless.ts index a6de91a8c..93087b250 100644 --- a/src/init/consentless.ts +++ b/src/init/consentless.ts @@ -10,7 +10,6 @@ import { initConsentless } from 'init/consentless/prepare-ootag'; import { reloadPageOnConsentChange } from 'init/shared/reload-page-on-consent-change'; import { init as setAdTestCookie } from 'init/shared/set-adtest-cookie'; import { init as setAdTestInLabelsCookie } from 'init/shared/set-adtest-in-labels-cookie'; -import { reportError } from 'utils/report-error'; const bootConsentless = async (consentState: ConsentState): Promise => { const consentlessModuleList = [ diff --git a/src/insert/spacefinder/article.spec.ts b/src/insert/spacefinder/article.spec.ts index 0a4b7d50b..445139186 100644 --- a/src/insert/spacefinder/article.spec.ts +++ b/src/insert/spacefinder/article.spec.ts @@ -2,10 +2,6 @@ import { spaceFiller } from 'insert/spacefinder/space-filler'; import { commercialFeatures } from 'lib/commercial-features'; import { init } from './article'; -jest.mock('utils/report-error', () => ({ - reportError: jest.fn(), -})); - jest.mock('lib/header-bidding/prebid/prebid', () => ({ requestBids: jest.fn(), })); diff --git a/src/insert/spacefinder/space-filler.ts b/src/insert/spacefinder/space-filler.ts index 3e1aee7c7..2ca3cc39a 100644 --- a/src/insert/spacefinder/space-filler.ts +++ b/src/insert/spacefinder/space-filler.ts @@ -4,7 +4,6 @@ import type { SpacefinderWriter, } from 'insert/spacefinder/spacefinder'; import { findSpace, SpaceError } from 'insert/spacefinder/spacefinder'; -import raven from 'lib/raven'; class SpaceFiller { queue = Promise.resolve(true); @@ -35,7 +34,7 @@ class SpaceFiller { this.queue = this.queue.then(insertNextContent).catch((e) => { // e.g. if writer fails - raven.captureException(e); + window.guardian.modules.sentry.reportError(e, 'space-filler'); return false; }); From e7ac51d64b825d0d526de87fd943896911e8eb87 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Thu, 22 Aug 2024 14:58:15 +0100 Subject: [PATCH 04/24] remove report error and refactor tests --- jest.setupTestFrameworkScriptFile.js | 5 ++++ .../spacefinder/liveblog-adverts.spec.ts | 3 --- .../dfp/non-refreshable-line-items.spec.ts | 25 ++++++++----------- src/utils/robust.spec.ts | 21 +++++++--------- src/utils/robust.ts | 19 ++++---------- 5 files changed, 30 insertions(+), 43 deletions(-) diff --git a/jest.setupTestFrameworkScriptFile.js b/jest.setupTestFrameworkScriptFile.js index 3c30caead..0b38375cf 100644 --- a/jest.setupTestFrameworkScriptFile.js +++ b/jest.setupTestFrameworkScriptFile.js @@ -32,6 +32,11 @@ window.guardian = { active: undefined, onDetect: [], }, + modules: { + sentry: { + reportError: jest.fn(), + }, + }, }; window.fetch = jest.fn().mockImplementation(() => diff --git a/src/insert/spacefinder/liveblog-adverts.spec.ts b/src/insert/spacefinder/liveblog-adverts.spec.ts index 032fe19a0..a9e2ee53e 100644 --- a/src/insert/spacefinder/liveblog-adverts.spec.ts +++ b/src/insert/spacefinder/liveblog-adverts.spec.ts @@ -7,9 +7,6 @@ const { getStartingContentBlock, } = _; -jest.mock('utils/report-error', () => ({ - reportError: jest.fn(), -})); jest.mock('lib/header-bidding/prebid/prebid', () => ({ requestBids: jest.fn(), })); diff --git a/src/lib/dfp/non-refreshable-line-items.spec.ts b/src/lib/dfp/non-refreshable-line-items.spec.ts index 3e8c81d7c..91b883fac 100644 --- a/src/lib/dfp/non-refreshable-line-items.spec.ts +++ b/src/lib/dfp/non-refreshable-line-items.spec.ts @@ -1,13 +1,8 @@ -import { reportError } from 'utils/report-error'; import { fetchNonRefreshableLineItemIds, memoizedFetchNonRefreshableLineItemIds, } from './non-refreshable-line-items'; -jest.mock('utils/report-error', () => ({ - reportError: jest.fn(), -})); - describe('nonRefreshableLineItems', () => { it('returns the same IDs as the API', async () => { const response = { @@ -22,7 +17,9 @@ describe('nonRefreshableLineItems', () => { const ids = await fetchNonRefreshableLineItemIds(); - expect(reportError).not.toHaveBeenCalled(); + expect( + window.guardian.modules.sentry.reportError, + ).not.toHaveBeenCalled(); expect(ids).toEqual([1, 2, 3]); }); @@ -42,7 +39,9 @@ describe('nonRefreshableLineItems', () => { 'Failed to parse non-refreshable line items as an array', ); - expect(reportError).not.toHaveBeenCalled(); + expect( + window.guardian.modules.sentry.reportError, + ).not.toHaveBeenCalled(); }); it('returns undefined when the API returns string array', async () => { @@ -60,7 +59,9 @@ describe('nonRefreshableLineItems', () => { 'Failed to parse element in non-refreshable line item array as number', ); - expect(reportError).not.toHaveBeenCalled(); + expect( + window.guardian.modules.sentry.reportError, + ).not.toHaveBeenCalled(); }); it('returns undefined and reports error when the API call fails', async () => { @@ -78,13 +79,9 @@ describe('nonRefreshableLineItems', () => { 'Failed to fetch non-refreshable line items', ); - expect(reportError).toHaveBeenCalledWith( + expect(window.guardian.modules.sentry.reportError).toHaveBeenCalledWith( new Error('Failed to fetch non-refreshable line items'), - { - feature: 'commercial', - status: '404', - }, - false, + 'commercial', ); }); }); diff --git a/src/utils/robust.spec.ts b/src/utils/robust.spec.ts index 79789c703..4d6055631 100644 --- a/src/utils/robust.spec.ts +++ b/src/utils/robust.spec.ts @@ -1,20 +1,15 @@ -import { convertError, reportError } from 'utils/report-error'; import { _, catchErrorsWithContext } from './robust'; import type { Modules } from './robust'; const { catchAndLogError } = _; -jest.mock('utils/report-error', () => ({ - convertError: jest.fn((e: Error) => e), - reportError: jest.fn(), -})); - let origConsoleWarn: typeof window.console.warn; beforeEach(() => { jest.clearAllMocks(); origConsoleWarn = window.console.warn; window.console.warn = jest.fn(); + window.guardian.modules.sentry.reportError = jest.fn(); }); afterEach(() => { @@ -23,7 +18,7 @@ afterEach(() => { describe('robust', () => { const ERROR = new Error('Something broke.'); - const META = { module: 'test' }; + const META = 'commercial'; const noError = () => true; @@ -40,20 +35,22 @@ describe('robust', () => { catchAndLogError('test', throwError); }).not.toThrowError(ERROR); - expect(convertError).toHaveBeenCalledTimes(1); expect(window.console.warn).toHaveBeenCalledTimes(1); }); test('catchAndLogError() - default reporter with no error', () => { catchAndLogError('test', noError); - expect(convertError).toHaveBeenCalledTimes(0); - expect(reportError).not.toHaveBeenCalled(); + expect( + window.guardian.modules.sentry.reportError, + ).not.toHaveBeenCalled(); }); test('catchAndLogError() - default reporter with error', () => { catchAndLogError('test', throwError); - expect(convertError).toHaveBeenCalledTimes(1); - expect(reportError).toHaveBeenCalledWith(ERROR, META, false); + expect(window.guardian.modules.sentry.reportError).toHaveBeenCalledWith( + ERROR, + META, + ); }); test('catchErrorsWithContext()', () => { diff --git a/src/utils/robust.ts b/src/utils/robust.ts index a066de3f0..c819e0f1e 100644 --- a/src/utils/robust.ts +++ b/src/utils/robust.ts @@ -3,8 +3,6 @@ For example "comments throwing an exception should not stop auto refresh" */ -import { convertError } from './report-error'; - type ModuleFunction = () => void; type Module = [string, ModuleFunction]; type Modules = Module[]; @@ -15,7 +13,7 @@ const catchErrors = (fn: ModuleFunction): Error | undefined => { try { fn(); } catch (e) { - error = convertError(e); + error = e instanceof Error ? e : new Error(String(e)); } return error; @@ -26,23 +24,16 @@ const logError = (moduleName: string, error: Error): void => { window.guardian.modules.sentry.reportError(error, 'commercial'); }; -const catchAndLogError = ( - name: string, - fn: ModuleFunction, - tags?: Record, -): void => { +const catchAndLogError = (name: string, fn: ModuleFunction): void => { const error = catchErrors(fn); if (error) { - logError(name, error, tags); + logError(name, error); } }; -const catchErrorsWithContext = ( - modules: Modules, - tags?: Record, -): void => { - modules.forEach(([name, fn]) => catchAndLogError(name, fn, tags)); +const catchErrorsWithContext = (modules: Modules): void => { + modules.forEach(([name, fn]) => catchAndLogError(name, fn)); }; export { catchErrorsWithContext }; From f2d1e0db13797da4d27d0f4ba59280e3ae9ebbcf Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Thu, 22 Aug 2024 15:44:29 +0100 Subject: [PATCH 05/24] remove raven-js and related modules --- package.json | 1 - pnpm-lock.yaml | 8 ---- src/init/consented.ts | 39 ++++++--------- src/lib/__mocks__/raven.ts | 9 ---- src/lib/raven.ts | 88 ---------------------------------- src/utils/report-error.spec.ts | 55 --------------------- src/utils/report-error.ts | 44 ----------------- 7 files changed, 14 insertions(+), 230 deletions(-) delete mode 100644 src/lib/__mocks__/raven.ts delete mode 100644 src/lib/raven.ts delete mode 100644 src/utils/report-error.spec.ts delete mode 100644 src/utils/report-error.ts diff --git a/package.json b/package.json index 19fe0ebe2..c1904a1dd 100644 --- a/package.json +++ b/package.json @@ -108,7 +108,6 @@ "fastdom": "^1.0.11", "lodash-es": "^4.17.21", "process": "^0.11.10", - "raven-js": "^3.27.2", "tslib": "^2.6.2", "web-vitals": "^4.2.1" }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 320f5ed03..012fc0b16 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -23,9 +23,6 @@ dependencies: process: specifier: ^0.11.10 version: 0.11.10 - raven-js: - specifier: ^3.27.2 - version: 3.27.2 tslib: specifier: ^2.6.2 version: 2.7.0 @@ -7919,11 +7916,6 @@ packages: resolution: {integrity: sha512-Hrgsx+orqoygnmhFbKaHE6c296J+HTAQXoxEF6gNupROmmGJRoyzfG3ccAveqCBrwr/2yxQ5BVd/GTl5agOwSg==} engines: {node: '>= 0.6'} - /raven-js@3.27.2: - resolution: {integrity: sha512-mFWQcXnhRFEQe5HeFroPaEghlnqy7F5E2J3Fsab189ondqUzcjwSVi7el7F36cr6PvQYXoZ1P2F5CSF2/azeMQ==} - deprecated: Please upgrade to @sentry/browser. See the migration guide https://bit.ly/3ybOlo7 - dev: false - /raw-body@2.5.2: resolution: {integrity: sha512-8zGqypfENjCIqGhgXToC8aB2r7YrBX+AQAfIPs/Mlk+BtPTztOvTS01NRW/3Eh60J+a48lt8qsCzirQ6loCVfA==} engines: {node: '>= 0.8'} diff --git a/src/init/consented.ts b/src/init/consented.ts index 09c469ff1..d86f0427a 100644 --- a/src/init/consented.ts +++ b/src/init/consented.ts @@ -27,11 +27,6 @@ import { init as initMessenger } from './consented/messenger'; type Modules = Array<[`${string}-${string}`, () => Promise]>; -const tags: Record = { - feature: 'commercial', - bundle: 'standalone', -}; - // modules necessary to load the first ads on the page const commercialBaseModules: Modules = []; @@ -73,18 +68,15 @@ const loadModules = (modules: Modules, eventName: string) => { modules.forEach((module) => { const [moduleName, moduleInit] = module; - catchErrorsWithContext( + catchErrorsWithContext([ [ - [ - moduleName, - function pushAfterComplete(): void { - const result = moduleInit(); - modulePromises.push(result); - }, - ], + moduleName, + function pushAfterComplete(): void { + const result = moduleInit(); + modulePromises.push(result); + }, ], - tags, - ); + ]); }); return Promise.allSettled(modulePromises).then(() => { @@ -121,18 +113,15 @@ const bootCommercial = async (): Promise => { // Init Commercial event timers EventTimer.init(); - catchErrorsWithContext( + catchErrorsWithContext([ [ - [ - 'ga-user-timing-commercial-start', - function runTrackPerformance() { - EventTimer.get().mark('commercialStart'); - EventTimer.get().mark('commercialBootStart'); - }, - ], + 'ga-user-timing-commercial-start', + function runTrackPerformance() { + EventTimer.get().mark('commercialStart'); + EventTimer.get().mark('commercialBootStart'); + }, ], - tags, - ); + ]); // Stub the command queue // @ts-expect-error -- it’s a stub, not the whole Googletag object diff --git a/src/lib/__mocks__/raven.ts b/src/lib/__mocks__/raven.ts deleted file mode 100644 index c6cfbc3e4..000000000 --- a/src/lib/__mocks__/raven.ts +++ /dev/null @@ -1,9 +0,0 @@ -// eslint-disable-next-line import/no-default-export -- Allow this here -export default { - wrap(fn: unknown): unknown { - return fn; - }, - captureException(): void { - // Do nothing - }, -}; diff --git a/src/lib/raven.ts b/src/lib/raven.ts deleted file mode 100644 index 3428deb97..000000000 --- a/src/lib/raven.ts +++ /dev/null @@ -1,88 +0,0 @@ -import type { RavenOptions } from 'raven-js'; -import raven from 'raven-js'; -import { adblockInUse } from './detect/detect-adblock'; - -const { - // linter, keep this multi-line - sentryPublicApiKey, - sentryHost, - edition, - contentType, - revisionNumber, - isDev, -} = window.guardian.config.page; -const { enableSentryReporting } = window.guardian.config.switches; -const sentryUrl = `https://${sentryPublicApiKey}@${sentryHost}`; - -let adblockBeingUsed = false; - -const sentryOptions: RavenOptions = { - whitelistUrls: [ - // localhost will not log errors, but call `shouldSendCallback` - /localhost/, - /assets\.guim\.co\.uk/, - /ophan\.co\.uk/, - ], - - tags: { - edition, - contentType, - revisionNumber, - }, - - ignoreErrors: [ - "Can't execute code from a freed script", - /There is no space left matching rules from/gi, - 'Top comments failed to load:', - 'Comments failed to load:', - /InvalidStateError/gi, - /Fetch error:/gi, - 'Network request failed', - 'This video is no longer available.', - 'UnknownError', - 'TypeError: Failed to fetch', - 'TypeError: NetworkError when attempting to fetch resource', - - // weatherapi/city.json frequently 404s and lib/fetch-json throws an error - 'Fetch error while requesting https://api.nextgen.guardianapps.co.uk/weatherapi/city.json:', - ], - - dataCallback(data: { tags: { origin?: string }; culprit?: string }) { - const { culprit } = data; - const resp = data; - const culpritMatches = culprit ? /j.ophan.co.uk/.test(culprit) : false; - - if (culprit) { - resp.culprit = culprit.replace(/\/[a-z\d]{32}(\/[^/]+)$/, '$1'); - } - - resp.tags.origin = culpritMatches ? 'ophan' : 'app'; - - return resp; - }, - - shouldSendCallback(data: { tags: { ignored?: unknown } }) { - const isIgnored = !!data.tags.ignored; - - // Sample at a very small rate. - const isInSample = Math.random() < 0.008; - - if (isDev && !isIgnored) { - console.warn('Raven captured event.', data); - } - - return ( - !!enableSentryReporting && - isInSample && - !isIgnored && - !adblockBeingUsed - ); - }, -}; - -void adblockInUse.then((isInUse: boolean) => { - adblockBeingUsed = isInUse; -}); - -// eslint-disable-next-line import/no-default-export -- Allow this default export -export default raven.config(sentryUrl, sentryOptions).install(); diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts deleted file mode 100644 index f7f5a8d0e..000000000 --- a/src/utils/report-error.spec.ts +++ /dev/null @@ -1,55 +0,0 @@ -import fakeRaven from 'raven-js'; -// import { reportError } from './report-error'; - -jest.mock('raven-js', () => ({ - config() { - return this; - }, - install() { - return this; - }, - captureException: jest.fn(), -})); - -describe('report-error', () => { - beforeEach(() => { - jest.spyOn(global.Math, 'random').mockReturnValue(0.5); - }); - - afterEach(() => { - jest.spyOn(global.Math, 'random').mockRestore(); - jest.resetAllMocks(); - }); - - const error = new Error('Something broke.'); - const tags = { test: 'testValue' }; - const ravenMetaData = { tags: tags }; - - test('Does NOT throw an error', () => { - expect(() => { - window.guardian.modules.sentry.reportError(error, 'commercial'); - }).not.toThrowError(error); - - expect(fakeRaven.captureException).toHaveBeenCalledWith( - error, - ravenMetaData, - ); - }); - - test('Does throw an error', () => { - expect(() => { - window.guardian.modules.sentry.reportError(error, 'commercial'); - }).toThrowError(error); - - expect(fakeRaven.captureException).toHaveBeenCalledWith( - error, - ravenMetaData, - ); - }); - - test('Applies a sampling rate that prevents a sample of errors being reporting', () => { - window.guardian.modules.sentry.reportError(error, 'commercial'); - - expect(fakeRaven.captureException).not.toHaveBeenCalled(); - }); -}); diff --git a/src/utils/report-error.ts b/src/utils/report-error.ts deleted file mode 100644 index bf722492d..000000000 --- a/src/utils/report-error.ts +++ /dev/null @@ -1,44 +0,0 @@ -import raven from 'lib/raven'; - -type FrontendError = (Error & { reported?: boolean }) | string; - -const convertError = (err: unknown): Error => { - if (err instanceof Error) { - return err; - } - if (typeof err === 'string') { - return new Error(err); - } - return new Error(String(err)); -}; - -/** - * Report errors to Sentry with optional tags metadata. - * @param err - An error object or string. - * @param tags - Tags to assign to the event. - * @param shouldThrow - Flag to optionally re-throw the error (true by default). This halts - * execution to ensure the error is still logged to the console via browsers' built-in logging - * for uncaught exceptions. This is optional because sometimes we log errors for tracking - * non-error data. - * @param sampleRate - A sampling rate to apply to events, used for highly frequent errors. - * A value of 0 will send no events, and a value of 1 (default) will send an event for - * users that have downloaded the raven client (0.8% of all users). - * See https://github.com/guardian/frontend/blob/faf2bb4f5e4aa123d1da86ea98cbd693c4e8ffd0/static/src/javascripts/lib/utils/raven.ts#L68 - */ -const reportError = ( - err: unknown, - tags: Record, - shouldThrow = true, - sampleRate = 1, -): void => { - const localError: FrontendError = convertError(err); - if (sampleRate >= Math.random()) { - raven.captureException(localError, { tags }); - } - if (shouldThrow) { - localError.reported = true; - throw localError; - } -}; - -export { convertError, reportError }; From 2e91fd41426fae11847065a3e11e120148f2b3b4 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Thu, 22 Aug 2024 16:37:15 +0100 Subject: [PATCH 06/24] add changeset --- .changeset/good-planets-smile.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/good-planets-smile.md diff --git a/.changeset/good-planets-smile.md b/.changeset/good-planets-smile.md new file mode 100644 index 000000000..4878bb06e --- /dev/null +++ b/.changeset/good-planets-smile.md @@ -0,0 +1,5 @@ +--- +'@guardian/commercial': patch +--- + +remove raven client from commercial for error reporting From a95b042a109be949d4f025c725a0477cde2855f6 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Fri, 23 Aug 2024 13:27:28 +0100 Subject: [PATCH 07/24] DRY refactor refactor report-error module and function calls to it --- src/core/messenger.ts | 3 +- src/define/create-advert.ts | 6 ++-- src/events/on-slot-render.ts | 6 ++-- src/events/render-advert.ts | 13 ++------ src/init/consented.ts | 7 ++-- src/init/consented/dynamic-ad-slots.ts | 4 +-- src/init/consented/prepare-permutive.ts | 3 +- src/insert/fill-dynamic-advert-slot.ts | 7 ++-- src/insert/spacefinder/article.spec.ts | 3 ++ .../spacefinder/liveblog-adverts.spec.ts | 3 ++ src/insert/spacefinder/space-filler.ts | 3 +- src/lib/cookies.js | 2 +- .../dfp/non-refreshable-line-items.spec.ts | 15 +++------ src/lib/dfp/non-refreshable-line-items.ts | 4 +-- src/utils/report-error.spec.ts | 32 +++++++++++++++++++ src/utils/report-error.ts | 4 +++ src/utils/robust.spec.ts | 11 ++----- src/utils/robust.ts | 10 +++--- 18 files changed, 78 insertions(+), 58 deletions(-) create mode 100644 src/utils/report-error.spec.ts create mode 100644 src/utils/report-error.ts diff --git a/src/core/messenger.ts b/src/core/messenger.ts index c9e96fca2..7648ed0c8 100644 --- a/src/core/messenger.ts +++ b/src/core/messenger.ts @@ -1,3 +1,4 @@ +import { reportError } from 'utils/report-error'; import { postMessage } from './messenger/post-message'; /** @@ -371,7 +372,7 @@ const onMessage = async (event: MessageEvent): Promise => { respond(message.id, event.source, null, response); }) .catch((ex: Error) => { - window.guardian.modules.sentry.reportError(ex, 'native-ads'); + reportError(ex, 'native-ads'); respond( message.id, event.source, diff --git a/src/define/create-advert.ts b/src/define/create-advert.ts index b12a36a1c..06e43ab32 100644 --- a/src/define/create-advert.ts +++ b/src/define/create-advert.ts @@ -1,5 +1,6 @@ import { log } from '@guardian/libs'; import type { SizeMapping } from 'core/ad-sizes'; +import { reportError } from 'utils/report-error'; import { Advert } from './Advert'; const createAdvert = ( @@ -20,10 +21,7 @@ const createAdvert = ( log('commercial', errMsg); if (!navigator.userAgent.includes('DuckDuckGo')) { - window.guardian.modules.sentry.reportError( - new Error(errMsg), - 'commercial', - ); + reportError(new Error(errMsg), 'commercial'); } return null; diff --git a/src/events/on-slot-render.ts b/src/events/on-slot-render.ts index 3d73678f0..948f2b94e 100644 --- a/src/events/on-slot-render.ts +++ b/src/events/on-slot-render.ts @@ -1,7 +1,7 @@ import { isString } from '@guardian/libs'; import type { AdSize } from 'core/ad-sizes'; import { createAdSize } from 'core/ad-sizes'; -// import { reportError } from 'utils/report-error'; +import { reportError } from 'utils/report-error'; import { getAdvertById } from '../lib/dfp/get-advert-by-id'; import { emptyAdvert } from './empty-advert'; import { renderAdvert } from './render-advert'; @@ -11,9 +11,7 @@ const reportEmptyResponse = () => { // let's report these and diagnose the problem in sentry. // Keep the sample rate low, otherwise we'll get rate-limited (report-error will also sample down) if (Math.random() < 1 / 10_000) { - // const adUnitPath = event.slot.getAdUnitPath(); - - window.guardian.modules.sentry.reportError( + reportError( new Error('dfp returned an empty ad response'), 'commercial', ); diff --git a/src/events/render-advert.ts b/src/events/render-advert.ts index ebfc3fc19..82a44b1fe 100644 --- a/src/events/render-advert.ts +++ b/src/events/render-advert.ts @@ -1,7 +1,7 @@ import { adSizes } from 'core/ad-sizes'; import { $$ } from 'utils/$$'; import fastdom from 'utils/fastdom-promise'; -// import { reportError } from 'utils/report-error'; +import { reportError } from 'utils/report-error'; import type { Advert } from '../define/Advert'; import { getAdIframe } from '../lib/dfp/get-ad-iframe'; import { renderAdvertLabel } from './render-advert-label'; @@ -178,14 +178,7 @@ const addContainerClass = (adSlotNode: HTMLElement, isRendered: boolean) => { } }); }; -//Just a local test to check function from window.guardian.modules.sentry -if (process.env.NODE_ENV !== 'production') { - try { - throw new Error('>>>>Test error for sentry'); - } catch (err) { - window.guardian.modules.sentry.reportError(err, 'test-feature'); - } -} + /** * @param advert - as defined in lib/dfp/Advert * @param slotRenderEndedEvent - GPT slotRenderEndedEvent @@ -233,7 +226,7 @@ const renderAdvert = ( .then(() => isRendered); }) .catch((err) => { - window.guardian.modules.sentry.reportError(err, 'commercial'); + reportError(err, 'commercial'); return Promise.resolve(false); }); }; diff --git a/src/init/consented.ts b/src/init/consented.ts index d86f0427a..45e5ec6ed 100644 --- a/src/init/consented.ts +++ b/src/init/consented.ts @@ -19,7 +19,7 @@ import { init as setAdTestInLabelsCookie } from 'init/shared/set-adtest-in-label import { init as prepareAdVerification } from 'lib/ad-verification/prepare-ad-verification'; import { commercialFeatures } from 'lib/commercial-features'; import { adSlotIdPrefix } from 'lib/dfp/dfp-env-globals'; -// import { reportError } from 'utils/report-error'; +import { reportError } from 'utils/report-error'; import { catchErrorsWithContext } from 'utils/robust'; import { initDfpListeners } from './consented/dfp-listeners'; import { initDynamicAdSlots } from './consented/dynamic-ad-slots'; @@ -139,10 +139,7 @@ const bootCommercial = async (): Promise => { await Promise.all(promises).then(recordCommercialMetrics); } catch (error) { // report async errors in bootCommercial to Sentry with the commercial feature tag - window.guardian.modules.sentry.reportError( - error, - 'consented-commercial', - ); + reportError(error, 'commercial'); } }; diff --git a/src/init/consented/dynamic-ad-slots.ts b/src/init/consented/dynamic-ad-slots.ts index e8b3421bb..4f7d06e03 100644 --- a/src/init/consented/dynamic-ad-slots.ts +++ b/src/init/consented/dynamic-ad-slots.ts @@ -5,7 +5,7 @@ import { init as initHighMerch } from 'insert/high-merch'; import { init as initMobileCrosswordsAdvert } from 'insert/mobile-crossword-banner'; import { init as initMobileSticky } from 'insert/mobile-sticky'; import { init as initLiveblogAdverts } from 'insert/spacefinder/liveblog-adverts'; -// import { reportError } from 'utils/report-error'; +import { reportError } from 'utils/report-error'; import { initArticleBodyAdverts } from './article-body-adverts'; type Modules = Array<[`${string}-${string}`, () => Promise]>; @@ -27,7 +27,7 @@ export const initDynamicAdSlots = async (): Promise => { try { await init(); } catch (error) { - window.guardian.modules.sentry.reportError(error, 'commercial'); + reportError(error, 'commercial'); } }), ); diff --git a/src/init/consented/prepare-permutive.ts b/src/init/consented/prepare-permutive.ts index f9170444a..3e65dfe60 100644 --- a/src/init/consented/prepare-permutive.ts +++ b/src/init/consented/prepare-permutive.ts @@ -1,5 +1,6 @@ import type { Edition } from 'core/types'; import type { Config, PageConfig, Permutive, UserConfig } from 'types/global'; +import { reportError } from 'utils/report-error'; interface PermutivePageConfig { page: Pick< @@ -156,7 +157,7 @@ const runPermutive = ( page: payload, }); } catch (err) { - logger(err); + logger(err, 'commercial'); } }; diff --git a/src/insert/fill-dynamic-advert-slot.ts b/src/insert/fill-dynamic-advert-slot.ts index 4c0033f1a..0184fc032 100644 --- a/src/insert/fill-dynamic-advert-slot.ts +++ b/src/insert/fill-dynamic-advert-slot.ts @@ -1,6 +1,6 @@ import { log } from '@guardian/libs'; import type { SizeMapping } from 'core/ad-sizes'; -// import { reportError } from 'utils/report-error'; +import { reportError } from 'utils/report-error'; import type { Advert } from '../define/Advert'; import { createAdvert } from '../define/create-advert'; import { enableLazyLoad } from '../display/lazy-load'; @@ -29,10 +29,7 @@ const fillDynamicAdSlot = ( if (dfpEnv.adverts.has(adSlot.id)) { const errorMessage = `Attempting to add slot with exisiting id ${adSlot.id}`; log('commercial', errorMessage); - window.guardian.modules.sentry.reportError( - Error(errorMessage), - 'commercial', - ); + reportError(Error(errorMessage), 'commercial'); return; } diff --git a/src/insert/spacefinder/article.spec.ts b/src/insert/spacefinder/article.spec.ts index 445139186..4f143847d 100644 --- a/src/insert/spacefinder/article.spec.ts +++ b/src/insert/spacefinder/article.spec.ts @@ -2,6 +2,9 @@ import { spaceFiller } from 'insert/spacefinder/space-filler'; import { commercialFeatures } from 'lib/commercial-features'; import { init } from './article'; +jest.mock('utils/report-error', () => ({ + reportError: jest.fn(), +})); jest.mock('lib/header-bidding/prebid/prebid', () => ({ requestBids: jest.fn(), })); diff --git a/src/insert/spacefinder/liveblog-adverts.spec.ts b/src/insert/spacefinder/liveblog-adverts.spec.ts index a9e2ee53e..032fe19a0 100644 --- a/src/insert/spacefinder/liveblog-adverts.spec.ts +++ b/src/insert/spacefinder/liveblog-adverts.spec.ts @@ -7,6 +7,9 @@ const { getStartingContentBlock, } = _; +jest.mock('utils/report-error', () => ({ + reportError: jest.fn(), +})); jest.mock('lib/header-bidding/prebid/prebid', () => ({ requestBids: jest.fn(), })); diff --git a/src/insert/spacefinder/space-filler.ts b/src/insert/spacefinder/space-filler.ts index 2ca3cc39a..2e54c5628 100644 --- a/src/insert/spacefinder/space-filler.ts +++ b/src/insert/spacefinder/space-filler.ts @@ -4,6 +4,7 @@ import type { SpacefinderWriter, } from 'insert/spacefinder/spacefinder'; import { findSpace, SpaceError } from 'insert/spacefinder/spacefinder'; +import { reportError } from 'utils/report-error'; class SpaceFiller { queue = Promise.resolve(true); @@ -34,7 +35,7 @@ class SpaceFiller { this.queue = this.queue.then(insertNextContent).catch((e) => { // e.g. if writer fails - window.guardian.modules.sentry.reportError(e, 'space-filler'); + reportError(e, 'space-filler'); return false; }); diff --git a/src/lib/cookies.js b/src/lib/cookies.js index c76290f02..7d18bcc84 100644 --- a/src/lib/cookies.js +++ b/src/lib/cookies.js @@ -43,7 +43,7 @@ const addCookie = (name, value, daysToLive, isCrossSubdomain = false) => { if (!isValidCookieValue(name) || !isValidCookieValue(value)) { if (window.guardian?.modules?.sentry?.reportError) { - window.guardian.modules.sentry.reportError( + reportError( new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), 'commercial', ); diff --git a/src/lib/dfp/non-refreshable-line-items.spec.ts b/src/lib/dfp/non-refreshable-line-items.spec.ts index 91b883fac..0a077b840 100644 --- a/src/lib/dfp/non-refreshable-line-items.spec.ts +++ b/src/lib/dfp/non-refreshable-line-items.spec.ts @@ -1,3 +1,4 @@ +import { reportError } from 'utils/report-error'; import { fetchNonRefreshableLineItemIds, memoizedFetchNonRefreshableLineItemIds, @@ -17,9 +18,7 @@ describe('nonRefreshableLineItems', () => { const ids = await fetchNonRefreshableLineItemIds(); - expect( - window.guardian.modules.sentry.reportError, - ).not.toHaveBeenCalled(); + expect(reportError).not.toHaveBeenCalled(); expect(ids).toEqual([1, 2, 3]); }); @@ -39,9 +38,7 @@ describe('nonRefreshableLineItems', () => { 'Failed to parse non-refreshable line items as an array', ); - expect( - window.guardian.modules.sentry.reportError, - ).not.toHaveBeenCalled(); + expect(reportError).not.toHaveBeenCalled(); }); it('returns undefined when the API returns string array', async () => { @@ -59,9 +56,7 @@ describe('nonRefreshableLineItems', () => { 'Failed to parse element in non-refreshable line item array as number', ); - expect( - window.guardian.modules.sentry.reportError, - ).not.toHaveBeenCalled(); + expect(reportError).not.toHaveBeenCalled(); }); it('returns undefined and reports error when the API call fails', async () => { @@ -79,7 +74,7 @@ describe('nonRefreshableLineItems', () => { 'Failed to fetch non-refreshable line items', ); - expect(window.guardian.modules.sentry.reportError).toHaveBeenCalledWith( + expect(reportError).toHaveBeenCalledWith( new Error('Failed to fetch non-refreshable line items'), 'commercial', ); diff --git a/src/lib/dfp/non-refreshable-line-items.ts b/src/lib/dfp/non-refreshable-line-items.ts index f6c44ab61..ec5fc9855 100644 --- a/src/lib/dfp/non-refreshable-line-items.ts +++ b/src/lib/dfp/non-refreshable-line-items.ts @@ -1,5 +1,5 @@ import { memoize } from 'lodash-es'; -// import { reportError } from 'utils/report-error'; +import { reportError } from 'utils/report-error'; export const fetchNonRefreshableLineItemIds = async (): Promise => { // When the env is CODE or local, use the CODE env's non-refreshable line items file @@ -42,7 +42,7 @@ export const fetchNonRefreshableLineItemIds = async (): Promise => { // Report an error to Sentry if we don't get an ok response // Note that in other cases (JSON parsing failure) we throw but don't report the error const error = Error('Failed to fetch non-refreshable line items'); - window.guardian.modules.sentry.reportError(error, 'commercial'); + reportError(error, 'commercial'); throw error; }; diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts new file mode 100644 index 000000000..1b6372d6c --- /dev/null +++ b/src/utils/report-error.spec.ts @@ -0,0 +1,32 @@ +import { reportError } from './report-error'; + +describe('report-error', () => { + beforeEach(() => { + jest.spyOn(global.Math, 'random').mockReturnValue(0.5); + }); + + afterEach(() => { + jest.spyOn(global.Math, 'random').mockRestore(); + jest.resetAllMocks(); + }); + + const error = new Error('Something broke.'); + const tags = 'testValue'; + + test('Does NOT throw an error', () => { + expect(() => { + reportError(error, tags); + }).not.toThrowError(error); + }); + + // temporarily disabled due to flakiness + // test('Does throw an error', () => { + // expect(() => { + // reportError(error, tags); + // }).toThrowError(error); + // }); + + test('Applies a sampling rate that prevents a sample of errors being reporting', () => { + reportError(error, tags); + }); +}); diff --git a/src/utils/report-error.ts b/src/utils/report-error.ts new file mode 100644 index 000000000..b75be6f92 --- /dev/null +++ b/src/utils/report-error.ts @@ -0,0 +1,4 @@ +// The report error function replace the older Raven based error reporting with the new Sentry based error reporting. +// This function is used to report errors to Sentry. +// This uses the `reportError` function from the `window.guardian.modules.sentry` object. +export const reportError = window.guardian.modules.sentry.reportError; diff --git a/src/utils/robust.spec.ts b/src/utils/robust.spec.ts index 4d6055631..a085b2f4e 100644 --- a/src/utils/robust.spec.ts +++ b/src/utils/robust.spec.ts @@ -1,3 +1,4 @@ +import { reportError } from 'utils/report-error'; import { _, catchErrorsWithContext } from './robust'; import type { Modules } from './robust'; @@ -9,7 +10,6 @@ beforeEach(() => { jest.clearAllMocks(); origConsoleWarn = window.console.warn; window.console.warn = jest.fn(); - window.guardian.modules.sentry.reportError = jest.fn(); }); afterEach(() => { @@ -40,17 +40,12 @@ describe('robust', () => { test('catchAndLogError() - default reporter with no error', () => { catchAndLogError('test', noError); - expect( - window.guardian.modules.sentry.reportError, - ).not.toHaveBeenCalled(); + expect(reportError).not.toHaveBeenCalled(); }); test('catchAndLogError() - default reporter with error', () => { catchAndLogError('test', throwError); - expect(window.guardian.modules.sentry.reportError).toHaveBeenCalledWith( - ERROR, - META, - ); + expect(reportError).toHaveBeenCalledWith(ERROR, META); }); test('catchErrorsWithContext()', () => { diff --git a/src/utils/robust.ts b/src/utils/robust.ts index c819e0f1e..47e6531d9 100644 --- a/src/utils/robust.ts +++ b/src/utils/robust.ts @@ -1,7 +1,9 @@ /* - Swallows (and reports) exceptions. Designed to wrap around modules at the "bootstrap" level. - For example "comments throwing an exception should not stop auto refresh" - */ +Swallows (and reports) exceptions. Designed to wrap around modules at the "bootstrap" level. +For example "comments throwing an exception should not stop auto refresh" +*/ + +import { reportError } from 'utils/report-error'; type ModuleFunction = () => void; type Module = [string, ModuleFunction]; @@ -21,7 +23,7 @@ const catchErrors = (fn: ModuleFunction): Error | undefined => { const logError = (moduleName: string, error: Error): void => { window.console.warn('Caught error.', error.stack); - window.guardian.modules.sentry.reportError(error, 'commercial'); + reportError(error, 'commercial'); }; const catchAndLogError = (name: string, fn: ModuleFunction): void => { From 97d74c86f6797050b2d18b36a57fdb4a004b6534 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Thu, 12 Sep 2024 15:55:24 +0100 Subject: [PATCH 08/24] remove errorHandler as a parameter --- src/core/messenger.ts | 25 ++++--------------------- src/init/consented/messenger.ts | 1 - src/init/consentless.ts | 2 +- 3 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/core/messenger.ts b/src/core/messenger.ts index 7648ed0c8..ca7c3230c 100644 --- a/src/core/messenger.ts +++ b/src/core/messenger.ts @@ -118,11 +118,6 @@ type ListenerOptions = { window?: WindowProxy; }; -type MessengerErrorHandler = ( - err: Error, - features: Record, -) => void; - /** * Types of functions to register a listener for a given type of iframe message */ @@ -496,23 +491,12 @@ export const unregister: UnregisterListener = (type, callback, options) => { * @param persistentListeners The persistent listener registration functions */ export const init = ( - listeners: Array< - ( - register: RegisterListener, - errorHandler: MessengerErrorHandler, - ) => void - >, - persistentListeners: Array< - ( - register: RegisterPersistentListener, - errorHandler: MessengerErrorHandler, - ) => void - >, - errorHandler: MessengerErrorHandler, + listeners: Array<(register: RegisterListener) => void>, + persistentListeners: Array<(register: RegisterPersistentListener) => void>, ): void => { - listeners.forEach((moduleInit) => moduleInit(register, errorHandler)); + listeners.forEach((moduleInit) => moduleInit(register)); persistentListeners.forEach((moduleInit) => - moduleInit(registerPersistentListener, errorHandler), + moduleInit(registerPersistentListener), ); }; @@ -524,5 +508,4 @@ export type { RegisterPersistentListener, UnregisterListener, ListenerOptions, - MessengerErrorHandler, }; diff --git a/src/init/consented/messenger.ts b/src/init/consented/messenger.ts index 5f8005844..152f3e05e 100644 --- a/src/init/consented/messenger.ts +++ b/src/init/consented/messenger.ts @@ -31,7 +31,6 @@ initMessenger( passback, ], [scroll, viewport], - reportError, ); const init = () => Promise.resolve(); diff --git a/src/init/consentless.ts b/src/init/consentless.ts index 93087b250..0bc2f7bcd 100644 --- a/src/init/consentless.ts +++ b/src/init/consentless.ts @@ -13,7 +13,7 @@ import { init as setAdTestInLabelsCookie } from 'init/shared/set-adtest-in-label const bootConsentless = async (consentState: ConsentState): Promise => { const consentlessModuleList = [ - initMessenger([background, resize, type], [], reportError), + initMessenger([background, resize, type], []), setAdTestCookie(), setAdTestInLabelsCookie(), initConsentless(consentState), From f12f6774b0a80ee2af5ec6c1e5f7de91f8ececf8 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Fri, 13 Sep 2024 11:15:40 +0100 Subject: [PATCH 09/24] add custom wrap report error function --- src/init/consented/dfp-listeners.ts | 11 +++++++++-- src/utils/report-error.ts | 13 +++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/init/consented/dfp-listeners.ts b/src/init/consented/dfp-listeners.ts index e743db7e8..f94854626 100644 --- a/src/init/consented/dfp-listeners.ts +++ b/src/init/consented/dfp-listeners.ts @@ -1,3 +1,4 @@ +import { wrapWithErrorReporting } from 'utils/report-error'; import { onSlotLoad } from '../../events/on-slot-load'; import { onSlotRender } from '../../events/on-slot-render'; import { onSlotViewableFunction } from '../../events/on-slot-viewable'; @@ -6,8 +7,14 @@ const initDfpListeners = (): Promise => { window.googletag.cmd.push(() => { const pubads = window.googletag.pubads(); - pubads.addEventListener('slotRenderEnded', onSlotRender); - pubads.addEventListener('slotOnload', onSlotLoad); + pubads.addEventListener( + 'slotRenderEnded', + wrapWithErrorReporting(onSlotRender), + ); + pubads.addEventListener( + 'slotOnload', + wrapWithErrorReporting(onSlotLoad), + ); pubads.addEventListener('impressionViewable', onSlotViewableFunction()); }); return Promise.resolve(); diff --git a/src/utils/report-error.ts b/src/utils/report-error.ts index b75be6f92..ec3f1e822 100644 --- a/src/utils/report-error.ts +++ b/src/utils/report-error.ts @@ -2,3 +2,16 @@ // This function is used to report errors to Sentry. // This uses the `reportError` function from the `window.guardian.modules.sentry` object. export const reportError = window.guardian.modules.sentry.reportError; + +export const wrapWithErrorReporting = ( + fn: (...args: unknown[]) => void, + tags: Record = {}, +) => { + return function (...args: unknown[]) { + try { + fn(...args); + } catch (e) { + reportError(e, JSON.stringify(tags)); + } + }; +}; From 46aef07c367906b059cc252ffc2648824528ce88 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Fri, 13 Sep 2024 12:02:06 +0100 Subject: [PATCH 10/24] remove extraneous check for function on window --- src/lib/cookies.js | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/lib/cookies.js b/src/lib/cookies.js index 7d18bcc84..0920ece33 100644 --- a/src/lib/cookies.js +++ b/src/lib/cookies.js @@ -42,17 +42,10 @@ const addCookie = (name, value, daysToLive, isCrossSubdomain = false) => { const expires = new Date(); if (!isValidCookieValue(name) || !isValidCookieValue(value)) { - if (window.guardian?.modules?.sentry?.reportError) { - reportError( - new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), - 'commercial', - ); - } else { - console.error( - 'Error reporting is not available:', - new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), - ); - } + reportError( + new Error(`${ERR_INVALID_COOKIE_NAME} .${name}=${value}`), + 'commercial', + ); } if (daysToLive) { From d927b2e1c6a7568b517a91696a9ef6aadd932b93 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Fri, 13 Sep 2024 12:02:35 +0100 Subject: [PATCH 11/24] remove flakey test --- src/utils/report-error.spec.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts index 1b6372d6c..83fbe72d1 100644 --- a/src/utils/report-error.spec.ts +++ b/src/utils/report-error.spec.ts @@ -19,13 +19,6 @@ describe('report-error', () => { }).not.toThrowError(error); }); - // temporarily disabled due to flakiness - // test('Does throw an error', () => { - // expect(() => { - // reportError(error, tags); - // }).toThrowError(error); - // }); - test('Applies a sampling rate that prevents a sample of errors being reporting', () => { reportError(error, tags); }); From d4aa2e2273c0cc9e9435382eff79c2f256cd8e3e Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Fri, 13 Sep 2024 12:04:57 +0100 Subject: [PATCH 12/24] remove comment --- src/utils/report-error.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/utils/report-error.ts b/src/utils/report-error.ts index ec3f1e822..dafdeea84 100644 --- a/src/utils/report-error.ts +++ b/src/utils/report-error.ts @@ -1,4 +1,3 @@ -// The report error function replace the older Raven based error reporting with the new Sentry based error reporting. // This function is used to report errors to Sentry. // This uses the `reportError` function from the `window.guardian.modules.sentry` object. export const reportError = window.guardian.modules.sentry.reportError; From be4f0580178121248659a9882b7f698b0617968b Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Fri, 13 Sep 2024 12:35:00 +0100 Subject: [PATCH 13/24] refactor wrapWithErrorReporting for linting --- src/utils/report-error.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/utils/report-error.ts b/src/utils/report-error.ts index dafdeea84..5401b16c3 100644 --- a/src/utils/report-error.ts +++ b/src/utils/report-error.ts @@ -1,16 +1,20 @@ // This function is used to report errors to Sentry. // This uses the `reportError` function from the `window.guardian.modules.sentry` object. -export const reportError = window.guardian.modules.sentry.reportError; +const reportError = window.guardian.modules.sentry.reportError; -export const wrapWithErrorReporting = ( - fn: (...args: unknown[]) => void, +type ErrorReportingFunction = (event: T) => void; + +const wrapWithErrorReporting = ( + fn: ErrorReportingFunction, tags: Record = {}, -) => { - return function (...args: unknown[]) { +): ErrorReportingFunction => { + return function (event: T) { try { - fn(...args); + fn(event); } catch (e) { reportError(e, JSON.stringify(tags)); } }; }; + +export { reportError, wrapWithErrorReporting }; From 139d115816183149a4c304308ae7b3356bf43e3f Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Fri, 13 Sep 2024 12:59:42 +0100 Subject: [PATCH 14/24] remove sampling rate test --- src/utils/report-error.spec.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts index 83fbe72d1..bea8734dd 100644 --- a/src/utils/report-error.spec.ts +++ b/src/utils/report-error.spec.ts @@ -18,8 +18,4 @@ describe('report-error', () => { reportError(error, tags); }).not.toThrowError(error); }); - - test('Applies a sampling rate that prevents a sample of errors being reporting', () => { - reportError(error, tags); - }); }); From d5b7391658fb6899976a0ffbe92d27fce45662e3 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Wed, 18 Sep 2024 15:21:16 +0100 Subject: [PATCH 15/24] re add tags arguments to reportError --- src/init/consented.ts | 21 ++++++++++++++------- src/types/global.ts | 6 +++++- src/utils/report-error.spec.ts | 4 ++-- src/utils/robust.spec.ts | 5 ++++- src/utils/robust.ts | 23 +++++++++++++++++------ 5 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/init/consented.ts b/src/init/consented.ts index 45e5ec6ed..36757eddc 100644 --- a/src/init/consented.ts +++ b/src/init/consented.ts @@ -27,6 +27,10 @@ import { init as initMessenger } from './consented/messenger'; type Modules = Array<[`${string}-${string}`, () => Promise]>; +const tags: Record = { + feature: 'commercial', + bundle: 'standalone', +}; // modules necessary to load the first ads on the page const commercialBaseModules: Modules = []; @@ -68,15 +72,18 @@ const loadModules = (modules: Modules, eventName: string) => { modules.forEach((module) => { const [moduleName, moduleInit] = module; - catchErrorsWithContext([ + catchErrorsWithContext( [ - moduleName, - function pushAfterComplete(): void { - const result = moduleInit(); - modulePromises.push(result); - }, + [ + moduleName, + function pushAfterComplete(): void { + const result = moduleInit(); + modulePromises.push(result); + }, + ], ], - ]); + tags, + ); }); return Promise.allSettled(modulePromises).then(() => { diff --git a/src/types/global.ts b/src/types/global.ts index c1203c726..1a4b6ac9f 100644 --- a/src/types/global.ts +++ b/src/types/global.ts @@ -470,7 +470,11 @@ declare global { offlineCount?: number; modules: { sentry: { - reportError: (error: unknown, feature: string) => void; + reportError: ( + error: unknown, + feature: string, + tags?: Record, + ) => void; }; }; }; diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts index bea8734dd..599a3950e 100644 --- a/src/utils/report-error.spec.ts +++ b/src/utils/report-error.spec.ts @@ -11,11 +11,11 @@ describe('report-error', () => { }); const error = new Error('Something broke.'); - const tags = 'testValue'; + const tags = { test: 'testValue' }; test('Does NOT throw an error', () => { expect(() => { - reportError(error, tags); + reportError(error, 'commercial', tags); }).not.toThrowError(error); }); }); diff --git a/src/utils/robust.spec.ts b/src/utils/robust.spec.ts index a085b2f4e..7087f5945 100644 --- a/src/utils/robust.spec.ts +++ b/src/utils/robust.spec.ts @@ -19,6 +19,9 @@ afterEach(() => { describe('robust', () => { const ERROR = new Error('Something broke.'); const META = 'commercial'; + const TAGS = { + module: 'test', + }; const noError = () => true; @@ -45,7 +48,7 @@ describe('robust', () => { test('catchAndLogError() - default reporter with error', () => { catchAndLogError('test', throwError); - expect(reportError).toHaveBeenCalledWith(ERROR, META); + expect(reportError).toHaveBeenCalledWith(ERROR, META, TAGS); }); test('catchErrorsWithContext()', () => { diff --git a/src/utils/robust.ts b/src/utils/robust.ts index 47e6531d9..951db6c57 100644 --- a/src/utils/robust.ts +++ b/src/utils/robust.ts @@ -21,21 +21,32 @@ const catchErrors = (fn: ModuleFunction): Error | undefined => { return error; }; -const logError = (moduleName: string, error: Error): void => { +const logError = ( + moduleName: string, + error: Error, + tags?: Record, +): void => { window.console.warn('Caught error.', error.stack); - reportError(error, 'commercial'); + reportError(error, 'commercial', { ...tags, module: moduleName }); }; -const catchAndLogError = (name: string, fn: ModuleFunction): void => { +const catchAndLogError = ( + name: string, + fn: ModuleFunction, + tags?: Record, +): void => { const error = catchErrors(fn); if (error) { - logError(name, error); + logError(name, error, tags); } }; -const catchErrorsWithContext = (modules: Modules): void => { - modules.forEach(([name, fn]) => catchAndLogError(name, fn)); +const catchErrorsWithContext = ( + modules: Modules, + tags?: Record, +): void => { + modules.forEach(([name, fn]) => catchAndLogError(name, fn, tags)); }; export { catchErrorsWithContext }; From f1871f1cd590fc737ce9c644ee0350b3bf53c725 Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Thu, 19 Sep 2024 16:35:32 +0100 Subject: [PATCH 16/24] add tags to on-slot-render --- src/events/on-slot-render.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/events/on-slot-render.ts b/src/events/on-slot-render.ts index 948f2b94e..0d50f8dca 100644 --- a/src/events/on-slot-render.ts +++ b/src/events/on-slot-render.ts @@ -6,14 +6,29 @@ import { getAdvertById } from '../lib/dfp/get-advert-by-id'; import { emptyAdvert } from './empty-advert'; import { renderAdvert } from './render-advert'; -const reportEmptyResponse = () => { +const reportEmptyResponse = ( + adSlotId: string, + event: googletag.events.SlotRenderEndedEvent, +) => { // This empty slot could be caused by a targeting problem, // let's report these and diagnose the problem in sentry. // Keep the sample rate low, otherwise we'll get rate-limited (report-error will also sample down) if (Math.random() < 1 / 10_000) { + const adUnitPath = event.slot.getAdUnitPath(); + const adTargetingKeys = event.slot.getTargetingKeys(); + const adTargetingKValues = adTargetingKeys.includes('k') + ? event.slot.getTargeting('k') + : []; + const adKeywords = adTargetingKValues.join(', '); reportError( new Error('dfp returned an empty ad response'), 'commercial', + { + feature: 'commercial', + adUnit: adUnitPath, + adSlot: adSlotId, + adKeywords, + }, ); } }; @@ -35,7 +50,7 @@ export const onSlotRender = ( if (event.isEmpty) { emptyAdvert(advert); - reportEmptyResponse(); + reportEmptyResponse(advert.id, event); advert.finishedRendering(false); } else { /** From 0875c60bab5d282d6d4a2d91f58c1250d568ce4c Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Thu, 19 Sep 2024 16:37:49 +0100 Subject: [PATCH 17/24] add tags to non-refreshable-line-items --- src/insert/fill-dynamic-advert-slot.ts | 4 +++- src/lib/dfp/non-refreshable-line-items.spec.ts | 3 ++- src/lib/dfp/non-refreshable-line-items.ts | 5 ++++- src/utils/report-error.ts | 2 +- 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/insert/fill-dynamic-advert-slot.ts b/src/insert/fill-dynamic-advert-slot.ts index 0184fc032..4f12bbb80 100644 --- a/src/insert/fill-dynamic-advert-slot.ts +++ b/src/insert/fill-dynamic-advert-slot.ts @@ -29,7 +29,9 @@ const fillDynamicAdSlot = ( if (dfpEnv.adverts.has(adSlot.id)) { const errorMessage = `Attempting to add slot with exisiting id ${adSlot.id}`; log('commercial', errorMessage); - reportError(Error(errorMessage), 'commercial'); + reportError(Error(errorMessage), 'commercial', { + adSlot: adSlot.id, + }); return; } diff --git a/src/lib/dfp/non-refreshable-line-items.spec.ts b/src/lib/dfp/non-refreshable-line-items.spec.ts index 0a077b840..a3eba848a 100644 --- a/src/lib/dfp/non-refreshable-line-items.spec.ts +++ b/src/lib/dfp/non-refreshable-line-items.spec.ts @@ -62,7 +62,7 @@ describe('nonRefreshableLineItems', () => { it('returns undefined and reports error when the API call fails', async () => { const response = { ok: false, - status: 404, + status: '404', }; Object.defineProperty(window, 'fetch', { @@ -77,6 +77,7 @@ describe('nonRefreshableLineItems', () => { expect(reportError).toHaveBeenCalledWith( new Error('Failed to fetch non-refreshable line items'), 'commercial', + { feature: 'commercial', status: response.status }, ); }); }); diff --git a/src/lib/dfp/non-refreshable-line-items.ts b/src/lib/dfp/non-refreshable-line-items.ts index ec5fc9855..15aecd352 100644 --- a/src/lib/dfp/non-refreshable-line-items.ts +++ b/src/lib/dfp/non-refreshable-line-items.ts @@ -42,7 +42,10 @@ export const fetchNonRefreshableLineItemIds = async (): Promise => { // Report an error to Sentry if we don't get an ok response // Note that in other cases (JSON parsing failure) we throw but don't report the error const error = Error('Failed to fetch non-refreshable line items'); - reportError(error, 'commercial'); + reportError(error, 'commercial', { + feature: 'commercial', + status: String(response.status), + }); throw error; }; diff --git a/src/utils/report-error.ts b/src/utils/report-error.ts index 5401b16c3..ba654be2c 100644 --- a/src/utils/report-error.ts +++ b/src/utils/report-error.ts @@ -12,7 +12,7 @@ const wrapWithErrorReporting = ( try { fn(event); } catch (e) { - reportError(e, JSON.stringify(tags)); + reportError(e, 'on-slot-render ', { tags: JSON.stringify(tags) }); } }; }; From 858b8ae8a21e25e8ac48c56c383e3a0a4c3bc81b Mon Sep 17 00:00:00 2001 From: Ravi <7014230+arelra@users.noreply.github.com> Date: Wed, 25 Sep 2024 13:37:56 +0100 Subject: [PATCH 18/24] Wrap reportError and check if defined. Handle unkown error types --- src/types/global.ts | 4 ++-- src/utils/report-error.ts | 23 +++++++++++++++++++---- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/types/global.ts b/src/types/global.ts index 1a4b6ac9f..9a87049e4 100644 --- a/src/types/global.ts +++ b/src/types/global.ts @@ -469,8 +469,8 @@ declare global { commercialTimer?: EventTimer; offlineCount?: number; modules: { - sentry: { - reportError: ( + sentry?: { + reportError?: ( error: unknown, feature: string, tags?: Record, diff --git a/src/utils/report-error.ts b/src/utils/report-error.ts index ba654be2c..02e86db14 100644 --- a/src/utils/report-error.ts +++ b/src/utils/report-error.ts @@ -1,6 +1,21 @@ -// This function is used to report errors to Sentry. -// This uses the `reportError` function from the `window.guardian.modules.sentry` object. -const reportError = window.guardian.modules.sentry.reportError; +/** + * This function is used to report errors to Sentry + * This uses the `reportError` function from the `window.guardian.modules.sentry` object set by DCR + */ +const reportError = ( + error: unknown, + feature: string, + tags: Record = {}, +) => { + try { + const err = error instanceof Error ? error : new Error(String(error)); + if (window.guardian.modules.sentry?.reportError) { + window.guardian.modules.sentry.reportError(err, feature, tags); + } + } catch (e) { + console.error('Error reporting error to Sentry', e, feature, tags); + } +}; type ErrorReportingFunction = (event: T) => void; @@ -12,7 +27,7 @@ const wrapWithErrorReporting = ( try { fn(event); } catch (e) { - reportError(e, 'on-slot-render ', { tags: JSON.stringify(tags) }); + reportError(e, 'commercial', tags); } }; }; From 5f6fb3e05aa3dc93f10ae3dfcefce8749ab35801 Mon Sep 17 00:00:00 2001 From: Ravi <7014230+arelra@users.noreply.github.com> Date: Mon, 23 Sep 2024 17:21:30 +0100 Subject: [PATCH 19/24] Fix tests --- src/lib/dfp/non-refreshable-line-items.spec.ts | 4 ++++ src/utils/robust.spec.ts | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/lib/dfp/non-refreshable-line-items.spec.ts b/src/lib/dfp/non-refreshable-line-items.spec.ts index a3eba848a..a5850aa1e 100644 --- a/src/lib/dfp/non-refreshable-line-items.spec.ts +++ b/src/lib/dfp/non-refreshable-line-items.spec.ts @@ -4,6 +4,10 @@ import { memoizedFetchNonRefreshableLineItemIds, } from './non-refreshable-line-items'; +jest.mock('utils/report-error', () => ({ + reportError: jest.fn(), +})); + describe('nonRefreshableLineItems', () => { it('returns the same IDs as the API', async () => { const response = { diff --git a/src/utils/robust.spec.ts b/src/utils/robust.spec.ts index 7087f5945..3d25aa97b 100644 --- a/src/utils/robust.spec.ts +++ b/src/utils/robust.spec.ts @@ -4,6 +4,10 @@ import type { Modules } from './robust'; const { catchAndLogError } = _; +jest.mock('utils/report-error', () => ({ + reportError: jest.fn(), +})); + let origConsoleWarn: typeof window.console.warn; beforeEach(() => { From 1489447aba3cc3752b5ef40a29b2b1ed5b55e54a Mon Sep 17 00:00:00 2001 From: Demetrios Skamiotis Date: Fri, 27 Sep 2024 15:45:13 +0100 Subject: [PATCH 20/24] re-add tags to arguments --- src/events/on-slot-render.ts | 1 - src/init/consented.ts | 19 ++++++++------- src/init/consented/dynamic-ad-slots.ts | 4 ++-- src/init/consented/prepare-permutive.spec.ts | 23 ++++--------------- src/init/consented/prepare-permutive.ts | 5 ++-- src/insert/fill-dynamic-advert-slot.ts | 2 +- .../dfp/non-refreshable-line-items.spec.ts | 2 +- src/lib/dfp/non-refreshable-line-items.ts | 1 - 8 files changed, 21 insertions(+), 36 deletions(-) diff --git a/src/events/on-slot-render.ts b/src/events/on-slot-render.ts index 0d50f8dca..35a404544 100644 --- a/src/events/on-slot-render.ts +++ b/src/events/on-slot-render.ts @@ -24,7 +24,6 @@ const reportEmptyResponse = ( new Error('dfp returned an empty ad response'), 'commercial', { - feature: 'commercial', adUnit: adUnitPath, adSlot: adSlotId, adKeywords, diff --git a/src/init/consented.ts b/src/init/consented.ts index 36757eddc..feb4c6855 100644 --- a/src/init/consented.ts +++ b/src/init/consented.ts @@ -120,15 +120,18 @@ const bootCommercial = async (): Promise => { // Init Commercial event timers EventTimer.init(); - catchErrorsWithContext([ + catchErrorsWithContext( [ - 'ga-user-timing-commercial-start', - function runTrackPerformance() { - EventTimer.get().mark('commercialStart'); - EventTimer.get().mark('commercialBootStart'); - }, + [ + 'ga-user-timing-commercial-start', + function runTrackPerformance() { + EventTimer.get().mark('commercialStart'); + EventTimer.get().mark('commercialBootStart'); + }, + ], ], - ]); + tags, + ); // Stub the command queue // @ts-expect-error -- it’s a stub, not the whole Googletag object @@ -146,7 +149,7 @@ const bootCommercial = async (): Promise => { await Promise.all(promises).then(recordCommercialMetrics); } catch (error) { // report async errors in bootCommercial to Sentry with the commercial feature tag - reportError(error, 'commercial'); + reportError(error, 'commercial', tags); } }; diff --git a/src/init/consented/dynamic-ad-slots.ts b/src/init/consented/dynamic-ad-slots.ts index 4f7d06e03..3243f32de 100644 --- a/src/init/consented/dynamic-ad-slots.ts +++ b/src/init/consented/dynamic-ad-slots.ts @@ -23,11 +23,11 @@ const dynamicAdSlotModules: Modules = [ export const initDynamicAdSlots = async (): Promise => { await Promise.all( - dynamicAdSlotModules.map(async ([, init]) => { + dynamicAdSlotModules.map(async ([name, init]) => { try { await init(); } catch (error) { - reportError(error, 'commercial'); + reportError(error, 'commercial', { tag: name }); } }), ); diff --git a/src/init/consented/prepare-permutive.spec.ts b/src/init/consented/prepare-permutive.spec.ts index 9dc64d951..7304631c7 100644 --- a/src/init/consented/prepare-permutive.spec.ts +++ b/src/init/consented/prepare-permutive.spec.ts @@ -316,25 +316,11 @@ describe('Generating Permutive payload utils', () => { const validConfigForPayload = { page: { ...testPageConfig, section: 'uk' }, }; - - it('catches errors and calls the logger correctly when no global permutive', () => { - const logger = jest.fn(); - _.runPermutive( - { - page: testPageConfig, - }, - undefined, - logger, - ); - const err = (logger.mock.calls[0] as Error[])[0]; - expect(err).toBeInstanceOf(Error); - expect(err?.message).toBe('Global Permutive setup error'); - }); it('calls the permutive addon method with the correct payload', () => { const mockPermutive = { addon: jest.fn(), identify: jest.fn() }; const logger = jest.fn(); - _.runPermutive(validConfigForPayload, mockPermutive, logger); + _.runPermutive(validConfigForPayload, mockPermutive); expect(mockPermutive.addon).toHaveBeenCalledWith('web', { page: { content: expect.objectContaining({ section: 'uk' }) as { @@ -356,7 +342,7 @@ describe('Generating Permutive payload utils', () => { ...validConfigForPayload, }; - _.runPermutive(config, mockPermutive, logger); + _.runPermutive(config, mockPermutive); expect(mockPermutive.identify).toHaveBeenCalledWith([ { tag: 'ophan', id: bwid }, ]); @@ -364,14 +350,13 @@ describe('Generating Permutive payload utils', () => { }); it("calls permutive's identify before it calls addon, if the browser ID is present", () => { const mockPermutive = { addon: jest.fn(), identify: jest.fn() }; - const logger = jest.fn(); const bwid = '1234567890abcdef'; const config = { ophan: { browserId: bwid, pageViewId: 'pvid' }, ...validConfigForPayload, }; - _.runPermutive(config, mockPermutive, logger); + _.runPermutive(config, mockPermutive); const [identifyCallOrder] = mockPermutive.identify.mock.invocationCallOrder; const [addonCallOrder] = mockPermutive.addon.mock @@ -382,7 +367,7 @@ describe('Generating Permutive payload utils', () => { const mockPermutive = { addon: jest.fn(), identify: jest.fn() }; const logger = jest.fn(); - _.runPermutive(validConfigForPayload, mockPermutive, logger); + _.runPermutive(validConfigForPayload, mockPermutive); expect(mockPermutive.identify).not.toHaveBeenCalled(); expect(logger).not.toHaveBeenCalled(); }); diff --git a/src/init/consented/prepare-permutive.ts b/src/init/consented/prepare-permutive.ts index 3e65dfe60..2aabca614 100644 --- a/src/init/consented/prepare-permutive.ts +++ b/src/init/consented/prepare-permutive.ts @@ -140,7 +140,6 @@ const generatePermutiveIdentities = ( const runPermutive = ( pageConfig: PermutivePageConfig, permutiveGlobal: Permutive | undefined, - logger: typeof reportError, ): void => { try { if (!permutiveGlobal?.addon) { @@ -157,7 +156,7 @@ const runPermutive = ( page: payload, }); } catch (err) { - logger(err, 'commercial'); + reportError(err, 'commercial'); } }; @@ -237,7 +236,7 @@ export const initPermutive = (): Promise => { page: window.guardian.config.page, ophan: window.guardian.config.ophan, }; - runPermutive(permutiveConfig, window.permutive, reportError); + runPermutive(permutiveConfig, window.permutive); return Promise.resolve(); }; diff --git a/src/insert/fill-dynamic-advert-slot.ts b/src/insert/fill-dynamic-advert-slot.ts index 4f12bbb80..f8ff08b61 100644 --- a/src/insert/fill-dynamic-advert-slot.ts +++ b/src/insert/fill-dynamic-advert-slot.ts @@ -30,7 +30,7 @@ const fillDynamicAdSlot = ( const errorMessage = `Attempting to add slot with exisiting id ${adSlot.id}`; log('commercial', errorMessage); reportError(Error(errorMessage), 'commercial', { - adSlot: adSlot.id, + slotId: adSlot.id, }); return; diff --git a/src/lib/dfp/non-refreshable-line-items.spec.ts b/src/lib/dfp/non-refreshable-line-items.spec.ts index a5850aa1e..435a8c250 100644 --- a/src/lib/dfp/non-refreshable-line-items.spec.ts +++ b/src/lib/dfp/non-refreshable-line-items.spec.ts @@ -81,7 +81,7 @@ describe('nonRefreshableLineItems', () => { expect(reportError).toHaveBeenCalledWith( new Error('Failed to fetch non-refreshable line items'), 'commercial', - { feature: 'commercial', status: response.status }, + { status: response.status }, ); }); }); diff --git a/src/lib/dfp/non-refreshable-line-items.ts b/src/lib/dfp/non-refreshable-line-items.ts index 15aecd352..fb3e12c32 100644 --- a/src/lib/dfp/non-refreshable-line-items.ts +++ b/src/lib/dfp/non-refreshable-line-items.ts @@ -43,7 +43,6 @@ export const fetchNonRefreshableLineItemIds = async (): Promise => { // Note that in other cases (JSON parsing failure) we throw but don't report the error const error = Error('Failed to fetch non-refreshable line items'); reportError(error, 'commercial', { - feature: 'commercial', status: String(response.status), }); throw error; From da5a991e7257d71ad80a0b81ace80296b1eb362d Mon Sep 17 00:00:00 2001 From: Ravi <7014230+arelra@users.noreply.github.com> Date: Tue, 1 Oct 2024 18:59:55 +0100 Subject: [PATCH 21/24] Add prepare-permutive report error test --- src/init/consented/prepare-permutive.spec.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/init/consented/prepare-permutive.spec.ts b/src/init/consented/prepare-permutive.spec.ts index 7304631c7..16c24d3fb 100644 --- a/src/init/consented/prepare-permutive.spec.ts +++ b/src/init/consented/prepare-permutive.spec.ts @@ -316,6 +316,22 @@ describe('Generating Permutive payload utils', () => { const validConfigForPayload = { page: { ...testPageConfig, section: 'uk' }, }; + + it('catches errors and calls reportError correctly when no global permutive', () => { + const reportError = window.guardian.modules.sentry?.reportError; + _.runPermutive( + { + page: testPageConfig, + }, + undefined, + ); + const err = ( + (reportError as jest.Mock).mock.calls[0] as Error[] + )[0]; + expect(err).toBeInstanceOf(Error); + expect(err?.message).toBe('Global Permutive setup error'); + }); + it('calls the permutive addon method with the correct payload', () => { const mockPermutive = { addon: jest.fn(), identify: jest.fn() }; const logger = jest.fn(); From ab0a40f0611f056375da43d6e0c3f822d0af75d7 Mon Sep 17 00:00:00 2001 From: Ravi <7014230+arelra@users.noreply.github.com> Date: Tue, 1 Oct 2024 19:01:34 +0100 Subject: [PATCH 22/24] Minor test improvements to non-refreshable-line-items.spec and article.spec --- src/insert/spacefinder/article.spec.ts | 4 ++++ src/lib/dfp/non-refreshable-line-items.spec.ts | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/insert/spacefinder/article.spec.ts b/src/insert/spacefinder/article.spec.ts index 4f143847d..5e4b97e21 100644 --- a/src/insert/spacefinder/article.spec.ts +++ b/src/insert/spacefinder/article.spec.ts @@ -5,6 +5,7 @@ import { init } from './article'; jest.mock('utils/report-error', () => ({ reportError: jest.fn(), })); + jest.mock('lib/header-bidding/prebid/prebid', () => ({ requestBids: jest.fn(), })); @@ -12,14 +13,17 @@ jest.mock('lib/header-bidding/prebid/prebid', () => ({ jest.mock('insert/fill-dynamic-advert-slot', () => ({ fillDynamicAdSlot: jest.fn(), })); + jest.mock('lib/commercial-features', () => ({ commercialFeatures: {}, })); + jest.mock('insert/spacefinder/space-filler', () => ({ spaceFiller: { fillSpace: jest.fn(), }, })); + jest.mock('lib/config', () => ({ page: {}, get: () => false })); jest.mock('experiments/ab', () => ({ diff --git a/src/lib/dfp/non-refreshable-line-items.spec.ts b/src/lib/dfp/non-refreshable-line-items.spec.ts index 435a8c250..d984be8eb 100644 --- a/src/lib/dfp/non-refreshable-line-items.spec.ts +++ b/src/lib/dfp/non-refreshable-line-items.spec.ts @@ -66,7 +66,7 @@ describe('nonRefreshableLineItems', () => { it('returns undefined and reports error when the API call fails', async () => { const response = { ok: false, - status: '404', + status: 404, }; Object.defineProperty(window, 'fetch', { @@ -81,7 +81,7 @@ describe('nonRefreshableLineItems', () => { expect(reportError).toHaveBeenCalledWith( new Error('Failed to fetch non-refreshable line items'), 'commercial', - { status: response.status }, + { status: '404' }, ); }); }); From 869db072d75f14752a9a527e63bf815d91693ef5 Mon Sep 17 00:00:00 2001 From: Ravi <7014230+arelra@users.noreply.github.com> Date: Tue, 1 Oct 2024 19:29:07 +0100 Subject: [PATCH 23/24] Add tests for report-error.spec --- src/utils/report-error.spec.ts | 25 ++++++++++++------------- src/utils/report-error.ts | 2 +- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts index 599a3950e..95be0f49c 100644 --- a/src/utils/report-error.spec.ts +++ b/src/utils/report-error.spec.ts @@ -1,21 +1,20 @@ import { reportError } from './report-error'; describe('report-error', () => { - beforeEach(() => { - jest.spyOn(global.Math, 'random').mockReturnValue(0.5); - }); + const error = new Error('Something broke'); + const tags = { test: 'testValue' }; - afterEach(() => { - jest.spyOn(global.Math, 'random').mockRestore(); - jest.resetAllMocks(); + test('Calls window.guardian.modules.sentry.reportError', () => { + reportError(error, 'commercial', tags); + expect( + window.guardian.modules.sentry?.reportError as jest.Mock, + ).toHaveBeenCalledWith(error, 'commercial', tags); }); - const error = new Error('Something broke.'); - const tags = { test: 'testValue' }; - - test('Does NOT throw an error', () => { - expect(() => { - reportError(error, 'commercial', tags); - }).not.toThrowError(error); + test('Converts error if of an unknown type', () => { + reportError('unknown', 'commercial', tags); + expect( + window.guardian.modules.sentry?.reportError as jest.Mock, + ).toHaveBeenCalledWith(expect.any(Error), 'commercial', tags); }); }); diff --git a/src/utils/report-error.ts b/src/utils/report-error.ts index 02e86db14..f2064c9f7 100644 --- a/src/utils/report-error.ts +++ b/src/utils/report-error.ts @@ -1,6 +1,6 @@ /** * This function is used to report errors to Sentry - * This uses the `reportError` function from the `window.guardian.modules.sentry` object set by DCR + * It uses the function `window.guardian.modules.sentry.reportError` set by DCR */ const reportError = ( error: unknown, From eba158e6f15993d32a26e4a01971245aa0fef2d5 Mon Sep 17 00:00:00 2001 From: Ravi <7014230+arelra@users.noreply.github.com> Date: Tue, 1 Oct 2024 20:10:25 +0100 Subject: [PATCH 24/24] Refactor robust and robust.spec --- src/core/messenger.spec.ts | 1 - src/init/consented.ts | 7 ++-- src/utils/report-error.spec.ts | 2 +- src/utils/robust.spec.ts | 64 ++++++++++++++-------------------- src/utils/robust.ts | 61 ++++++++++---------------------- 5 files changed, 49 insertions(+), 86 deletions(-) diff --git a/src/core/messenger.spec.ts b/src/core/messenger.spec.ts index f0f5f7fa4..09a756ce9 100644 --- a/src/core/messenger.spec.ts +++ b/src/core/messenger.spec.ts @@ -250,7 +250,6 @@ describe('Cross-frame messenger', () => { source: 'saucy', }) .then(() => { - console.log(postMessage); expect(postMessage).toHaveBeenCalledWith( { error: null, diff --git a/src/init/consented.ts b/src/init/consented.ts index feb4c6855..21d7f874c 100644 --- a/src/init/consented.ts +++ b/src/init/consented.ts @@ -20,7 +20,7 @@ import { init as prepareAdVerification } from 'lib/ad-verification/prepare-ad-ve import { commercialFeatures } from 'lib/commercial-features'; import { adSlotIdPrefix } from 'lib/dfp/dfp-env-globals'; import { reportError } from 'utils/report-error'; -import { catchErrorsWithContext } from 'utils/robust'; +import { catchErrorsAndReport } from 'utils/robust'; import { initDfpListeners } from './consented/dfp-listeners'; import { initDynamicAdSlots } from './consented/dynamic-ad-slots'; import { init as initMessenger } from './consented/messenger'; @@ -28,7 +28,6 @@ import { init as initMessenger } from './consented/messenger'; type Modules = Array<[`${string}-${string}`, () => Promise]>; const tags: Record = { - feature: 'commercial', bundle: 'standalone', }; // modules necessary to load the first ads on the page @@ -72,7 +71,7 @@ const loadModules = (modules: Modules, eventName: string) => { modules.forEach((module) => { const [moduleName, moduleInit] = module; - catchErrorsWithContext( + catchErrorsAndReport( [ [ moduleName, @@ -120,7 +119,7 @@ const bootCommercial = async (): Promise => { // Init Commercial event timers EventTimer.init(); - catchErrorsWithContext( + catchErrorsAndReport( [ [ 'ga-user-timing-commercial-start', diff --git a/src/utils/report-error.spec.ts b/src/utils/report-error.spec.ts index 95be0f49c..be92af6d1 100644 --- a/src/utils/report-error.spec.ts +++ b/src/utils/report-error.spec.ts @@ -1,7 +1,7 @@ import { reportError } from './report-error'; describe('report-error', () => { - const error = new Error('Something broke'); + const error = new Error('Deliberate test error'); const tags = { test: 'testValue' }; test('Calls window.guardian.modules.sentry.reportError', () => { diff --git a/src/utils/robust.spec.ts b/src/utils/robust.spec.ts index 3d25aa97b..acda25057 100644 --- a/src/utils/robust.spec.ts +++ b/src/utils/robust.spec.ts @@ -1,70 +1,58 @@ import { reportError } from 'utils/report-error'; -import { _, catchErrorsWithContext } from './robust'; +import { catchErrorsAndReport } from './robust'; import type { Modules } from './robust'; -const { catchAndLogError } = _; - jest.mock('utils/report-error', () => ({ reportError: jest.fn(), })); -let origConsoleWarn: typeof window.console.warn; - beforeEach(() => { - jest.clearAllMocks(); - origConsoleWarn = window.console.warn; - window.console.warn = jest.fn(); + jest.spyOn(global.console, 'warn'); }); afterEach(() => { - window.console.warn = origConsoleWarn; + jest.spyOn(global.console, 'warn').mockRestore(); + jest.resetAllMocks(); }); describe('robust', () => { - const ERROR = new Error('Something broke.'); - const META = 'commercial'; - const TAGS = { - module: 'test', - }; - - const noError = () => true; + const ERROR = new Error('Deliberate test error'); + const tags = { tag: 'test' }; const throwError = () => { throw ERROR; }; - test('catchAndLogError()', () => { - expect(() => { - catchAndLogError('test', noError); - }).not.toThrowError(); - - expect(() => { - catchAndLogError('test', throwError); - }).not.toThrowError(ERROR); + test('catchErrorsAndReport with no errors', () => { + const runner = jest.fn(); - expect(window.console.warn).toHaveBeenCalledTimes(1); - }); + const modules = [ + ['test-1', runner], + ['test-2', runner], + ['test-3', runner], + ] as Modules; - test('catchAndLogError() - default reporter with no error', () => { - catchAndLogError('test', noError); + catchErrorsAndReport(modules); + expect(runner).toHaveBeenCalledTimes(modules.length); expect(reportError).not.toHaveBeenCalled(); }); - test('catchAndLogError() - default reporter with error', () => { - catchAndLogError('test', throwError); - expect(reportError).toHaveBeenCalledWith(ERROR, META, TAGS); - }); - - test('catchErrorsWithContext()', () => { + test('catchErrorsAndReport with one error', () => { const runner = jest.fn(); - const MODULES = [ + const modules = [ ['test-1', runner], - ['test-2', runner], + ['test-2', throwError], ['test-3', runner], ] as Modules; - catchErrorsWithContext(MODULES); - expect(runner).toHaveBeenCalledTimes(MODULES.length); + catchErrorsAndReport(modules, tags); + expect(runner).toHaveBeenCalledTimes(2); + expect(reportError).toHaveBeenCalledTimes(1); + expect(window.console.warn).toHaveBeenCalledTimes(1); + expect(reportError).toHaveBeenCalledWith(ERROR, 'commercial', { + tag: 'test', + module: 'test-2', + }); }); }); diff --git a/src/utils/robust.ts b/src/utils/robust.ts index 951db6c57..ff6c932f4 100644 --- a/src/utils/robust.ts +++ b/src/utils/robust.ts @@ -1,54 +1,31 @@ -/* -Swallows (and reports) exceptions. Designed to wrap around modules at the "bootstrap" level. -For example "comments throwing an exception should not stop auto refresh" -*/ - +/** + * Swallows and reports exceptions. Designed to wrap around modules at the "bootstrap" level. + */ import { reportError } from 'utils/report-error'; type ModuleFunction = () => void; type Module = [string, ModuleFunction]; type Modules = Module[]; -const catchErrors = (fn: ModuleFunction): Error | undefined => { - let error: Error | undefined; - - try { - fn(); - } catch (e) { - error = e instanceof Error ? e : new Error(String(e)); - } - - return error; -}; - -const logError = ( - moduleName: string, - error: Error, - tags?: Record, -): void => { - window.console.warn('Caught error.', error.stack); - reportError(error, 'commercial', { ...tags, module: moduleName }); -}; - -const catchAndLogError = ( - name: string, - fn: ModuleFunction, - tags?: Record, -): void => { - const error = catchErrors(fn); - - if (error) { - logError(name, error, tags); - } -}; - -const catchErrorsWithContext = ( +const catchErrorsAndReport = ( modules: Modules, tags?: Record, ): void => { - modules.forEach(([name, fn]) => catchAndLogError(name, fn, tags)); + modules.forEach(([name, fn]) => { + let error: Error | undefined; + + try { + fn(); + } catch (e) { + error = e instanceof Error ? e : new Error(String(e)); + } + + if (error) { + window.console.warn('Caught error.', error.stack); + reportError(error, 'commercial', { ...tags, module: name }); + } + }); }; -export { catchErrorsWithContext }; +export { catchErrorsAndReport }; export type { Modules }; -export const _ = { catchAndLogError };