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

feat: handle rate limit exceeded scenario in HttpTransport #2551

Closed
wants to merge 1 commit into from

Conversation

Nahrawene-ben-hmida
Copy link

📜 Description

Pull Request Description: Improve Error Handling for Rate Limits in HttpTransport

Changes Made

  1. Added QuotaExceededException:

    • Introduced a new custom exception (QuotaExceededException) to signal explicitly when a rate limit is reached (HTTP status code 429).
    • Simplifies the handling of rate-limiting scenarios programmatically for users of the library.
  2. Updated send Method:

    • Added a check for HTTP status code 429 in the send method.
    • Throws the newly created QuotaExceededException when the rate limit is exceeded.
  3. Improved Error Feedback:

    • Ensures errors caused by rate limits are clearly communicated.
    • Aligns with Sentry's goal of providing better transparency and usability for developers.

Motivation

The current implementation does not make it straightforward to identify and handle rate-limiting errors. This improvement introduces an explicit mechanism to manage such cases, enhancing the developer experience and making the library more robust.

Testing

  • Validated that the QuotaExceededException is thrown correctly when a 429 status code is received.
  • Ensured existing behavior remains consistent for other HTTP status codes.

Impact

  • Fully backward-compatible.
  • Improves the error-handling ergonomics for developers using the Sentry Dart SDK.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify changes.
  • No new PII added, or the SDK only sends newly added PII if sendDefaultPii is enabled.
  • I updated the documentation if needed.
  • All tests are passing.
  • No breaking changes.

@melWiss
Copy link

melWiss commented Jan 3, 2025

thanks for pushing this through, our team is desperate to have this quickly

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!

@@ -50,6 +50,10 @@ class HttpTransport implements Transport {
if (response.statusCode == 200) {
return _parseEventId(response);
}
if (response.statusCode == 429) {
// throw error if rate limit is reached
throw QuotaExceededException();
Copy link
Contributor

@buenaflor buenaflor Jan 13, 2025

Choose a reason for hiding this comment

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

in the SDK generally we don't throw exceptions, but rather log it

e.g options.logger( SentryLevel.warning, 'example status code message')

and throwing here might cause a recursion since we override FlutterError.onError and try to send unhandled errors where this Exception will be triggered again and so on etc..

@denrase
Copy link
Collaborator

denrase commented Jan 21, 2025

@melWiss @Nahrawene-ben-hmida Took the liberty to create a PR where we call the logger with a warning on 429 headers. Please check if this is sufficient for your use-case and get back to us, thx! 🙇

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.

4 participants