feat: Set log level for Fetch/XHR breadcrumbs based on status code #13711
Conversation
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
| * @param breadcrumb | ||
| */ | ||
| export function assignBreadcrumbLogLevel(breadcrumb: Breadcrumb): Breadcrumb { | ||
| const statusCode = breadcrumb.data?.status_code; |
There was a problem hiding this comment.
FetchBreadcrumbData and XhrBreadcrumbData are sent as the data property, which optionally contains status_code.
There was a problem hiding this comment.
Hey, thanks for (once again) contributing! :)
On first glance, this looks great to me! Would be amazing if you could add some tests for this. I think we should be able to test this in our Node integration-tests fairly well. (as well as maybe a couple of unit tests for the assignBreadcrumbLogLevel if you feel like doing extra credit work 😅)
I'm also gonna tag @lforst for a review since he has more context on this issue.
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
…Breadcrumb` is called Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
| } else if (statusCode >= 500) { | ||
| return 'error'; | ||
| } else { | ||
| return 'info'; |
There was a problem hiding this comment.
For the undefined and other cases, the fallback level is info. So, all the test cases for breadcrumbs in browser-integration-tests must be updated, which is quite a lot.
Is this behaviour needed? E.g., non-error request breadcrumb will have an additional property level: 'info'
There was a problem hiding this comment.
Currently many tests are failing b/c of the default property. Maybe we shouldn't set it if the response has a non-error status code. Otherwise, it's a handful to fix those tests (though it's simply just adding the property).
Any thoughts?
There was a problem hiding this comment.
For now, i've left out the default value (info) to not break the many other tests. But other sdks use a default value: getsentry/sentry-php@d12482a. So we should consider it.
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
Modify `getBreadcrumbLogLevel` to account only for 4xx or 5xx status. Tests updated to reflect the changes to `getBreadcrumbLogLevel` function. Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
lforst
changed the title
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #13711 Co-authored-by: lforst <8118419+lforst@users.noreply.github.com>
Fixes #13359
yarn lint) & (yarn test).