Capturing Issue Description in Sentry

[ramadanomar]

Related to

Related to #1604 by @sarayourfriend

Description

Not sure on when how to reproduce this issue, but this should return the whole exception to provide more context.

[sarayourfriend]

Inside of Pinia stores you can access the pre-configured Sentry instance via this.$nuxt.$sentry. In this case I think it'd make sense to use captureException explicitly when an error is caught. For the case where we set the message to "Oops! Something went wrong", if we could capture the event with something more specific that would be great. Maybe captureMessage is better in that case. I'm not totally sure of the difference. Do you happen to know?

@obulat and @dhruvkb do y'all know when that "Oops! Something went wrong" clause happens and what it's meant to handle?

BTW, to test this, I threw an arbitrary error in fetchSingleMediaType before setMedia is called and then a breakpoint in my browser debugger on this line.

[ramadanomar]

Inside of Pinia stores you can access the pre-configured Sentry instance via this.$nuxt.$sentry.

I tried accesing it this way before but it keeps failing when i run pnpm test with the error "Cannot read properties of undefined (reading 'captureException')"

[sarayourfriend]

Ah! You've got to mock out context variables in tests. You can add $nuxt and the $sentry mock to this Pinia test utility we use:

openverse-frontend/test/unit/test-utils/pinia.js

Lines 4 to 9 in b863e3e

	export const createPinia = () =>
	pinia.createPinia().use(() => ({
	$nuxt: {
	$openverseApiToken: '',
	},
	}))

[ramadanomar]

if we could capture the event with something more specific that would be great. Maybe captureMessage is better in that case. I'm not totally sure of the difference.

According to sentry docs, we can pass a non-error object to captureException tho it will lack the context. You can pass an Error object to captureException() to get it captured as event. It's also possible to pass non-Error objects and strings, but be aware that the resulting events in Sentry may be missing a stacktrace.

Capture event triggers a manual event, that means we recieve all the breadcumbs that were collected until that moment. We can also include a dump of the application if we are sure we won't surpass the event payload max size

[sarayourfriend]

For now I think better not to dump the application. If we need it to debug a particular error that sounds like a great feature though, thanks for sharing it.

Do whatever you think is best vis a vis which Sentry function to use 🙂

[obulat]

@obulat and @dhruvkb do y'all know when that "Oops! Something went wrong" clause happens and what it's meant to handle?

No, I don't know what it was meant to handle. It was there even before Nuxt :) I really wonder what we catch with this PR!

[ramadanomar]

Then i believe capture event is the best option, as we can enrich the event data as we see fit

[obulat]

I'm not sure what the scope of this PR is, @ramadanomar, but it would be nice to re-write the handleMediaError function in light of the Axios handling errors page.

Please, feel free to leave this PR as is, and we can develop more in future PRs, too.

For instances of Axios errors:

In case the error has a response property

The request was made but has received a response that is not in 2xx range.

• set the message to "Error fetching ${mediaType}. Request failed with status code ${error.response.status}"
• capture the event in Sentry

In case the error doesn't have a response property, but has a request property

The request was made but no response was received.
error.request is an instance of XMLHttpRequest in the browser and an instance of http.ClientRequest in node.js

• set the message to "Error fetching ${mediaType}. Could not receive a response from the server"
• capture the event in Sentry

Otherwise

Something happened in setting up the request that triggered an Error

• set the message to "Error fetching ${mediaType}. Unknown Axios error"
• capture the event in Sentry.

Non-axios errors

I have no idea if those can happen, but we should still add a branch to capture those with Sentry and set the message to "Error fetching ${mediaType}. Unknown error"

I wonder how much we want to capture in Sentry. Do we want to capture all failed requests in Sentry, @WordPress/openverse-frontend? Would capturing the event with toJSON make sense, or would it be too much information?

[ramadanomar]

I'm not sure what the scope of this PR is, @ramadanomar, but it would be nice to re-write the handleMediaError function in light of the Axios handling errors page.

Hey! Thanks for the feedback. My scope was to provide more context to audio errors. I am pushing an updated handleMediaError function right now. As of toJSON, i am not sure when this error is called to determine how much info we need. The new handleMediaError function should allow for easy changes in case we need to edit the level of detail we require.

[sarayourfriend]

@obulat From my perspective all failed requests that we don't know how to explicitly handle should be captured in Sentry so that we can be aware of them and either (a) handle those explicitly or (b) explicitly ignore them as non-issues or unsolveable. Does that seem like a reasonable standard?

[obulat]

This change will definitely help us fix more errors! I've left a couple of nit-picky formatting comments, other that that it's good to go 🎉

[sarayourfriend]

Looks great. Haven't run it locally yet but I anticipate it being fine. My only request is to expand the unit tests to cover the two new branches and to confirm that captureEvent is called as we expect.

[ramadanomar]

Any idea on how i could mock the sentry capture event without importing nuxtjs/sentry module? tried accesing it through this.$nuxt.$sentry , this.$sentry, $nuxt.$sentry and $sentry but they all seem to be undefined. Another method i tried was mocking the function call, something similar to

jest.mock('axios', () => ({
  ...jest.requireActual('axios'),
  isAxiosError: jest.fn((obj) => 'response' in obj),
}))

but couldn't seem to be able to deconstruct the event properly. Any pointers on how i could approach this? Thanks!

[obulat]

@ramadanomar, to mock Sentry centrally, you can add this snippet to test/unit/setup-after-env.js:

import Vue from 'vue'

Vue.prototype.$nuxt = {
  context: {
    $sentry: {
      captureException: jest.fn(),
    },
  },
}

You can also add any other functions that need to be mocked.
I found this solution here: vuejs/pinia#1252

[obulat]

Then, for example, on line 385 in test/unit/specs/stores/media-store.spec.js you could use the mock like this:

      expect(mediaStore.$nuxt.$sentry.captureEvent).toHaveBeenCalledWith({
        message:
          'Error fetching audio from API. Request failed with status code: 500',
        extra: { error },
      })

Please note that you would also need to add captureEvent to the mock above for this to work correctly.

[obulat]

@ramadanomar, I've opened a PR in your fork with some more tests for different branches of error handling. I would really be happy to approve after those tests are added.

[ramadanomar]

I've merged your pr @obulat. Thanks for the help!

[obulat]

Thank you for all your work on this PR, @ramadanomar ! It will be really valuable in triaging Sentry issues.

Stale review

[zackkrida]

LGTM