Repository navigation
[APM] Surface http errors to users - #42160
Conversation
💔 Build Failed |
|
Pinging @elastic/apm-ui |
💚 Build Succeeded |
💚 Build Succeeded |
dgieselaar
left a comment
There was a problem hiding this comment.
LGTM, just a few nits.
| <NoServicesMessage isLoading={false} historicalDataFound={false} /> | ||
| ); | ||
| expect(wrapper).toMatchSnapshot(); | ||
| Object.values(FETCH_STATUS).forEach(status => { |
There was a problem hiding this comment.
are these useful tests? I guess they don't hurt either, but it feels like it's just generating code paths and then generating snapshots of all the different code paths. The component itself seems to be simple (and will likely stay that way) to warrant snapshot tests.
There was a problem hiding this comment.
I felt it was an okay-ish way to test the different code paths since the generated snapshots were rather small (I agree that changes in big snapshots are often overlooked). What do you suggest instead?
| title: i18n.translate('xpack.apm.fetcher.error.title', { | ||
| defaultMessage: `Error while fetching resource` | ||
| }), | ||
| text: `${idx(err.res, r => r.status)}: ${idx(err.res, r => r.url)}` |
There was a problem hiding this comment.
I feel like this could be a bit more user friendly. Maybe a little markup? Do we need the status code? Is there any property from the error (response) we can display instead? (e.g., often errors have a message property that we can surface).
There was a problem hiding this comment.
There is also the error.statusText which we can use instead of status. So instead of 404 it would show Not Found. It is not always as distinguishable as a status code (I'm thinking if the user takes a screenshot and opens a support ticket) but I'm okay with changing it.
There was a problem hiding this comment.
Full list of status codes and status texts: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
Looks like the texts are unique so might be as good as a status code for us.
There was a problem hiding this comment.
Added some markup and the statusText. Looks a whole bit nicer now :)
💚 Build Succeeded |
* [APM] Surface http errors to users * Fix tests * Add markup to error message * Remove transaction type from ui filters
* [APM] Surface http errors to users * Fix tests * Add markup to error message * Remove transaction type from ui filters
|
The mechanism works, but seems like something else was changed and the content is now less than useful: Created a bug: #45263 |
* [APM] Surface http errors to users * Fix tests * Add markup to error message * Remove transaction type from ui filters
Closes #40986
For components that don't handle errors explicitly a generic toast error will be displayed
The only component that handle errors explicitly is the service overview (this being the first page the users see, it's probably a bit more important)
