From 0c4809031e8b73cc35d8f7a4176763ae3d9083bd Mon Sep 17 00:00:00 2001 From: duyhungtnn Date: Tue, 26 Nov 2024 08:43:02 +0700 Subject: [PATCH] fix: skip generating error events for unauthorized or forbidden errors (#188) --- e2e/events.spec.ts | 21 ++++----- src/internal/event/EventInteractor.ts | 13 +++++- .../evaluation/EvaluationInteractor.spec.ts | 19 ++++---- test/internal/event/EventInteractor.spec.ts | 43 ++++++++++++++++--- test/internal/remote/ApiClient.spec.ts | 12 +++--- 5 files changed, 70 insertions(+), 38 deletions(-) diff --git a/e2e/events.spec.ts b/e2e/events.spec.ts index bd94ded7..42085125 100644 --- a/e2e/events.spec.ts +++ b/e2e/events.spec.ts @@ -198,7 +198,7 @@ suite('e2e/events', () => { const events = component.dataModule.eventStorage().getAll() - expect(events).toHaveLength(1) + expect(events).toHaveLength(0) expect( events.some((e) => { return ( @@ -207,16 +207,14 @@ suite('e2e/events', () => { e.event.event.apiId === ApiId.GET_EVALUATIONS ) }), - ).toBe(true) + ).toBe(false) - await expect(() => client.flush()).rejects.toThrowError( - ForbiddenException, - ) + await client.flush() const events2 = component.dataModule.eventStorage().getAll() - // error from /register_events does not get stored - expect(events2).toHaveLength(1) + // no events should be stored + expect(events2).toHaveLength(0) destroyBKTClient() @@ -236,10 +234,9 @@ suite('e2e/events', () => { const component2 = getDefaultComponent(client) const events3 = component2.dataModule.eventStorage().getAll() - - // error from /register_events does not get stored - expect(events3).toHaveLength(3) - // ForbiddenError should still exist + // 2 events - latency and response size + expect(events3).toHaveLength(2) + // ForbiddenError should not exist expect( events.some((e) => { return ( @@ -248,7 +245,7 @@ suite('e2e/events', () => { e.event.event.apiId === ApiId.GET_EVALUATIONS ) }), - ).toBe(true) + ).toBe(false) await client2.flush() diff --git a/src/internal/event/EventInteractor.ts b/src/internal/event/EventInteractor.ts index 7c06ce7f..4110e5aa 100644 --- a/src/internal/event/EventInteractor.ts +++ b/src/internal/event/EventInteractor.ts @@ -3,7 +3,7 @@ import { Clock } from '../Clock' import { IdGenerator } from '../IdGenerator' import { Evaluation } from '../model/Evaluation' import { EventType, Event, MetricsEvent } from '../model/Event' -import { ApiId } from '../model/MetricsEventData' +import { ApiId, MetricsEventType } from '../model/MetricsEventData' import { User } from '../model/User' import { ApiClient } from '../remote/ApiClient' import { @@ -169,6 +169,17 @@ export class EventInteractor { } trackFailure(apiId: ApiId, featureTag: string, error: BKTException): void { + + if (error.type === MetricsEventType.UnauthorizedError) { + console.error('An unauthorized error occurred. Please check your API Key.') + return + } + + if (error.type === MetricsEventType.ForbiddenError) { + console.error('An forbidden error occurred. Please check your API Key.') + return + } + const metricsEvents: Event[] = [ { id: this.idGenerator.newId(), diff --git a/test/internal/evaluation/EvaluationInteractor.spec.ts b/test/internal/evaluation/EvaluationInteractor.spec.ts index 6da72e00..222ba57a 100644 --- a/test/internal/evaluation/EvaluationInteractor.spec.ts +++ b/test/internal/evaluation/EvaluationInteractor.spec.ts @@ -1,4 +1,4 @@ -import { HttpResponse, StrictRequest, http } from 'msw' +import { HttpResponse, http } from 'msw' import { SetupServer } from 'msw/node' import { expect, @@ -97,7 +97,7 @@ suite('internal/evaluation/EvaluationInteractor', () => { component.dataModule.evaluationStorage().getCurrentEvaluationsId(), ).toBeNull() - const mockListener = vi.fn<[], void>() + const mockListener = vi.fn() interactor.addUpdateListener(mockListener) const result = await interactor.fetch(user1) @@ -158,7 +158,7 @@ suite('internal/evaluation/EvaluationInteractor', () => { }, { once: true }), ) - const mockListener = vi.fn<[], void>() + const mockListener = vi.fn() interactor.addUpdateListener(mockListener) // initial request @@ -194,10 +194,7 @@ suite('internal/evaluation/EvaluationInteractor', () => { }) test('update with no change', async () => { - const requestInterceptor = vi.fn< - [StrictRequest], - void - >() + const requestInterceptor = vi.fn() server.use( http.post< @@ -233,7 +230,7 @@ suite('internal/evaluation/EvaluationInteractor', () => { }), ) - const mockListener = vi.fn<[], void>() + const mockListener = vi.fn() interactor.addUpdateListener(mockListener) const result1 = await interactor.fetch(user1) @@ -360,7 +357,7 @@ suite('internal/evaluation/EvaluationInteractor', () => { userAttributesUpdated: false, }) - const mockListener = vi.fn<[], void>() + const mockListener = vi.fn() interactor.addUpdateListener(mockListener) @@ -414,7 +411,7 @@ suite('internal/evaluation/EvaluationInteractor', () => { userAttributesUpdated: false, }) - const mockListener = vi.fn<[], void>() + const mockListener = vi.fn() interactor.addUpdateListener(mockListener) @@ -468,7 +465,7 @@ suite('internal/evaluation/EvaluationInteractor', () => { userAttributesUpdated: false, }) - const mockListener = vi.fn<[], void>() + const mockListener = vi.fn() interactor.addUpdateListener(mockListener) diff --git a/test/internal/event/EventInteractor.spec.ts b/test/internal/event/EventInteractor.spec.ts index 7fa94553..a2f5556c 100644 --- a/test/internal/event/EventInteractor.spec.ts +++ b/test/internal/event/EventInteractor.spec.ts @@ -15,10 +15,12 @@ import { import { BKTConfig, defineBKTConfig } from '../../../src/BKTConfig' import { BadRequestException, + ForbiddenException, InternalServerErrorException, NetworkException, RedirectRequestException, TimeoutException, + UnauthorizedException, UnknownException, } from '../../../src/BKTExceptions' import { DefaultComponent } from '../../../src/internal/di/Component' @@ -106,7 +108,7 @@ suite('internal/event/EventInteractor', () => { }) test('trackEvaluationEvent', () => { - const mockListener = vi.fn<[Event[]], void>() + const mockListener = vi.fn() interactor.setEventUpdateListener(mockListener) interactor.trackEvaluationEvent('feature_tag_value', user1, evaluation1) @@ -144,7 +146,7 @@ suite('internal/event/EventInteractor', () => { }) test('trackDefaultEvaluationEvent', () => { - const mockListener = vi.fn<[Event[]], void>() + const mockListener = vi.fn() interactor.setEventUpdateListener(mockListener) interactor.trackDefaultEvaluationEvent( @@ -186,7 +188,7 @@ suite('internal/event/EventInteractor', () => { }) test('trackGoalEvent', () => { - const mockListener = vi.fn<[Event[]], void>() + const mockListener = vi.fn() interactor.setEventUpdateListener(mockListener) interactor.trackGoalEvent('feature_tag_value', user1, 'goal_id_value', 0.5) @@ -220,7 +222,7 @@ suite('internal/event/EventInteractor', () => { }) test('trackSuccess', () => { - const mockListener = vi.fn<[Event[]], void>() + const mockListener = vi.fn() interactor.setEventUpdateListener(mockListener) interactor.trackSuccess(ApiId.GET_EVALUATION, 'feature_tag_value', 1, 723) @@ -308,7 +310,7 @@ suite('internal/event/EventInteractor', () => { extraLabels: { timeout: '1.5' }, }, ])('trackFailure: $errr -> type: $type', ({ error, type, extraLabels }) => { - const mockListener = vi.fn<[Event[]], void>() + const mockListener = vi.fn() interactor.setEventUpdateListener(mockListener) interactor.trackFailure(ApiId.GET_EVALUATION, 'feature_tag_value', error) @@ -402,6 +404,33 @@ suite('internal/event/EventInteractor', () => { ]) }) + test('Skip generating error events for unauthorized or forbidden errors', () => { + // trackFailure saves one Event in each call + interactor.trackFailure( + ApiId.GET_EVALUATIONS, + 'feature_tag_value', + new UnauthorizedException(), + ) + interactor.trackFailure( + ApiId.GET_EVALUATIONS, + 'feature_tag_value_1', + new ForbiddenException(), + ) + interactor.trackFailure( + ApiId.REGISTER_EVENTS, + 'feature_tag_value_1', + new InternalServerErrorException(), + ) + + const events = eventStorage + .getAll() + .map((e) => interactor.getMetricsEventUniqueKey(e.event as MetricsEvent)) + + expect(events).toStrictEqual([ + '3::type.googleapis.com/bucketeer.event.client.InternalServerErrorMetricsEvent', + ]) + }) + suite('sendEvents', () => { test('success', async () => { server.use( @@ -409,7 +438,7 @@ suite('internal/event/EventInteractor', () => { Record, RegisterEventsRequest, RegisterEventsResponse - >(`${config.apiEndpoint}/register_events`, async ({ request }) => { + >(`${config.apiEndpoint}/register_events`, async ({request}) => { const body = await request.json() expect(body.events).toHaveLength(3) expect(body.sourceId).toEqual(SourceID.JAVASCRIPT) @@ -452,7 +481,7 @@ suite('internal/event/EventInteractor', () => { Record, RegisterEventsRequest, RegisterEventsResponse - >(`${config.apiEndpoint}/register_events`, async ({ request }) => { + >(`${config.apiEndpoint}/register_events`, async ({request}) => { const body = await request.json() expect(body.events).toHaveLength(3) return HttpResponse.json({ diff --git a/test/internal/remote/ApiClient.spec.ts b/test/internal/remote/ApiClient.spec.ts index 60e7f473..bf8474d5 100644 --- a/test/internal/remote/ApiClient.spec.ts +++ b/test/internal/remote/ApiClient.spec.ts @@ -209,17 +209,14 @@ suite('internal/remote/ApiClient', () => { suite('registerEvents', () => { test('success', async () => { - const requestInterceptor = vi.fn< - [StrictRequest], - void - >() + const requestInterceptor = vi.fn() server.use( http.post< Record, RegisterEventsRequest, RegisterEventsResponse - >(`${endpoint}/register_events`, async ({ request }) => { + >(`${endpoint}/register_events`, async ({request}) => { requestInterceptor(request) return HttpResponse.json({ errors: { @@ -315,9 +312,10 @@ suite('internal/remote/ApiClient', () => { evaluationEvent1, metricsEvent1, ]) - + + assert(response.type === 'failure') expect(response.type).toBe('failure') - + const error = response.error expect(error.name).toBe('UnknownException')