From acf0dcf1363750103e160c859823e6770789e9f7 Mon Sep 17 00:00:00 2001 From: Victoria A <52001888+adjeiv@users.noreply.github.com> Date: Thu, 8 Feb 2024 13:40:20 +0300 Subject: [PATCH 1/4] Add `SEARCH_RESPONSE_TIME` event Signed-off-by: Olga Bulat --- frontend/src/data/media-service.ts | 64 +++++++++++++++++++++++++++++- frontend/src/stores/media/index.ts | 16 +++++++- frontend/src/stores/search.ts | 4 +- frontend/src/types/analytics.ts | 29 ++++++++++++++ 4 files changed, 108 insertions(+), 5 deletions(-) diff --git a/frontend/src/data/media-service.ts b/frontend/src/data/media-service.ts index 1d56e69766d..36b1a718f8a 100644 --- a/frontend/src/data/media-service.ts +++ b/frontend/src/data/media-service.ts @@ -6,9 +6,14 @@ import type { import type { ApiService } from "~/data/api-service" import type { DetailFromMediaType, Media } from "~/types/media" import { AUDIO, type SupportedMediaType } from "~/constants/media" +import type { Events } from "~/types/analytics" import type { AxiosResponse } from "axios" +export type SearchTimeEvent = + | Events["AUDIO_SEARCH_RESPONSE_TIME"] + | Events["IMAGE_SEARCH_RESPONSE_TIME"] + export interface MediaResult< T extends Media | Media[] | Record, > { @@ -28,6 +33,53 @@ class MediaService { this.mediaType = mediaType } + /** + * Processes AxiosResponse from a search query to + * construct SEARCH_RESPONSE_TIME analytics event. + * @param response - Axios response + * @param requestDatetime - datetime before request was sent + */ + buildSearchTimeEvent( + response: AxiosResponse, + requestDatetime: Date + ): SearchTimeEvent | undefined { + const REQUIRED_HEADERS = ["date", "cf-cache-status", "cf-ray"] + + const responseHeaders = response.headers + if (!REQUIRED_HEADERS.every((header) => header in responseHeaders)) { + return + } + + const responseDatetime = new Date(responseHeaders["date"]) + if (responseDatetime < requestDatetime) { + // response returned was from the local cache + return + } + + const cfRayIATA = responseHeaders["cf-ray"].split("-")[1] + if (cfRayIATA === undefined) { + return + } + + const elapsedSeconds = Math.floor( + (responseDatetime.getTime() - requestDatetime.getTime()) / 1000 + ) + + const responseUrl = + response.request?.responseURL ?? response.request?.res?.responseUrl + if (!responseUrl) { + return + } + const url = new URL(responseUrl) + + return { + cfCacheStatus: String(responseHeaders["cf-cache-status"]), + cfRayIATA: String(cfRayIATA), + elapsedTime: elapsedSeconds, + queryString: url.search, + } + } + /** * Decodes the text data to avoid encoding problems. * Also, converts the results from an array of media @@ -54,17 +106,25 @@ class MediaService { */ async search( params: PaginatedSearchQuery | PaginatedCollectionQuery - ): Promise>> { + ): Promise<{ + searchTimeEvent: SearchTimeEvent | undefined + data: MediaResult> + }> { // Add the `peaks` param to all audio searches automatically if (this.mediaType === AUDIO) { params.peaks = "true" } + const requestDatetime = new Date() + const res = await this.apiService.query>( this.mediaType, params as unknown as Record ) - return this.transformResults(res.data) + + const searchTimeEvent = this.buildSearchTimeEvent(res, requestDatetime) + + return { searchTimeEvent, data: this.transformResults(res.data) } } /** diff --git a/frontend/src/stores/media/index.ts b/frontend/src/stores/media/index.ts index 2da62aad110..c6141b5c27c 100644 --- a/frontend/src/stores/media/index.ts +++ b/frontend/src/stores/media/index.ts @@ -27,6 +27,8 @@ import { deepFreeze } from "~/utils/deep-freeze" interface SearchFetchState extends Omit { hasStarted: boolean } +import type { EventName } from "~/types/analytics" +import type { SearchTimeEvent } from "~/data/media-service" export type MediaStoreResult = { count: number @@ -448,6 +450,17 @@ export const useMediaStore = defineStore("media", { this.currentPage = 0 }, + recordSearchTime( + event: SearchTimeEvent | undefined, + mediaType: SupportedMediaType + ) { + if (event) { + const searchEvent = + `${mediaType.toUpperCase()}_SEARCH_RESPONSE_TIME` as EventName + this.$nuxt.$sendCustomEvent(searchEvent, event) + } + }, + /** * @param mediaType - the mediaType to fetch (do not use 'All_media' here) * @param shouldPersistMedia - whether the existing media should be added to or replaced. @@ -470,7 +483,8 @@ export const useMediaStore = defineStore("media", { try { const accessToken = this.$nuxt.$openverseApiToken const service = initServices[mediaType](accessToken) - const data = await service.search(queryParams) + const { searchTimeEvent, data } = await service.search(queryParams) + this.recordSearchTime(searchTimeEvent, mediaType) const mediaCount = data.result_count let errorData: FetchingError | undefined /** diff --git a/frontend/src/stores/search.ts b/frontend/src/stores/search.ts index 6ba3460e6d3..9e8834ac333 100644 --- a/frontend/src/stores/search.ts +++ b/frontend/src/stores/search.ts @@ -121,10 +121,10 @@ export function buildCollectionQuery( const { collection, ...params } = collectionParams const query: PaginatedCollectionQuery = { + collection, + unstable__collection: collection, ...params, ...getSensitiveQuery("API"), - unstable__collection: collection, - collection, } if ("tag" in query) { query.unstable__tag = query.tag diff --git a/frontend/src/types/analytics.ts b/frontend/src/types/analytics.ts index 554c1e6a6f8..4a980395f15 100644 --- a/frontend/src/types/analytics.ts +++ b/frontend/src/types/analytics.ts @@ -435,6 +435,7 @@ export type Events = { /** the reasons for why this result is considered sensitive */ sensitivities: string } + /** * Description: The user expands collapsed tags or collapses the expanded ones. * @@ -457,6 +458,34 @@ export type Events = { /** The search type when the network error occurred */ searchType: SupportedSearchType } + + /** + * Description: Time client-side search responses. Gives us observability into + * real user experience of search timings. + * Questions: + * - How long does it take for the client to receive a response to search requests? + */ + IMAGE_SEARCH_RESPONSE_TIME: { + /** the Cloudflare cache status, denoting whether the request hit Cloudflare or went all the way to our servers */ + cfCacheStatus: string + /** the IATA location identifier as part of the `cf-ray` header, indicating the data centre the request passed through */ + cfRayIATA: string + /** how many seconds it took to receive a response for the request */ + elapsedTime: number + /** full query string */ + queryString: string + } + + AUDIO_SEARCH_RESPONSE_TIME: { + /** the Cloudflare cache status, denoting whether the request hit Cloudflare or went all the way to our servers */ + cfCacheStatus: string + /** the IATA location identifier as part of the `cf-ray` header, indicating the data centre the request passed through */ + cfRayIATA: string + /** how many seconds it took to receive a response for the request */ + elapsedTime: number + /** full query string */ + queryString: string + } } /** From a3c6bc58907efcdb3bba5cdb1be4674765dc28ab Mon Sep 17 00:00:00 2001 From: Olga Bulat Date: Fri, 5 Apr 2024 15:25:57 +0300 Subject: [PATCH 2/4] Add and update tests Signed-off-by: Olga Bulat --- .../unit/specs/data/media-service.spec.ts | 126 ++++++++++++++++++ .../unit/specs/stores/media-store.spec.js | 10 +- 2 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 frontend/test/unit/specs/data/media-service.spec.ts diff --git a/frontend/test/unit/specs/data/media-service.spec.ts b/frontend/test/unit/specs/data/media-service.spec.ts new file mode 100644 index 00000000000..1bbfa990089 --- /dev/null +++ b/frontend/test/unit/specs/data/media-service.spec.ts @@ -0,0 +1,126 @@ +import { mockCreateApiService } from "~~/test/unit/test-utils/api-service-mock" + +import { initServices } from "~/stores/media/services" + +const API_IMAGES_ENDPOINT = "images/" +const API_AUDIO_ENDPOINT = "audio/" +const BASE_URL = "https://www.mockapiservice.openverse.engineering/v1/" + +beforeAll(() => { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + jest.useFakeTimers("modern") + jest.setSystemTime(new Date("Tue, 17 Dec 2019 20:20:00 GMT")) +}) + +afterAll(() => { + jest.useRealTimers() +}) + +describe("Media Service search and recordSearchTime", () => { + it("should not send a SEARCH_RESPONSE_TIME analytics event if any required header is missing", async () => { + mockCreateApiService((axiosMockAdapter) => { + axiosMockAdapter.onGet().reply(200, {}) + }) + + const res = await initServices.image().search({}) + + expect(res.searchTimeEvent).not.toBeDefined() + }) + + it("should not send a SEARCH_RESPONSE_TIME analytics event if the response was locally cached", async () => { + mockCreateApiService((axiosMockAdapter) => { + axiosMockAdapter.onGet().reply(() => { + return [ + 200, + {}, + { + date: "Tue, 17 Dec 2019 19:00:00 GMT", + "cf-ray": "230b030023ae284c-SJC", + "cf-cache-status": "HIT", + }, + ] + }) + }) + + const res = await initServices.audio().search({}) + + expect(res.searchTimeEvent).not.toBeDefined() + }) + + it("should not send a SEARCH_RESPONSE_TIME analytics event if the cf-ray is malformed", async () => { + mockCreateApiService((axiosMockAdapter) => { + axiosMockAdapter.onGet().reply((config) => { + // force config.url so the responseURL is set in the AxiosRequest + config.url = BASE_URL + config.url + return [ + 200, + {}, + { + date: "Tue, 17 Dec 2019 20:30:00 GMT", + "cf-ray": "230b030023ae284c", + "cf-cache-status": "HIT", + }, + ] + }) + }) + + const res = await initServices.audio().search({}) + + expect(res.searchTimeEvent).not.toBeDefined() + }) + + it("should send SEARCH_RESPONSE_TIME analytics with correct parameters", async () => { + mockCreateApiService((axiosMockAdapter) => { + axiosMockAdapter + .onGet(API_IMAGES_ENDPOINT, { params: { q: "apple" } }) + .reply((config) => { + config.url = BASE_URL + config.url + "?q=apple" + return [ + 200, + {}, + { + date: "Tue, 17 Dec 2019 20:20:02 GMT", + "cf-ray": "230b030023ae2822-SJC", + "cf-cache-status": "HIT", + }, + ] + }) + + axiosMockAdapter + .onGet(API_AUDIO_ENDPOINT, { params: { q: "table", peaks: "true" } }) + .reply((config) => { + config.url = BASE_URL + config.url + "?q=table&peaks=true" + return [ + 200, + {}, + { + date: "Tue, 17 Dec 2019 20:20:03 GMT", + "cf-ray": "240b030b23ae2822-LHR", + "cf-cache-status": "MISS", + }, + ] + }) + }) + + const IMAGE_QUERY_PARAMS = { q: "apple" } + const imageRes = await initServices.image().search(IMAGE_QUERY_PARAMS) + + expect(imageRes.searchTimeEvent).toEqual({ + cfCacheStatus: "HIT", + cfRayIATA: "SJC", + elapsedTime: 2, + queryString: "?q=apple", + }) + + const AUDIO_QUERY_PARAMS = { q: "table" } + const audioRes = await initServices.audio().search(AUDIO_QUERY_PARAMS) + + expect(audioRes.searchTimeEvent).toEqual({ + cfCacheStatus: "MISS", + cfRayIATA: "LHR", + elapsedTime: 3, + queryString: "?q=table&peaks=true", + }) + }) +}) diff --git a/frontend/test/unit/specs/stores/media-store.spec.js b/frontend/test/unit/specs/stores/media-store.spec.js index 1e612faa378..cc6b35396a7 100644 --- a/frontend/test/unit/specs/stores/media-store.spec.js +++ b/frontend/test/unit/specs/stores/media-store.spec.js @@ -42,7 +42,10 @@ const searchResults = (mediaType) => ({ }) const mockImplementation = (mediaType) => () => - Promise.resolve({ ...searchResults(mediaType) }) + Promise.resolve({ + searchTimeEvent: undefined, + data: { ...searchResults(mediaType) }, + }) const mockSearchAudio = jest.fn().mockImplementation(mockImplementation(AUDIO)) const mockSearchImage = jest.fn().mockImplementation(mockImplementation(IMAGE)) const mockGetMediaDetail = jest.fn() @@ -329,7 +332,10 @@ describe("Media Store", () => { shouldPersistMedia: true, mediaType: IMAGE, } - const emptyResult = { result_count: 0, page_count: 0, results: [] } + const emptyResult = { + searchTimeEvent: undefined, + data: { result_count: 0, page_count: 0, results: [] }, + } mockSearchImage.mockResolvedValueOnce(emptyResult) await mediaStore.fetchSingleMediaType(params) From 398639ea52e895358c1f2fd2495dcf002d67185d Mon Sep 17 00:00:00 2001 From: Olga Bulat Date: Fri, 5 Apr 2024 16:08:38 +0300 Subject: [PATCH 3/4] Rename variables Signed-off-by: Olga Bulat --- frontend/src/data/media-service.ts | 16 ++++++++-------- frontend/src/stores/media/index.ts | 14 +++++++------- .../test/unit/specs/data/media-service.spec.ts | 10 +++++----- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/frontend/src/data/media-service.ts b/frontend/src/data/media-service.ts index 36b1a718f8a..568f9ec6272 100644 --- a/frontend/src/data/media-service.ts +++ b/frontend/src/data/media-service.ts @@ -10,7 +10,7 @@ import type { Events } from "~/types/analytics" import type { AxiosResponse } from "axios" -export type SearchTimeEvent = +export type SearchTimeEventPayload = | Events["AUDIO_SEARCH_RESPONSE_TIME"] | Events["IMAGE_SEARCH_RESPONSE_TIME"] @@ -34,15 +34,15 @@ class MediaService { } /** - * Processes AxiosResponse from a search query to - * construct SEARCH_RESPONSE_TIME analytics event. + * Processes AxiosResponse from a search query to construct + * SEARCH_RESPONSE_TIME analytics event payload. * @param response - Axios response * @param requestDatetime - datetime before request was sent */ - buildSearchTimeEvent( + buildEventPayload( response: AxiosResponse, requestDatetime: Date - ): SearchTimeEvent | undefined { + ): SearchTimeEventPayload | undefined { const REQUIRED_HEADERS = ["date", "cf-cache-status", "cf-ray"] const responseHeaders = response.headers @@ -107,7 +107,7 @@ class MediaService { async search( params: PaginatedSearchQuery | PaginatedCollectionQuery ): Promise<{ - searchTimeEvent: SearchTimeEvent | undefined + eventPayload: SearchTimeEventPayload | undefined data: MediaResult> }> { // Add the `peaks` param to all audio searches automatically @@ -122,9 +122,9 @@ class MediaService { params as unknown as Record ) - const searchTimeEvent = this.buildSearchTimeEvent(res, requestDatetime) + const eventPayload = this.buildEventPayload(res, requestDatetime) - return { searchTimeEvent, data: this.transformResults(res.data) } + return { eventPayload, data: this.transformResults(res.data) } } /** diff --git a/frontend/src/stores/media/index.ts b/frontend/src/stores/media/index.ts index c6141b5c27c..9b90d7d57a6 100644 --- a/frontend/src/stores/media/index.ts +++ b/frontend/src/stores/media/index.ts @@ -28,7 +28,7 @@ interface SearchFetchState extends Omit { hasStarted: boolean } import type { EventName } from "~/types/analytics" -import type { SearchTimeEvent } from "~/data/media-service" +import type { SearchTimeEventPayload } from "~/data/media-service" export type MediaStoreResult = { count: number @@ -451,13 +451,13 @@ export const useMediaStore = defineStore("media", { }, recordSearchTime( - event: SearchTimeEvent | undefined, + payload: SearchTimeEventPayload | undefined, mediaType: SupportedMediaType ) { - if (event) { - const searchEvent = + if (payload) { + const eventName = `${mediaType.toUpperCase()}_SEARCH_RESPONSE_TIME` as EventName - this.$nuxt.$sendCustomEvent(searchEvent, event) + this.$nuxt.$sendCustomEvent(eventName, payload) } }, @@ -483,8 +483,8 @@ export const useMediaStore = defineStore("media", { try { const accessToken = this.$nuxt.$openverseApiToken const service = initServices[mediaType](accessToken) - const { searchTimeEvent, data } = await service.search(queryParams) - this.recordSearchTime(searchTimeEvent, mediaType) + const { eventPayload, data } = await service.search(queryParams) + this.recordSearchTime(eventPayload, mediaType) const mediaCount = data.result_count let errorData: FetchingError | undefined /** diff --git a/frontend/test/unit/specs/data/media-service.spec.ts b/frontend/test/unit/specs/data/media-service.spec.ts index 1bbfa990089..f3e03355f2e 100644 --- a/frontend/test/unit/specs/data/media-service.spec.ts +++ b/frontend/test/unit/specs/data/media-service.spec.ts @@ -25,7 +25,7 @@ describe("Media Service search and recordSearchTime", () => { const res = await initServices.image().search({}) - expect(res.searchTimeEvent).not.toBeDefined() + expect(res.eventPayload).not.toBeDefined() }) it("should not send a SEARCH_RESPONSE_TIME analytics event if the response was locally cached", async () => { @@ -45,7 +45,7 @@ describe("Media Service search and recordSearchTime", () => { const res = await initServices.audio().search({}) - expect(res.searchTimeEvent).not.toBeDefined() + expect(res.eventPayload).not.toBeDefined() }) it("should not send a SEARCH_RESPONSE_TIME analytics event if the cf-ray is malformed", async () => { @@ -67,7 +67,7 @@ describe("Media Service search and recordSearchTime", () => { const res = await initServices.audio().search({}) - expect(res.searchTimeEvent).not.toBeDefined() + expect(res.eventPayload).not.toBeDefined() }) it("should send SEARCH_RESPONSE_TIME analytics with correct parameters", async () => { @@ -106,7 +106,7 @@ describe("Media Service search and recordSearchTime", () => { const IMAGE_QUERY_PARAMS = { q: "apple" } const imageRes = await initServices.image().search(IMAGE_QUERY_PARAMS) - expect(imageRes.searchTimeEvent).toEqual({ + expect(imageRes.eventPayload).toEqual({ cfCacheStatus: "HIT", cfRayIATA: "SJC", elapsedTime: 2, @@ -116,7 +116,7 @@ describe("Media Service search and recordSearchTime", () => { const AUDIO_QUERY_PARAMS = { q: "table" } const audioRes = await initServices.audio().search(AUDIO_QUERY_PARAMS) - expect(audioRes.searchTimeEvent).toEqual({ + expect(audioRes.eventPayload).toEqual({ cfCacheStatus: "MISS", cfRayIATA: "LHR", elapsedTime: 3, From ef4d26dd603e6d1fe1650fc882356b5366b7b09e Mon Sep 17 00:00:00 2001 From: Olga Bulat Date: Tue, 9 Apr 2024 07:09:46 +0300 Subject: [PATCH 4/4] Join the two events into one Signed-off-by: Olga Bulat --- frontend/src/data/media-service.ts | 5 ++--- frontend/src/stores/media/index.ts | 12 +++--------- frontend/src/types/analytics.ts | 16 ++++------------ .../test/unit/specs/data/media-service.spec.ts | 2 ++ 4 files changed, 11 insertions(+), 24 deletions(-) diff --git a/frontend/src/data/media-service.ts b/frontend/src/data/media-service.ts index 568f9ec6272..1a3e1a9d11b 100644 --- a/frontend/src/data/media-service.ts +++ b/frontend/src/data/media-service.ts @@ -10,9 +10,7 @@ import type { Events } from "~/types/analytics" import type { AxiosResponse } from "axios" -export type SearchTimeEventPayload = - | Events["AUDIO_SEARCH_RESPONSE_TIME"] - | Events["IMAGE_SEARCH_RESPONSE_TIME"] +export type SearchTimeEventPayload = Events["SEARCH_RESPONSE_TIME"] export interface MediaResult< T extends Media | Media[] | Record, @@ -77,6 +75,7 @@ class MediaService { cfRayIATA: String(cfRayIATA), elapsedTime: elapsedSeconds, queryString: url.search, + mediaType: this.mediaType, } } diff --git a/frontend/src/stores/media/index.ts b/frontend/src/stores/media/index.ts index 9b90d7d57a6..f9ae991d1c8 100644 --- a/frontend/src/stores/media/index.ts +++ b/frontend/src/stores/media/index.ts @@ -27,7 +27,6 @@ import { deepFreeze } from "~/utils/deep-freeze" interface SearchFetchState extends Omit { hasStarted: boolean } -import type { EventName } from "~/types/analytics" import type { SearchTimeEventPayload } from "~/data/media-service" export type MediaStoreResult = { @@ -450,14 +449,9 @@ export const useMediaStore = defineStore("media", { this.currentPage = 0 }, - recordSearchTime( - payload: SearchTimeEventPayload | undefined, - mediaType: SupportedMediaType - ) { + recordSearchTime(payload: SearchTimeEventPayload | undefined) { if (payload) { - const eventName = - `${mediaType.toUpperCase()}_SEARCH_RESPONSE_TIME` as EventName - this.$nuxt.$sendCustomEvent(eventName, payload) + this.$nuxt.$sendCustomEvent("SEARCH_RESPONSE_TIME", payload) } }, @@ -484,7 +478,7 @@ export const useMediaStore = defineStore("media", { const accessToken = this.$nuxt.$openverseApiToken const service = initServices[mediaType](accessToken) const { eventPayload, data } = await service.search(queryParams) - this.recordSearchTime(eventPayload, mediaType) + this.recordSearchTime(eventPayload) const mediaCount = data.result_count let errorData: FetchingError | undefined /** diff --git a/frontend/src/types/analytics.ts b/frontend/src/types/analytics.ts index 4a980395f15..b44a82e33be 100644 --- a/frontend/src/types/analytics.ts +++ b/frontend/src/types/analytics.ts @@ -1,6 +1,7 @@ import type { MediaType, SearchType, + SupportedMediaType, SupportedSearchType, } from "~/constants/media" import type { ReportReason } from "~/constants/content-report" @@ -465,18 +466,9 @@ export type Events = { * Questions: * - How long does it take for the client to receive a response to search requests? */ - IMAGE_SEARCH_RESPONSE_TIME: { - /** the Cloudflare cache status, denoting whether the request hit Cloudflare or went all the way to our servers */ - cfCacheStatus: string - /** the IATA location identifier as part of the `cf-ray` header, indicating the data centre the request passed through */ - cfRayIATA: string - /** how many seconds it took to receive a response for the request */ - elapsedTime: number - /** full query string */ - queryString: string - } - - AUDIO_SEARCH_RESPONSE_TIME: { + SEARCH_RESPONSE_TIME: { + /** the media type being searched */ + mediaType: SupportedMediaType /** the Cloudflare cache status, denoting whether the request hit Cloudflare or went all the way to our servers */ cfCacheStatus: string /** the IATA location identifier as part of the `cf-ray` header, indicating the data centre the request passed through */ diff --git a/frontend/test/unit/specs/data/media-service.spec.ts b/frontend/test/unit/specs/data/media-service.spec.ts index f3e03355f2e..c4f233301da 100644 --- a/frontend/test/unit/specs/data/media-service.spec.ts +++ b/frontend/test/unit/specs/data/media-service.spec.ts @@ -111,6 +111,7 @@ describe("Media Service search and recordSearchTime", () => { cfRayIATA: "SJC", elapsedTime: 2, queryString: "?q=apple", + mediaType: "image", }) const AUDIO_QUERY_PARAMS = { q: "table" } @@ -121,6 +122,7 @@ describe("Media Service search and recordSearchTime", () => { cfRayIATA: "LHR", elapsedTime: 3, queryString: "?q=table&peaks=true", + mediaType: "audio", }) }) })