Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Centralise frontend error reporting (and suppress unactionable Sentry errors) #3850

Merged
merged 12 commits into from Mar 21, 2024
Merged
1 change: 1 addition & 0 deletions frontend/nuxt.config.ts
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
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
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
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring needs to be updated.

*/
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* API response code monitoring, where we can more easily trace the cause
* 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 },
})
}
}
Comment on lines +116 to +121
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

``

Suggested change
} else {
context.$sentry.captureException(originalError, {
extra: { fetchingError },
})
}
}
return
}
// Send all other errors to Sentry
context.$sentry.captureException(originalError, {
extra: { fetchingError },
})
}

Style nit: perhaps this would make adding future additional cases less error-prone, or avoid confusion if folks think there's some special relationship between the process.client && fetchingError.code === "ERR_NETWORK" errors and Sentry specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth many times on this, and ultimately found both just as confusing, and went with a coin toss. I'll switch this, as any actual preference is better than my 50/50 non-choice!


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
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 ?? "",
}
Comment on lines +506 to +508
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{
searchTerm: queryParams.q ?? "",
}
{ searchTerm: queryParams.q ?? "" }

Nit: these seem more readable when on a single line

)

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
@@ -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,
}
Comment on lines +65 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{
id,
}
{ id }

Nit

)

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
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
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
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