Skip to content

raven - capture breadcrumb on failed request #1293

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

Merged
merged 2 commits into from
Apr 10, 2018

Conversation

kumavis
Copy link
Contributor

@kumavis kumavis commented Apr 6, 2018

Fixes #1009

If there is an issue performing the request (not an error response from the server), we don't get any data about what failed, only a TypeError: Failed to Fetch with no trace or hint of what was being fetched.

With this change, we capture a breadcrumb indicating that the fetch failed, then re-throw the error.

If there is no additional catch in the chain, this will result in an unhandled promise rejection with the original unhelpful error message: TypeError: Failed to Fetch.

Question: can you set a log level on a type: 'http' breadcrumb? will this be rendered correctly in Sentry?

@kumavis kumavis requested a review from kamilogorek as a code owner April 6, 2018 20:12
@kumavis
Copy link
Contributor Author

kumavis commented Apr 6, 2018

i dont understand the lint error

.catch is a syntax error

@kamilogorek
Copy link
Contributor

@kamilogorek
Copy link
Contributor

Question: can you set a log level on a type: 'http' breadcrumb? will this be rendered correctly in Sentry?

Yes, you should be able to do this just fine

@kumavis
Copy link
Contributor Author

kumavis commented Apr 9, 2018

@kamilogorek that seems like gratuitous coding around the linter, instead of removing an unnecessary lint rule or just adding a flag to ignore the lint rule in this case

@kumavis
Copy link
Contributor Author

kumavis commented Apr 9, 2018

and yet this is what eslint --fix produces
whatever, let me commit that

@kumavis
Copy link
Contributor Author

kumavis commented Apr 9, 2018

ECMAScript version 3 compatible style, avoiding dot notation for reserved word properties.

'catch' is a reserved word in ECMAScript 3, IE8 & 9 require this written in brackets e.g. this['catch'].bind() rather than the dot notation.

hmm, TIL

@kumavis
Copy link
Contributor Author

kumavis commented Apr 9, 2018

@kamilogorek ready to go if it looks good to you

@kamilogorek
Copy link
Contributor

I was trying to find this explanation and post it here. Thanks :)

@kamilogorek kamilogorek merged commit 4fb4ca3 into getsentry:master Apr 10, 2018
@kumavis
Copy link
Contributor Author

kumavis commented Apr 11, 2018

@kamilogorek thanks/ looking forward to getting this live to help diagnose some of our sentry reports 😸

@kumavis kumavis deleted the patch-2 branch April 11, 2018 16:03
@kamilogorek
Copy link
Contributor

@kumavis I'll release a new version once this fix gets merged - #1302
Hopefully on Monday morning (as I try not to release on Fridays if its not a major issue) :)

@kamilogorek
Copy link
Contributor

@kumavis released as 3.24.2

@victorpavlenko
Copy link

victorpavlenko commented Jun 18, 2018

@kumavis
@kamilogorek

Still, doesn't work

TypeError: Failed to fetch
/ in _promiseRejectionHandler
No additional details are available for this frame.

v: raven-js : 3.24.2

@kumavis
Copy link
Contributor Author

kumavis commented Oct 15, 2018

@Xurma2 note that this doesnt re-add the stack, its just supposed to add a breadcrumb

@kamilogorek doesnt seem like its actually adding the breadcrumb, but not sure why

relevant code is now here

['catch'](function(err) {
// if there is an error performing the request
self.captureBreadcrumb({
type: 'http',
category: 'fetch',
data: fetchData,
level: 'error'
});
throw err;
});

@kamilogorek
Copy link
Contributor

@kumavis would you mind giving new v4 SDK a try? We are only supporting security/major issues in old raven-js for quite some time now. In new SDK it's handled here though -

@kumavis
Copy link
Contributor Author

kumavis commented Oct 19, 2018

@kamilogorek i dont know why i didnt see it before -- but I do see the Failed to fetch breadcrumbs now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants