From d5fe2703a28c9e41f6673f7b66022624ac0eb68a Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 6 Mar 2023 20:16:56 +1100 Subject: [PATCH 1/4] Ensure we report errors even when unexpected this happen during reporting --- code/lib/core-server/src/withTelemetry.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/code/lib/core-server/src/withTelemetry.ts b/code/lib/core-server/src/withTelemetry.ts index bc43ca681184..4846643e280e 100644 --- a/code/lib/core-server/src/withTelemetry.ts +++ b/code/lib/core-server/src/withTelemetry.ts @@ -64,7 +64,12 @@ export async function sendTelemetryError( options: TelemetryOptions ) { try { - const errorLevel = await getErrorLevel(options); + let errorLevel = 'error'; + try { + errorLevel = await getErrorLevel(options); + } catch (err) { + // If this throws, eg. due to main.js breaking, we fall back to 'full' + } if (errorLevel !== 'none') { const precedingUpgrade = await getPrecedingUpgrade(); @@ -74,7 +79,7 @@ export async function sendTelemetryError( eventType, precedingUpgrade, error: errorLevel === 'full' ? error : undefined, - errorHash: oneWayHash(error.message), + errorHash: oneWayHash(error.message || 'No error message'), }, { immediate: true, From bb7676a0720c6f7ee9e7bf188972ecfb7fcdd0f6 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 6 Mar 2023 20:17:25 +1100 Subject: [PATCH 2/4] Use the empty string for the hash --- code/lib/core-server/src/withTelemetry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/lib/core-server/src/withTelemetry.ts b/code/lib/core-server/src/withTelemetry.ts index 4846643e280e..307b8ed8df77 100644 --- a/code/lib/core-server/src/withTelemetry.ts +++ b/code/lib/core-server/src/withTelemetry.ts @@ -79,7 +79,7 @@ export async function sendTelemetryError( eventType, precedingUpgrade, error: errorLevel === 'full' ? error : undefined, - errorHash: oneWayHash(error.message || 'No error message'), + errorHash: oneWayHash(error.message || ''), }, { immediate: true, From b125f358cfe900d0250bf92efe47f5e5d9a83742 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 6 Mar 2023 20:33:26 +1100 Subject: [PATCH 3/4] Fix test names + main.js failure test --- .../lib/core-server/src/withTelemetry.test.ts | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/code/lib/core-server/src/withTelemetry.test.ts b/code/lib/core-server/src/withTelemetry.test.ts index a63df263dea2..519de40c1458 100644 --- a/code/lib/core-server/src/withTelemetry.test.ts +++ b/code/lib/core-server/src/withTelemetry.test.ts @@ -68,12 +68,12 @@ describe('when command fails', () => { expect(telemetry).toHaveBeenCalledTimes(0); expect(telemetry).not.toHaveBeenCalledWith( 'error', - { eventType: 'dev', error }, + expect.objectContaining({}), expect.objectContaining({}) ); }); - it('does not send error message when crash reports are disabled', async () => { + it('does not send full error message when crash reports are disabled', async () => { jest.mocked(loadAllPresets).mockResolvedValueOnce({ apply: async () => ({ enableCrashReports: false } as any), }); @@ -106,7 +106,7 @@ describe('when command fails', () => { ); }); - it('does not send full error message when telemetry is disabled', async () => { + it('does not send any error message when telemetry is disabled', async () => { jest.mocked(loadAllPresets).mockResolvedValueOnce({ apply: async () => ({ disableTelemetry: true } as any), }); @@ -140,7 +140,7 @@ describe('when command fails', () => { ); }); - it('does not send error messages when disabled crash reports are cached', async () => { + it('does not send full error messages when disabled crash reports are cached', async () => { jest.mocked(loadAllPresets).mockResolvedValueOnce({ apply: async () => ({} as any), }); @@ -176,7 +176,7 @@ describe('when command fails', () => { ); }); - it('does not send error messages when disabled crash reports are prompted', async () => { + it('does not send full error messages when disabled crash reports are prompted', async () => { jest.mocked(loadAllPresets).mockResolvedValueOnce({ apply: async () => ({} as any), }); @@ -214,18 +214,19 @@ describe('when command fails', () => { ); }); - // if main.js has errors, we have no way to tell if they've disabled telemetry - it('does not send error messages when presets fail to evaluate', async () => { + // if main.js has errors, we have no way to tell if they've disabled error reporting, + // so we assume they have. + it('does not send full error messages when presets fail to evaluate', async () => { jest.mocked(loadAllPresets).mockRejectedValueOnce(error); await expect(async () => withTelemetry('dev', { cliOptions: {} as any, presetOptions: {} as any }, run) ).rejects.toThrow(error); - expect(telemetry).toHaveBeenCalledTimes(1); - expect(telemetry).not.toHaveBeenCalledWith( + expect(telemetry).toHaveBeenCalledTimes(2); + expect(telemetry).toHaveBeenCalledWith( 'error', - { eventType: 'dev', error }, + { eventType: 'dev' }, expect.objectContaining({}) ); }); From a140e6feebf57882b9aaeca00aeaae81905c375b Mon Sep 17 00:00:00 2001 From: Michael Shilman Date: Mon, 6 Mar 2023 20:53:23 +0800 Subject: [PATCH 4/4] Update code/lib/core-server/src/withTelemetry.ts --- code/lib/core-server/src/withTelemetry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/lib/core-server/src/withTelemetry.ts b/code/lib/core-server/src/withTelemetry.ts index 307b8ed8df77..d6e72c7fe418 100644 --- a/code/lib/core-server/src/withTelemetry.ts +++ b/code/lib/core-server/src/withTelemetry.ts @@ -68,7 +68,7 @@ export async function sendTelemetryError( try { errorLevel = await getErrorLevel(options); } catch (err) { - // If this throws, eg. due to main.js breaking, we fall back to 'full' + // If this throws, eg. due to main.js breaking, we fall back to 'error' } if (errorLevel !== 'none') { const precedingUpgrade = await getPrecedingUpgrade();