Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(browser): Attach host as part of error message to "Failed to fetch" errors #15729

Merged
merged 4 commits into from
Mar 24, 2025
Merged

feat(browser): Attach host as part of error message to "Failed to fetch" errors #15729

merged 4 commits into from
Mar 24, 2025

Conversation

mydea
Copy link
Member

@mydea mydea commented Mar 19, 2025

Closes #15709

This also adds tests for the various types of TypeErrors that fetch can produce.

@mydea mydea requested review from lforst and chargome March 19, 2025 10:39
@mydea mydea self-assigned this Mar 19, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 19, 2025 •
edited
Loading

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 23.21 KB +0.4% +94 B 🔺
@sentry/browser - with treeshaking flags 23.01 KB +0.44% +103 B 🔺
@sentry/browser (incl. Tracing) 36.63 KB +0.26% +94 B 🔺
@sentry/browser (incl. Tracing, Replay) 73.79 KB +0.12% +88 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 67.12 KB +0.14% +96 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 78.42 KB +0.12% +93 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 91 KB +0.1% +88 B 🔺
@sentry/browser (incl. Feedback) 40.34 KB +0.22% +90 B 🔺
@sentry/browser (incl. sendFeedback) 27.85 KB +0.32% +89 B 🔺
@sentry/browser (incl. FeedbackAsync) 32.63 KB +0.27% +89 B 🔺
@sentry/react 25 KB +0.34% +86 B 🔺
@sentry/react (incl. Tracing) 38.52 KB +0.25% +98 B 🔺
@sentry/vue 27.44 KB +0.35% +96 B 🔺
@sentry/vue (incl. Tracing) 38.3 KB +0.23% +88 B 🔺
@sentry/svelte 23.25 KB +0.39% +91 B 🔺
CDN Bundle 24.43 KB +0.39% +95 B 🔺
CDN Bundle (incl. Tracing) 36.63 KB +0.26% +94 B 🔺
CDN Bundle (incl. Tracing, Replay) 71.62 KB +0.13% +92 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 76.83 KB +0.13% +101 B 🔺
CDN Bundle - uncompressed 71.39 KB +0.32% +230 B 🔺
CDN Bundle (incl. Tracing) - uncompressed 108.59 KB +0.21% +230 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 219.84 KB +0.11% +230 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 232.41 KB +0.1% +230 B 🔺
@sentry/nextjs (client) 39.81 KB +0.23% +93 B 🔺
@sentry/sveltekit (client) 37.03 KB +0.22% +82 B 🔺
@sentry/node 142.61 KB -0.01% -1 B 🔽
@sentry/node - without tracing 96 KB - -
@sentry/aws-serverless 120.36 KB -0.01% -1 B 🔽

View base workflow run

chargome
chargome reviewed Mar 19, 2025
View reviewed changes
Copy link
Member

@chargome chargome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, I should push my changes earlier 😄 The tests are nice!

This will have immediate impact on the grouping in the issue streams, should we flag it first to check the outcomes eg in docs?

@mydea mydea force-pushed the fn/add-fetch-error-tests branch from 65b835e to b3fde77 Compare March 19, 2025 13:06
lforst
lforst reviewed Mar 19, 2025
View reviewed changes
packages/core/src/utils-hoist/instrument/fetch.ts
) {
try {
const url = new URL(handlerData.fetchData.url);
error.message = `${error.message} (${url.host})`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether we should limit to the domain (eg example.com) instead of the entire host (foo.bar.example.com). Domain could get pretty noisy in multi-tenant situations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I forgot. Message doesn't contribute to grouping if there is a stack trace. Then it's probably fine/better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so basically url.host.split('.).slice(-2).join('.'), to just get the last part? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted this and also added a test for this!

@mydea mydea force-pushed the fn/add-fetch-error-tests branch from c432818 to 981a5f8 Compare March 19, 2025 14:16
@chargome chargome mentioned this pull request Mar 19, 2025
Draft
@lforst
Copy link
Member

lforst commented Mar 20, 2025

This will have immediate impact on the grouping in the issue streams, should we flag it first to check the outcomes eg in docs?

I actually don't think it will. I recently found this in the product in the grouping section in an issue:

Screenshot 2025-03-20 at 10 55 33

lforst
lforst approved these changes Mar 20, 2025
View reviewed changes
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice

@lforst lforst changed the title feat(browser): Attach top-level domain to "Failed to fetch" errors feat(browser): Attach host as part of error message to "Failed to fetch" errors Mar 20, 2025
mydea added 3 commits March 24, 2025 10:34
@mydea
feat(browser): Attach top-level domain to "Failed to fetch" errors
35be94b 
Closes #15709

This also adds tests for the various types of TypeErrors that fetch can produce.
@mydea
take top level domain only
46e4ded
@mydea
do not use subdomain
38e1b89
@mydea mydea force-pushed the fn/add-fetch-error-tests branch from 981a5f8 to 38e1b89 Compare March 24, 2025 09:38
@mydea
Copy link
Member Author

mydea commented Mar 24, 2025

Updated this to use the full host (e.g. subdomain.sentry.io), because turns out the previous logic was flawed, as it e.g. does not handle domain.co.uk. Also, it seems that the hostname is correctly identified for grouping anyhow, so this should be fine.

@mydea mydea merged commit 0cf8ec2 into develop Mar 24, 2025
153 checks passed
@mydea mydea deleted the fn/add-fetch-error-tests branch March 24, 2025 10:21
@github-actions github-actions bot mentioned this pull request Mar 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lforst lforst lforst approved these changes

@chargome chargome chargome approved these changes

Assignees

@mydea mydea

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Attach top level domain to Failed to fetch errors.
3 participants
@mydea @lforst @chargome