-
-
Notifications
You must be signed in to change notification settings - Fork 333
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
ref(transport): Log a warning when dropping envelopes due to rate-limiting #4463
ref(transport): Log a warning when dropping envelopes due to rate-limiting #4463
Conversation
Log a warning if all items of the current envelope were rate-limited Fixes getsentry#4456
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4463 +/- ##
=============================================
- Coverage 91.817% 91.810% -0.008%
=============================================
Files 610 610
Lines 68193 68231 +38
Branches 24478 24497 +19
=============================================
+ Hits 62613 62643 +30
- Misses 5487 5496 +9
+ Partials 93 92 -1
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the warning log message will be lost in all the noise, but we will fix that here #4467.
1e206e5
to
f88da37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR deserves an entry into the Changelog. I think it's an improvement. So add an ##Unreleased entry with a ### Improvements section to the changelog and mention this PR.
@philipphofmann Done, I've also removed the A general question from my side - not every single commit on a branch has to follow the rules defined here, right? So as long as let's say the commit message of the final "Squash & Merge" does follow the rules, I'm good? |
Yes, only the "Squash & merge" message needs to follow the rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
📜 Description
SENTRY_LOG_WARN
.Update(will be addressed in a separate PR)clang-formatter
to19.1.2
clang-format version mismatch, expected: 19.1.1, but found: 19.1.2. Please run
make initto update your local dev tools. This may actually upgrade to a newer version than what is currently recorded in the lockfile; if that happens, please commit the update to the lockfile as well.
make init
, but then followedThis may actually upgrade to a newer version than what is currently recorded in the lockfile; if that happens, please commit the update to the lockfile as well.
pre-commit
,python
andmobile-dev-inc/tap/maestro
got updated as well, but since those weren't crucial for the commit, I didn't stage/commit those changes.💡 Motivation and Context
💚 How did you test it?
SentryHttpTransportTests
See below:
SentryHttpTransportTests
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps