Capturing Issue Description in Sentry

[ramadanomar]

trafficstars

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.

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

[ramadanomar]

Thanks for the changes @obulat . I resolved them.

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

[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: https://github.com/vuejs/pinia/discussions/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!