Skip to content

Commit

Permalink
Centralise frontend error reporting (and suppress unactionable Sentry…
Browse files Browse the repository at this point in the history
… errors) (#3850)

* Centralise frontend network error reporting

* Add shim for tests

* Revert unnecessary change to `useSearch`

* Remove unnecessary errors plugin shim causing jest.mock to fail for feature flag json

See note regarding setup-after-env in this section of Jest documentation: https://jestjs.io/docs/jest-object\#jestmockmodulename-factory-options

* Fix jest types to match actual jest version

* Add unit tests for errors plugin

* Skip 429s and rearrange function for clarity

* WIP/ PR does not work due to analytics composable

* Consider SSR/CSR in test

* Remove stray console.error

* Add search route network error handling test

* Fix bad merge of pnpm lock
  • Loading branch information
sarayourfriend authored Mar 21, 2024
1 parent 1e15853 commit ee3caaa
Show file tree
Hide file tree
Showing 15 changed files with 500 additions and 93 deletions.
1 change: 1 addition & 0 deletions frontend/nuxt.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ const config: NuxtConfig = {
"~/plugins/polyfills.client.ts",
"~/plugins/sentry.ts",
"~/plugins/analytics.ts",
"~/plugins/errors.ts",
],
css: ["~/assets/fonts.css", "~/styles/tailwind.css", "~/styles/accent.css"],
head,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,14 @@ import { cyclicShift } from "~/utils/math"
import { keycodes } from "~/constants/key-codes"
import { useAnalytics } from "~/composables/use-analytics"
import { useDialogControl } from "~/composables/use-dialog-control"
import { useSearch } from "~/composables/use-search"
import { useMediaStore } from "~/stores/media"
import { useSearchStore } from "~/stores/search"
import { useHydrating } from "~/composables/use-hydrating"
import { useAnalytics } from "~/composables/use-analytics"
import VLogoButton from "~/components/VHeader/VLogoButton.vue"
import VInputModal from "~/components/VModal/VInputModal.vue"
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/composables/use-search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import { useMediaStore } from "~/stores/media"

import { useI18nResultsCount } from "~/composables/use-i18n-utilities"
import { useMatchSearchRoutes } from "~/composables/use-match-routes"
import type { EventName, Events } from "~/types/analytics"
import { useAnalytics } from "~/composables/use-analytics"

export const useSearch = (
sendCustomEvent: <T extends EventName>(name: T, payload: Events[T]) => void
sendCustomEvent: ReturnType<typeof useAnalytics>["sendCustomEvent"]
) => {
const mediaStore = useMediaStore()
const searchStore = useSearchStore()
Expand Down
147 changes: 147 additions & 0 deletions frontend/src/plugins/errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
import axios from "axios"
import { Plugin, Context } from "@nuxt/types"

import { ERR_UNKNOWN, ErrorCode, errorCodes } from "~/constants/errors"
import type { FetchingError, RequestKind } from "~/types/fetch-state"
import type { SupportedSearchType } from "~/constants/media"

const isValidErrorCode = (
code: string | undefined | null
): code is ErrorCode => {
if (!code) {
return false
}
return (errorCodes as readonly string[]).includes(code)
}

function isDetailedResponseData(data: unknown): data is { detail: string } {
return !!data && typeof data === "object" && "detail" in data
}

/**
* Normalize any error occurring during a network call.
*
* @param error - Any error arising during a network call
* @param searchType - The type of search selected when the error occurred
* @param requestKind - The kind of request the error occurred for
* @param details - Any additional details to attach to the error
* @returns Normalized error object
*/
export function normalizeFetchingError(
error: unknown,
searchType: SupportedSearchType,
requestKind: RequestKind,
details?: Record<string, string>
): FetchingError {
const fetchingError: FetchingError = {
requestKind,
details,
searchType,
code: ERR_UNKNOWN,
}

if (!axios.isAxiosError(error)) {
fetchingError.message = (error as Error).message
return fetchingError
}

// Otherwise, it's an AxiosError
if (isValidErrorCode(error.code)) {
fetchingError.code = error.code
}

if (error.response?.status) {
fetchingError.statusCode = error.response.status
}

const responseData = error?.response?.data

// Use the message returned by the API.
if (isDetailedResponseData(responseData)) {
fetchingError.message = responseData.detail as string
} else {
fetchingError.message = error.message
}

return fetchingError
}

/**
* Record network errors using the appropriate tool, as needed,
* based on response code, status, and request kind.
* @param error - The error to record
*/
function recordError(
context: Context,
originalError: unknown,
fetchingError: FetchingError
) {
if (fetchingError.statusCode === 429) {
// These are more readily monitored via the Cloudflare dashboard.
return
}

if (
fetchingError.requestKind === "single-result" &&
fetchingError.statusCode === 404
) {
/**
* Do not record 404s for single result requests because:
* 1. Plausible will already record them as resulting in a 404 page view
* 2. The Openverse API 404s on malformed identifiers, so there is no way
* to distinguish between truly not found works and bad requests from
* the client side.
* 3. There isn't much we can do other than monitor for an anomalously high
* number of 404 responses from the frontend server that could indicate a frontend
* implementation or configuration error suddenly causing malformed
* identifiers to be used. Neither Sentry nor Plausible are the right tool
* for that task. If the 404s are caused by an API issue, we'd see that in
* API response code monitoring, where we can more easily trace the cause
*/
return
}

if (process.client && fetchingError.code === "ERR_NETWORK") {
/**
* Record network errors in Plausible so that we can evaluate potential
* regional or device configuration issues, for which Sentry is not
* as good a tool. Additionally, the number of these events are trivial
* for Plausible, but do actually affect our Sentry quota enough that it
* is worth diverting them.
*/
context.$sendCustomEvent("NETWORK_ERROR", {
requestKind: fetchingError.requestKind,
searchType: fetchingError.searchType,
})
} else {
context.$sentry.captureException(originalError, {
extra: { fetchingError },
})
}
}

function createProcessFetchingError(
context: Context
): typeof normalizeFetchingError {
function processFetchingError(
...[originalError, ...args]: Parameters<typeof normalizeFetchingError>
) {
const fetchingError = normalizeFetchingError(originalError, ...args)
recordError(context, originalError, fetchingError)
return fetchingError
}

return processFetchingError
}

declare module "@nuxt/types" {
interface Context {
$processFetchingError: ReturnType<typeof createProcessFetchingError>
}
}

const plugin: Plugin = async (context, inject) => {
inject("processFetchingError", createProcessFetchingError(context))
}

export default plugin
16 changes: 9 additions & 7 deletions frontend/src/stores/media/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { defineStore } from "pinia"

import { warn } from "~/utils/console"
import { hash, rand as prng } from "~/utils/prng"
import { isRetriable, parseFetchingError } from "~/utils/errors"
import { isRetriable } from "~/utils/errors"
import type {
AudioDetail,
DetailFromMediaType,
Expand Down Expand Up @@ -499,15 +499,17 @@ export const useMediaStore = defineStore("media", {
})
return mediaCount
} catch (error: unknown) {
const errorData = parseFetchingError(error, mediaType, "search", {
searchTerm: queryParams.q ?? "",
})
const errorData = this.$nuxt.$processFetchingError(
error,
mediaType,
"search",
{
searchTerm: queryParams.q ?? "",
}
)

this._updateFetchState(mediaType, "end", errorData)

this.$nuxt.$sentry.captureException(error, {
extra: { errorData },
})
return null
}
},
Expand Down
12 changes: 8 additions & 4 deletions frontend/src/stores/media/related-media.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { defineStore } from "pinia"

import { parseFetchingError } from "~/utils/errors"
import { initServices } from "~/stores/media/services"

import type { FetchingError, FetchState } from "~/types/fetch-state"
Expand Down Expand Up @@ -59,9 +58,14 @@ export const useRelatedMediaStore = defineStore("related-media", {

return this.media.length
} catch (error) {
const errorData = parseFetchingError(error, mediaType, "related", {
id,
})
const errorData = this.$nuxt.$processFetchingError(
error,
mediaType,
"related",
{
id,
}
)

this._endFetching(errorData)
this.$nuxt.$sentry.captureException(error, { extra: { errorData } })
Expand Down
12 changes: 7 additions & 5 deletions frontend/src/stores/media/single-result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import type { SupportedMediaType } from "~/constants/media"
import { initServices } from "~/stores/media/services"
import { useMediaStore } from "~/stores/media/index"
import { useProviderStore } from "~/stores/provider"
import { parseFetchingError } from "~/utils/errors"

import type { FetchingError, FetchState } from "~/types/fetch-state"

Expand Down Expand Up @@ -168,11 +167,14 @@ export const useSingleResultStore = defineStore("single-result", {

return item as DetailFromMediaType<MediaType>
} catch (error) {
const errorData = parseFetchingError(error, type, "single-result", {
id,
})
const errorData = this.$nuxt.$processFetchingError(
error,
type,
"single-result",
{ id }
)
this._updateFetchState("end", errorData)
this.$nuxt.$sentry.captureException(error, { extra: { errorData } })

return null
}
},
Expand Down
8 changes: 5 additions & 3 deletions frontend/src/stores/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { ssrRef } from "@nuxtjs/composition-api"

import { capitalCase } from "~/utils/case"
import { env } from "~/utils/env"
import { parseFetchingError } from "~/utils/errors"
import {
AUDIO,
IMAGE,
Expand Down Expand Up @@ -153,12 +152,15 @@ export const useProviderStore = defineStore("provider", {
sortedProviders = sortProviders(res)
this._updateFetchState(mediaType, "end")
} catch (error: unknown) {
const errorData = parseFetchingError(error, mediaType, "provider")
const errorData = this.$nuxt.$processFetchingError(
error,
mediaType,
"provider"
)

// Fallback on existing providers if there was an error
sortedProviders = this.providers[mediaType]
this._updateFetchState(mediaType, "end", errorData)
this.$nuxt.$sentry.captureException(error, { extra: { errorData } })
} finally {
this.providers[mediaType] = sortedProviders
this.sourceNames[mediaType] = sortedProviders.map((p) => p.source_name)
Expand Down
17 changes: 17 additions & 0 deletions frontend/src/types/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import type { ReportReason } from "~/constants/content-report"
import type { FilterCategory } from "~/constants/filters"
import { ResultKind } from "~/types/result"

import { RequestKind } from "./fetch-state"

export type AudioInteraction = "play" | "pause" | "seek"
export type AudioInteractionData = Exclude<
Events["AUDIO_INTERACTION"],
Expand Down Expand Up @@ -352,6 +354,7 @@ export type Events = {
/** whether the switch was turned on or off */
checked: boolean
}

/**
* Description: The user flips the sidebar toggle to not blur sensitive
* content in the search results.
Expand All @@ -364,6 +367,7 @@ export type Events = {
/** whether the switch was turned on or off */
checked: boolean
}

/**
* Description: The user proceeds to see the sensitive content from the
* content safety wall.
Expand All @@ -379,6 +383,7 @@ export type Events = {
/** the reasons for why this result is considered sensitive */
sensitivities: string
}

/**
* Description: The user opts not to see the sensitive content and to go back
* to the search results from the content safety wall.
Expand All @@ -397,6 +402,7 @@ export type Events = {
/** the reasons for why this result is considered sensitive */
sensitivities: string
}

/**
* Description: The user opts to re-hide the sensitive content that has been
* unblurred and presented to them.
Expand All @@ -423,6 +429,17 @@ export type Events = {
/** The state of the tags after the user interaction. */
toState: "expanded" | "collapsed"
}
/**
* Description: Recorded when a network error occurs. Recorded in Plausible,
* rather than Sentry, because we never have sufficient information in Sentry
* to identify patterns that could be relevant, like regional issues.
*/
NETWORK_ERROR: {
/** The kind of request the network error occurred during */
requestKind: RequestKind
/** The search type when the network error occurred */
searchType: SupportedSearchType
}
}

/**
Expand Down
Loading

0 comments on commit ee3caaa

Please sign in to comment.