From 31395122b1c220af87e188dc910a047d733e268b Mon Sep 17 00:00:00 2001 From: Christopher Radek Date: Mon, 27 Jun 2022 12:07:39 -0700 Subject: [PATCH 1/7] add tests to ensure plugin errors do not bubble to analytics API calls --- .../analytics/__tests__/integration.test.ts | 88 ++++++++++++++++++ .../core/analytics/__tests__/test-plugins.ts | 89 +++++++++++++++++++ 2 files changed, 177 insertions(+) create mode 100644 packages/browser/src/core/analytics/__tests__/integration.test.ts create mode 100644 packages/browser/src/core/analytics/__tests__/test-plugins.ts diff --git a/packages/browser/src/core/analytics/__tests__/integration.test.ts b/packages/browser/src/core/analytics/__tests__/integration.test.ts new file mode 100644 index 000000000..3c4d9bcb5 --- /dev/null +++ b/packages/browser/src/core/analytics/__tests__/integration.test.ts @@ -0,0 +1,88 @@ +import { PriorityQueue } from '../../../lib/priority-queue' +import { MiddlewareParams } from '../../../plugins/middleware' +import { Context } from '../../context' +import { EventQueue } from '../../queue/event-queue' +import { Analytics } from '../index' +import { + AfterPlugin, + BeforePlugin, + DestinationPlugin, + EnrichmentPlugin, +} from './test-plugins' + +describe('Analytics', () => { + describe('plugin error behavior', () => { + const retriesEnabledScenarios = [true, false] + retriesEnabledScenarios.forEach((retriesEnabled) => { + describe(`with retries ${ + retriesEnabled ? 'enabled' : 'disabled' + }`, () => { + let analytics: Analytics + + beforeEach(() => { + const queue = new EventQueue( + new PriorityQueue(retriesEnabled ? 3 : 1, []) + ) + analytics = new Analytics({ writeKey: 'writeKey' }, {}, queue) + }) + + // Indicates plugins should throw + const shouldThrow = true + const testPlugins = [ + new BeforePlugin({ shouldThrow }), + new EnrichmentPlugin({ shouldThrow }), + new DestinationPlugin({ shouldThrow }), + new AfterPlugin({ shouldThrow }), + ] + + testPlugins.forEach((plugin) => { + it(`"${plugin.type}" plugin errors should not throw (single dispatched event)`, async () => { + const trackSpy = jest.spyOn(plugin, 'track') + + await analytics.register(plugin) + await expect(analytics.track('test')).resolves.toBeInstanceOf( + Context + ) + + expect(trackSpy).toHaveBeenCalledTimes(1) + }) + + it(`"${plugin.type}" plugin errors should not throw (multiple dispatched events)`, async () => { + const trackSpy = jest.spyOn(plugin, 'track') + + await analytics.register(plugin) + await Promise.all([ + expect(analytics.track('test1')).resolves.toBeInstanceOf(Context), + expect(analytics.track('test2')).resolves.toBeInstanceOf(Context), + expect(analytics.track('test3')).resolves.toBeInstanceOf(Context), + ]) + + expect(trackSpy).toHaveBeenCalledTimes(3) + }) + }) + + it(`source middleware should not throw when "next" not called`, async () => { + const sourceMiddleware = jest.fn(() => {}) + + await analytics.addSourceMiddleware(sourceMiddleware) + + await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) + + expect(sourceMiddleware).toHaveBeenCalledTimes(1) + }) + + it(`source middleware errors should not throw`, async () => { + const sourceMiddleware = jest.fn(() => { + throw new Error('Source middleware error') + }) + + await analytics.addSourceMiddleware(sourceMiddleware) + + await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) + + expect(sourceMiddleware).toHaveBeenCalledTimes(1) + }) + }) + }) + }) +}) diff --git a/packages/browser/src/core/analytics/__tests__/test-plugins.ts b/packages/browser/src/core/analytics/__tests__/test-plugins.ts new file mode 100644 index 000000000..847db3ca9 --- /dev/null +++ b/packages/browser/src/core/analytics/__tests__/test-plugins.ts @@ -0,0 +1,89 @@ +import { Context, ContextCancelation, Plugin } from '../../../index' + +export interface BasePluginOptions { + shouldThrow?: boolean + shouldCancel?: boolean +} + +class BasePlugin implements Partial { + public version = '1.0.0' + private shouldCancel: boolean + private shouldThrow: boolean + + constructor({ + shouldCancel = false, + shouldThrow = false, + }: BasePluginOptions) { + this.shouldCancel = shouldCancel + this.shouldThrow = shouldThrow + } + + isLoaded() { + return true + } + + load() { + return Promise.resolve() + } + + alias(ctx: Context): Context { + return this.task(ctx) + } + + group(ctx: Context): Context { + return this.task(ctx) + } + + identify(ctx: Context): Context { + return this.task(ctx) + } + + page(ctx: Context): Context { + return this.task(ctx) + } + + screen(ctx: Context): Context { + return this.task(ctx) + } + + track(ctx: Context): Context { + return this.task(ctx) + } + + private task(ctx: Context): Context { + if (this.shouldCancel) { + ctx.cancel( + new ContextCancelation({ + retry: false, + }) + ) + } else if (this.shouldThrow) { + throw new Error(`Error thrown in task`) + } + return ctx + } +} + +export class BeforePlugin extends BasePlugin implements Plugin { + public name = 'Test Before Error' + public type = 'before' as const +} + +export class EnrichmentPlugin extends BasePlugin implements Plugin { + public name = 'Test Enrichment Error' + public type = 'enrichment' as const +} + +export class DestinationPlugin extends BasePlugin implements Plugin { + public name = 'Test Destination Error' + public type = 'destination' as const + + public ready() { + return Promise.resolve(true) + } +} + +export class AfterPlugin extends BasePlugin implements Plugin { + public name = 'Test After Error' + public type = 'after' as const +} From 7ebee3b4493a611fc414c6d298ada0bb6d1cb21d Mon Sep 17 00:00:00 2001 From: Christopher Radek Date: Mon, 27 Jun 2022 12:20:56 -0700 Subject: [PATCH 2/7] fix vscode launch configurations --- .vscode/launch.json | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 34a49e714..a418f0046 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -6,9 +6,15 @@ { "type": "node", "request": "launch", - "name": "Jest Current File", + "name": "(packages/browser) Jest Current File", "program": "${workspaceFolder}/node_modules/.bin/jest", - "args": ["--testTimeout=100000", "--runTestsByPath", "${relativeFile}"], + "args": [ + "--testTimeout=100000", + "-c", + "${workspaceFolder}/packages/browser/jest.config.js", + "--runTestsByPath", + "${relativeFile}" + ], "console": "integratedTerminal", "internalConsoleOptions": "neverOpen", "skipFiles": [ @@ -18,9 +24,13 @@ }, { "type": "node", - "name": "Jest All", + "name": "(packages/browser) Jest All", "request": "launch", - "args": ["--runInBand"], + "args": [ + "--runInBand", + "-c", + "${workspaceFolder}/packages/browser/jest.config.js" + ], "cwd": "${workspaceFolder}", "console": "integratedTerminal", "internalConsoleOptions": "neverOpen", From b225844662f6c62952b986d7ea0caf120f1fd643 Mon Sep 17 00:00:00 2001 From: Christopher Radek Date: Mon, 27 Jun 2022 15:44:59 -0700 Subject: [PATCH 3/7] add ContextCancelation retry tests and improve plugin error behavior integration tests --- .../analytics/__tests__/integration.test.ts | 71 +++++++++++++++++-- .../core/queue/__tests__/event-queue.test.ts | 40 +++++++++-- 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/packages/browser/src/core/analytics/__tests__/integration.test.ts b/packages/browser/src/core/analytics/__tests__/integration.test.ts index 3c4d9bcb5..49bad1422 100644 --- a/packages/browser/src/core/analytics/__tests__/integration.test.ts +++ b/packages/browser/src/core/analytics/__tests__/integration.test.ts @@ -18,23 +18,45 @@ describe('Analytics', () => { retriesEnabled ? 'enabled' : 'disabled' }`, () => { let analytics: Analytics + const attemptCount = retriesEnabled ? 2 : 1 beforeEach(() => { - const queue = new EventQueue( - new PriorityQueue(retriesEnabled ? 3 : 1, []) - ) + const queue = new EventQueue(new PriorityQueue(attemptCount, [])) analytics = new Analytics({ writeKey: 'writeKey' }, {}, queue) }) // Indicates plugins should throw const shouldThrow = true + + it(`"before" plugin errors should not throw (single dispatched event)`, async () => { + const plugin = new BeforePlugin({ shouldThrow }) + const trackSpy = jest.spyOn(plugin, 'track') + + await analytics.register(plugin) + await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) + + expect(trackSpy).toHaveBeenCalledTimes(attemptCount) + }) + + it(`"before" plugin errors should not throw (multiple dispatched events)`, async () => { + const plugin = new BeforePlugin({ shouldThrow }) + const trackSpy = jest.spyOn(plugin, 'track') + + await analytics.register(plugin) + await Promise.all([ + expect(analytics.track('test1')).resolves.toBeInstanceOf(Context), + expect(analytics.track('test2')).resolves.toBeInstanceOf(Context), + expect(analytics.track('test3')).resolves.toBeInstanceOf(Context), + ]) + + expect(trackSpy).toHaveBeenCalledTimes(3 * attemptCount) + }) + const testPlugins = [ - new BeforePlugin({ shouldThrow }), new EnrichmentPlugin({ shouldThrow }), new DestinationPlugin({ shouldThrow }), new AfterPlugin({ shouldThrow }), ] - testPlugins.forEach((plugin) => { it(`"${plugin.type}" plugin errors should not throw (single dispatched event)`, async () => { const trackSpy = jest.spyOn(plugin, 'track') @@ -61,14 +83,49 @@ describe('Analytics', () => { }) }) + it('"before" plugin supports cancelation (single dispatched event)', async () => { + const plugin = new BeforePlugin({ shouldCancel: true }) + const trackSpy = jest.spyOn(plugin, 'track') + + await analytics.register(plugin) + await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) + + expect(trackSpy).toHaveBeenCalledTimes(1) + }) + + it('"before" plugin supports cancelation (multiple dispatched events)', async () => { + const plugin = new BeforePlugin({ shouldCancel: true }) + const trackSpy = jest.spyOn(plugin, 'track') + + await analytics.register(plugin) + await Promise.all([ + expect(analytics.track('test1')).resolves.toBeInstanceOf(Context), + expect(analytics.track('test2')).resolves.toBeInstanceOf(Context), + expect(analytics.track('test3')).resolves.toBeInstanceOf(Context), + ]) + + expect(trackSpy).toHaveBeenCalledTimes(3) + }) + it(`source middleware should not throw when "next" not called`, async () => { const sourceMiddleware = jest.fn(() => {}) await analytics.addSourceMiddleware(sourceMiddleware) - await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) + const context = await analytics.track('test') expect(sourceMiddleware).toHaveBeenCalledTimes(1) + expect(context).toBeInstanceOf(Context) + expect( + context.logs().find(({ message }) => { + return message === 'Failed to deliver' + }) + ).toBeDefined() + expect( + context.stats.metrics.find(({ metric }) => { + return metric === 'delivery_failed' + }) + ).toBeDefined() }) it(`source middleware errors should not throw`, async () => { @@ -80,7 +137,7 @@ describe('Analytics', () => { await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) - expect(sourceMiddleware).toHaveBeenCalledTimes(1) + expect(sourceMiddleware).toHaveBeenCalledTimes(1 * attemptCount) }) }) }) diff --git a/packages/browser/src/core/queue/__tests__/event-queue.test.ts b/packages/browser/src/core/queue/__tests__/event-queue.test.ts index 31316073a..e1096578a 100644 --- a/packages/browser/src/core/queue/__tests__/event-queue.test.ts +++ b/packages/browser/src/core/queue/__tests__/event-queue.test.ts @@ -226,20 +226,48 @@ describe('Flushing', () => { ajs ) - eq.dispatch(fruitBasket) - eq.dispatch(basketView) - eq.dispatch(shopper) + const dispatches = [ + eq.dispatch(fruitBasket), + eq.dispatch(basketView), + eq.dispatch(shopper), + ] expect(eq.queue.length).toBe(3) - const flushed = await flushAll(eq) + const flushed = await Promise.all(dispatches) + // delivered both basket and shopper - expect(flushed).toEqual([basketView, shopper]) + expect(flushed).toEqual([fruitBasket, basketView, shopper]) - // nothing to retry + // nothing was retried + expect(basketView.attempts).toEqual(1) + expect(shopper.attempts).toEqual(1) + expect(fruitBasket.attempts).toEqual(1) expect(eq.queue.length).toBe(0) }) + test('does not retry non retriable cancelations (dispatchSingle)', async () => { + const eq = new EventQueue() + + await eq.register( + Context.system(), + { + ...testPlugin, + track: async (ctx) => { + if (ctx === fruitBasket) { + throw new ContextCancelation({ retry: false, reason: 'Test!' }) + } + return ctx + }, + }, + ajs + ) + + const context = await eq.dispatchSingle(fruitBasket) + + expect(context.attempts).toEqual(1) + }) + test('retries retriable cancelations', async () => { // make sure all backoffs return immediatelly jest.spyOn(timer, 'backoff').mockImplementationOnce(() => 100) From 57f2737bfbdf44cdee2ea4ef4e397d1c79ae8433 Mon Sep 17 00:00:00 2001 From: Christopher Radek Date: Mon, 27 Jun 2022 15:48:30 -0700 Subject: [PATCH 4/7] add ContextCancelation support to before plugins and prevent bubbling before plugin errors to analytics API calls --- packages/browser/src/core/queue/delivery.ts | 15 +++++++-------- packages/browser/src/core/queue/event-queue.ts | 13 +++++-------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/packages/browser/src/core/queue/delivery.ts b/packages/browser/src/core/queue/delivery.ts index 60b25d8c6..11d3234e7 100644 --- a/packages/browser/src/core/queue/delivery.ts +++ b/packages/browser/src/core/queue/delivery.ts @@ -14,7 +14,7 @@ async function tryOperation( export function attempt( ctx: Context, plugin: Plugin -): Promise { +): Promise { ctx.log('debug', 'plugin', { plugin: plugin.name }) const start = new Date().getTime() @@ -43,7 +43,7 @@ export function attempt( error: err, }) - return + return err } ctx.log('error', 'plugin Error', { @@ -63,13 +63,12 @@ export function ensure( plugin: Plugin ): Promise { return attempt(ctx, plugin).then((newContext) => { - if (newContext === undefined || newContext instanceof Error) { - ctx.log('debug', 'Context canceled') - ctx.stats.increment('context_canceled') - ctx.cancel(newContext) - return undefined + if (newContext instanceof Context) { + return newContext } - return newContext + ctx.log('debug', 'Context canceled') + ctx.stats.increment('context_canceled') + ctx.cancel(newContext) }) } diff --git a/packages/browser/src/core/queue/event-queue.ts b/packages/browser/src/core/queue/event-queue.ts index 39de9deb7..5722711b5 100644 --- a/packages/browser/src/core/queue/event-queue.ts +++ b/packages/browser/src/core/queue/event-queue.ts @@ -86,14 +86,14 @@ export class EventQueue extends Emitter { } private async subscribeToDelivery(ctx: Context): Promise { - return new Promise((resolve, reject) => { + return new Promise((resolve) => { const onDeliver = (flushed: Context, delivered: boolean): void => { if (flushed.isSame(ctx)) { this.off('flush', onDeliver) if (delivered) { resolve(flushed) } else { - reject(flushed) + resolve(flushed) } } } @@ -116,7 +116,7 @@ export class EventQueue extends Emitter { const accepted = this.enqueuRetry(err, ctx) if (!accepted) { - throw err + return ctx } return this.subscribeToDelivery(ctx) @@ -250,16 +250,13 @@ export class EventQueue extends Emitter { for (const beforeWare of before) { const temp: Context | undefined = await ensure(ctx, beforeWare) - if (temp !== undefined) { + if (temp instanceof Context) { ctx = temp } } for (const enrichmentWare of enrichment) { - const temp: Context | Error | undefined = await attempt( - ctx, - enrichmentWare - ) + const temp = await attempt(ctx, enrichmentWare) if (temp instanceof Context) { ctx = temp } From 118f18e3da3770bb10ad40cbe00f4b459800a4d2 Mon Sep 17 00:00:00 2001 From: Christopher Radek Date: Tue, 28 Jun 2022 12:19:16 -0700 Subject: [PATCH 5/7] add Context.failedDelivery() to check if an event failed to be delivered --- .../analytics/__tests__/integration.test.ts | 116 +++++++++++++----- packages/browser/src/core/context/index.ts | 13 ++ .../browser/src/core/queue/event-queue.ts | 1 + 3 files changed, 99 insertions(+), 31 deletions(-) diff --git a/packages/browser/src/core/analytics/__tests__/integration.test.ts b/packages/browser/src/core/analytics/__tests__/integration.test.ts index 49bad1422..4e3e2fc16 100644 --- a/packages/browser/src/core/analytics/__tests__/integration.test.ts +++ b/packages/browser/src/core/analytics/__tests__/integration.test.ts @@ -33,9 +33,10 @@ describe('Analytics', () => { const trackSpy = jest.spyOn(plugin, 'track') await analytics.register(plugin) - await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) - + const context = await analytics.track('test') expect(trackSpy).toHaveBeenCalledTimes(attemptCount) + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeTruthy() }) it(`"before" plugin errors should not throw (multiple dispatched events)`, async () => { @@ -43,13 +44,32 @@ describe('Analytics', () => { const trackSpy = jest.spyOn(plugin, 'track') await analytics.register(plugin) - await Promise.all([ - expect(analytics.track('test1')).resolves.toBeInstanceOf(Context), - expect(analytics.track('test2')).resolves.toBeInstanceOf(Context), - expect(analytics.track('test3')).resolves.toBeInstanceOf(Context), + const contexts = await Promise.all([ + analytics.track('test1'), + analytics.track('test2'), + analytics.track('test3'), ]) - expect(trackSpy).toHaveBeenCalledTimes(3 * attemptCount) + + for (const context of contexts) { + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeTruthy() + } + }) + + it(`"before" plugin errors should not impact callbacks`, async () => { + const plugin = new BeforePlugin({ shouldThrow }) + const trackSpy = jest.spyOn(plugin, 'track') + + await analytics.register(plugin) + + const context = await new Promise((resolve) => { + void analytics.track('test', resolve) + }) + + expect(trackSpy).toHaveBeenCalledTimes(attemptCount) + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeTruthy() }) const testPlugins = [ @@ -62,24 +82,43 @@ describe('Analytics', () => { const trackSpy = jest.spyOn(plugin, 'track') await analytics.register(plugin) - await expect(analytics.track('test')).resolves.toBeInstanceOf( - Context - ) + const context = await analytics.track('test') expect(trackSpy).toHaveBeenCalledTimes(1) + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeFalsy() }) it(`"${plugin.type}" plugin errors should not throw (multiple dispatched events)`, async () => { const trackSpy = jest.spyOn(plugin, 'track') await analytics.register(plugin) - await Promise.all([ - expect(analytics.track('test1')).resolves.toBeInstanceOf(Context), - expect(analytics.track('test2')).resolves.toBeInstanceOf(Context), - expect(analytics.track('test3')).resolves.toBeInstanceOf(Context), + + const contexts = await Promise.all([ + analytics.track('test1'), + analytics.track('test2'), + analytics.track('test3'), ]) expect(trackSpy).toHaveBeenCalledTimes(3) + for (const context of contexts) { + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeFalsy() + } + }) + + it(`"${plugin.type}" plugin errors should not impact callbacks`, async () => { + const trackSpy = jest.spyOn(plugin, 'track') + + await analytics.register(plugin) + + const context = await new Promise((resolve) => { + void analytics.track('test', resolve) + }) + + expect(trackSpy).toHaveBeenCalledTimes(1) + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeFalsy() }) }) @@ -88,9 +127,11 @@ describe('Analytics', () => { const trackSpy = jest.spyOn(plugin, 'track') await analytics.register(plugin) - await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) + const context = await analytics.track('test') expect(trackSpy).toHaveBeenCalledTimes(1) + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeTruthy() }) it('"before" plugin supports cancelation (multiple dispatched events)', async () => { @@ -98,13 +139,17 @@ describe('Analytics', () => { const trackSpy = jest.spyOn(plugin, 'track') await analytics.register(plugin) - await Promise.all([ - expect(analytics.track('test1')).resolves.toBeInstanceOf(Context), - expect(analytics.track('test2')).resolves.toBeInstanceOf(Context), - expect(analytics.track('test3')).resolves.toBeInstanceOf(Context), + const contexts = await Promise.all([ + analytics.track('test1'), + analytics.track('test2'), + analytics.track('test3'), ]) - expect(trackSpy).toHaveBeenCalledTimes(3) + + for (const context of contexts) { + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeTruthy() + } }) it(`source middleware should not throw when "next" not called`, async () => { @@ -116,16 +161,7 @@ describe('Analytics', () => { expect(sourceMiddleware).toHaveBeenCalledTimes(1) expect(context).toBeInstanceOf(Context) - expect( - context.logs().find(({ message }) => { - return message === 'Failed to deliver' - }) - ).toBeDefined() - expect( - context.stats.metrics.find(({ metric }) => { - return metric === 'delivery_failed' - }) - ).toBeDefined() + expect(context.failedDelivery()).toBeTruthy() }) it(`source middleware errors should not throw`, async () => { @@ -135,9 +171,27 @@ describe('Analytics', () => { await analytics.addSourceMiddleware(sourceMiddleware) - await expect(analytics.track('test')).resolves.toBeInstanceOf(Context) + const context = await analytics.track('test') expect(sourceMiddleware).toHaveBeenCalledTimes(1 * attemptCount) + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeTruthy() + }) + + it(`source middleware errors should not impact callbacks`, async () => { + const sourceMiddleware = jest.fn(() => { + throw new Error('Source middleware error') + }) + + await analytics.addSourceMiddleware(sourceMiddleware) + + const context = await new Promise((resolve) => { + void analytics.track('test', resolve) + }) + + expect(sourceMiddleware).toHaveBeenCalledTimes(1 * attemptCount) + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeTruthy() }) }) }) diff --git a/packages/browser/src/core/context/index.ts b/packages/browser/src/core/context/index.ts index d7793b264..7c7bbbf90 100644 --- a/packages/browser/src/core/context/index.ts +++ b/packages/browser/src/core/context/index.ts @@ -24,6 +24,10 @@ interface CancelationOptions { type?: string } +export interface ContextFailedDelivery { + reason: unknown +} + export class ContextCancelation { retry: boolean type: string @@ -44,6 +48,7 @@ export class Context implements AbstractContext { public logger = new Logger() public stats: Stats private _id: string + private _failedDelivery?: ContextFailedDelivery constructor(event: SegmentEvent, id?: string) { this._attempts = 0 @@ -110,6 +115,14 @@ export class Context implements AbstractContext { return this._event } + public failedDelivery(): ContextFailedDelivery | undefined { + return this._failedDelivery + } + + public setFailedDelivery(options: ContextFailedDelivery) { + this._failedDelivery = options + } + public logs(): LogMessage[] { return this.logger.logs } diff --git a/packages/browser/src/core/queue/event-queue.ts b/packages/browser/src/core/queue/event-queue.ts index 5722711b5..63a0d4303 100644 --- a/packages/browser/src/core/queue/event-queue.ts +++ b/packages/browser/src/core/queue/event-queue.ts @@ -159,6 +159,7 @@ export class EventQueue extends Emitter { } catch (err) { ctx.log('error', 'Failed to deliver', err as object) ctx.stats.increment('delivery_failed') + ctx.setFailedDelivery({ reason: err }) throw err } } From 0c71171d5aeb03463e738edd5acfa7e7b7f77721 Mon Sep 17 00:00:00 2001 From: Christopher Radek Date: Thu, 30 Jun 2022 10:15:02 -0700 Subject: [PATCH 6/7] only set failed delivery on context on final attempt --- .../analytics/__tests__/integration.test.ts | 25 +++++++++++++++++++ .../browser/src/core/queue/event-queue.ts | 4 ++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/packages/browser/src/core/analytics/__tests__/integration.test.ts b/packages/browser/src/core/analytics/__tests__/integration.test.ts index 4e3e2fc16..03cb7a5c2 100644 --- a/packages/browser/src/core/analytics/__tests__/integration.test.ts +++ b/packages/browser/src/core/analytics/__tests__/integration.test.ts @@ -1,6 +1,7 @@ import { PriorityQueue } from '../../../lib/priority-queue' import { MiddlewareParams } from '../../../plugins/middleware' import { Context } from '../../context' +import { Plugin } from '../../plugin' import { EventQueue } from '../../queue/event-queue' import { Analytics } from '../index' import { @@ -72,6 +73,30 @@ describe('Analytics', () => { expect(context.failedDelivery()).toBeTruthy() }) + if (retriesEnabled) { + it(`will not mark delivery failed if retry succeeds`, async () => { + const plugin: Plugin = { + name: 'Test Retries Plugin', + type: 'before', + version: '1.0.0', + isLoaded: () => true, + load: () => Promise.resolve(), + track: jest.fn((ctx) => { + if (ctx.attempts < attemptCount) { + throw new Error(`Plugin failed!`) + } + return ctx + }), + } + await analytics.register(plugin) + + const context = await analytics.track('test') + expect(plugin.track).toHaveBeenCalledTimes(attemptCount) + expect(context).toBeInstanceOf(Context) + expect(context.failedDelivery()).toBeFalsy() + }) + } + const testPlugins = [ new EnrichmentPlugin({ shouldThrow }), new DestinationPlugin({ shouldThrow }), diff --git a/packages/browser/src/core/queue/event-queue.ts b/packages/browser/src/core/queue/event-queue.ts index 63a0d4303..be4b1c777 100644 --- a/packages/browser/src/core/queue/event-queue.ts +++ b/packages/browser/src/core/queue/event-queue.ts @@ -111,11 +111,13 @@ export class EventQueue extends Emitter { return this.deliver(ctx).catch((err) => { if (err instanceof ContextCancelation && err.retry === false) { + ctx.setFailedDelivery({ reason: err }) return ctx } const accepted = this.enqueuRetry(err, ctx) if (!accepted) { + ctx.setFailedDelivery({ reason: err }) return ctx } @@ -159,7 +161,6 @@ export class EventQueue extends Emitter { } catch (err) { ctx.log('error', 'Failed to deliver', err as object) ctx.stats.increment('delivery_failed') - ctx.setFailedDelivery({ reason: err }) throw err } } @@ -196,6 +197,7 @@ export class EventQueue extends Emitter { const accepted = this.enqueuRetry(err, ctx) if (!accepted) { + ctx.setFailedDelivery({ reason: err }) this.emit('flush', ctx, false) } From 62e445eb9a96329ecfa3830907af6849a27c66b1 Mon Sep 17 00:00:00 2001 From: Christopher Radek Date: Thu, 7 Jul 2022 13:11:54 -0700 Subject: [PATCH 7/7] add changeset --- .changeset/odd-flies-protect.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/odd-flies-protect.md diff --git a/.changeset/odd-flies-protect.md b/.changeset/odd-flies-protect.md new file mode 100644 index 000000000..3e1d3fbe1 --- /dev/null +++ b/.changeset/odd-flies-protect.md @@ -0,0 +1,5 @@ +--- +'@segment/analytics-next': minor +--- + +Adds `context.failedDelivery()` to improve detection of events that could not be delivered due to plugin errors.