Merge pull request #19537 from mozilla/pay-3202-cms-statsd-improve

feat(strapi): move statsd logging to strapi client

Author: StaberindeZA

libs/shared/cms/src/lib/product-configuration.manager.spec.ts

@@ -94,7 +94,6 @@ describe('productConfigurationManager', () => {
   beforeEach(async () => {
     const module = await Test.createTestingModule({
       providers: [
-        { provide: StatsDService, useValue: mockStatsd },
         MockStrapiClientConfigProvider,
         MockFirestoreProvider,
         MockStatsDProvider,

libs/shared/cms/src/lib/product-configuration.manager.ts

@@ -53,7 +53,7 @@ import {
   ServicesWithCapabilitiesResultUtil,
   servicesWithCapabilitiesQuery,
 } from './queries/services-with-capabilities';
-import { StrapiClient, StrapiClientEventResponse } from './strapi.client';
+import { StrapiClient, type StrapiClientEventResponse } from './strapi.client';
 import { DeepNonNullable } from './types';
 import {
   iapOfferingsByStoreIDsQuery,

libs/shared/cms/src/lib/strapi.client.spec.ts

@@ -61,7 +61,6 @@ jest.useFakeTimers();
 
 describe('StrapiClient', () => {
   let strapiClient: StrapiClient;
-  const onCallback = jest.fn();
 
   beforeEach(async () => {
     const module = await Test.createTestingModule({
@@ -73,11 +72,6 @@ describe('StrapiClient', () => {
     }).compile();
 
     strapiClient = module.get(StrapiClient);
-    strapiClient.on('response', onCallback);
-  });
-
-  afterEach(() => {
-    onCallback.mockClear();
   });
 
   describe('query', () => {
@@ -122,14 +116,6 @@ describe('StrapiClient', () => {
       await expect(() =>
         strapiClient.query(offeringQuery, queryArgs)
       ).rejects.toThrow(new StrapiQueryError(offeringQuery, queryArgs, error));
-
-      expect(onCallback).toHaveBeenCalledWith(
-        expect.objectContaining({
-          method: 'query',
-          cache: false,
-          error: expect.anything(),
-        })
-      );
     });
   });
 

libs/shared/cms/src/lib/strapi.client.ts

@@ -41,7 +41,7 @@ export interface StrapiClientEventResponse {
   requestEndTime: number;
   elapsed: number;
   cache: boolean;
-  cacheType: 'method' | 'memory' | 'stale' | 'fallback';
+  cacheType: 'method' | 'memory' | 'stale' | 'fallback' | 'fallbackFailed';
   query?: TypedDocumentNode;
   variables?: string;
   error?: Error;
@@ -141,6 +141,7 @@ export class StrapiClient {
             requestStartTime: startTime,
             requestEndTime: endTime,
             elapsed: endTime - startTime,
+            error: result === 'fallback' || result === 'fallbackFailed',
             cache: result === 'stale' || result === 'fallback',
             cacheType: result,
           });
@@ -155,26 +156,12 @@ export class StrapiClient {
     query: TypedDocumentNode<Result, Variables>,
     variables: Variables
   ): Promise<Result> {
-    const requestStartTime = Date.now();
-
     try {
       return await this.client.request<Result, any>({
         document: query,
         variables,
       });
     } catch (e) {
-      const requestEndTime = Date.now();
-      this.emitter.emit('response', {
-        method: 'query',
-        query,
-        variables: JSON.stringify(variables),
-        requestStartTime,
-        requestEndTime,
-        elapsed: requestEndTime - requestStartTime,
-        error: e,
-        cache: false,
-      });
-
       throw new StrapiQueryError(query, variables, e);
     }
   }

libs/shared/db/type-cacheable/src/lib/type-cachable-stale-while-revalidate-with-fallback.spec.ts

@@ -51,9 +51,8 @@ describe('StaleWhileRevalidateWithFallbackStrategy', () => {
     };
     await cacheClient.set(cacheKey, cacheValue);
 
-    const result = await staleWhileRevalidateWithFallbackStrategy.handle(
-      context
-    );
+    const result =
+      await staleWhileRevalidateWithFallbackStrategy.handle(context);
 
     expect(result).toEqual(cacheValue.value);
 
@@ -80,9 +79,8 @@ describe('StaleWhileRevalidateWithFallbackStrategy', () => {
       isCacheable: () => true,
     } satisfies CacheStrategyContext;
 
-    const result = await staleWhileRevalidateWithFallbackStrategy.handle(
-      context
-    );
+    const result =
+      await staleWhileRevalidateWithFallbackStrategy.handle(context);
 
     expect(result).toEqual(methodValue);
 
@@ -117,9 +115,8 @@ describe('StaleWhileRevalidateWithFallbackStrategy', () => {
     };
     await cacheClient.set(cacheKey, cacheValue);
 
-    const result = await staleWhileRevalidateWithFallbackStrategy.handle(
-      context
-    );
+    const result =
+      await staleWhileRevalidateWithFallbackStrategy.handle(context);
 
     expect(result).toEqual(cacheValue.value);
 
@@ -179,9 +176,8 @@ describe('StaleWhileRevalidateWithFallbackStrategy', () => {
       .spyOn(cacheClient, 'get')
       .mockRejectedValue(new Error('cache get error'));
 
-    const result = await staleWhileRevalidateWithFallbackStrategy.handle(
-      context
-    );
+    const result =
+      await staleWhileRevalidateWithFallbackStrategy.handle(context);
 
     expect(result).toEqual(methodValue);
 
@@ -216,9 +212,8 @@ describe('StaleWhileRevalidateWithFallbackStrategy', () => {
     };
     await cacheClient.set(cacheKey, cacheValue);
 
-    const result = await staleWhileRevalidateWithFallbackStrategy.handle(
-      context
-    );
+    const result =
+      await staleWhileRevalidateWithFallbackStrategy.handle(context);
 
     expect(result).toEqual(cacheValue.value);
 
@@ -252,7 +247,11 @@ describe('StaleWhileRevalidateWithFallbackStrategy', () => {
     ).rejects.toThrowError('method error');
 
     expect(targetMethod).toHaveBeenCalled();
-    expect(onRequestFinished).not.toHaveBeenCalled();
+    expect(onRequestFinished).toHaveBeenCalledWith(
+      expect.any(Number),
+      expect.any(Number),
+      'fallbackFailed'
+    );
     expect(onAsyncCacheWriteFailure).not.toHaveBeenCalled();
   });
 });

libs/shared/db/type-cacheable/src/lib/type-cachable-stale-while-revalidate-with-fallback.ts

@@ -19,7 +19,7 @@ interface CacheEntry {
  * - method: There was no fresh enough value in cache, so the method was called.
  * - fallback: The method failed, so a very old (older than stale) value was returned.
  */
-type CacheResult = 'stale' | 'method' | 'fallback';
+type CacheResult = 'stale' | 'method' | 'fallback' | 'fallbackFailed';
 
 export class StaleWhileRevalidateWithFallbackStrategy implements CacheStrategy {
   private pendingCacheRequestMap = new Map<
@@ -149,6 +149,7 @@ export class StaleWhileRevalidateWithFallbackStrategy implements CacheStrategy {
         this.onRequestFinshed?.(startTime, Date.now(), 'fallback');
         return oldResult.value;
       } else {
+        this.onRequestFinshed?.(startTime, Date.now(), 'fallbackFailed');
         throw err;
       }
     }