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.

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

fetch error logging to use warn instead of error #43

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

victorlpgazolli
Copy link
Contributor

@victorlpgazolli victorlpgazolli commented May 6, 2024

Simple change from console.error to console.warn in .fetch method

motivation:
Users with no internet access can cause fetch to throw errors, this can be seen by these errors messages:

  • "Request timeout after 10000 milliseconds"
  • "Network request failed"

When this sdk is integrated with an app that uses sentry, handling this error with console.error can flood sentry issues with users who has internet related problems (timeout or unreachable network).

One way to handle this kind of error would be to check for response.status === 0, and ignore this internet related error.
BUT this pull request suggest an 'one-liner' approach (nothing sofisticated, just a workaround).

Other way (arguably the best way) would be to just throw this exception, and let it break. So the developer (user of this sdk) can handle this kind of error. I would like to see this implementation :)

tldr;
@amplitude/experiment-react-native-client flooding Sentry platform with "Network request failed" and "Request timeout after 10000 milliseconds" with this "console.error"

@victorlpgazolli
Copy link
Contributor Author

Btw, i follow this Issue:
#9 (comment)

But the recommendations don't seem to fix the root problem

@victorlpgazolli victorlpgazolli changed the title fetch update error logging to use warn instead of error fetch error logging to use warn instead of error May 6, 2024
@bgiori bgiori merged commit 6f9a57a into amplitude:main Jul 1, 2024
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.

2 participants