-
Notifications
You must be signed in to change notification settings - Fork 141
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
Statsbeat do not count failed request when throttle #911
Conversation
Library/Sender.ts
Outdated
@@ -197,11 +205,8 @@ class Sender { | |||
} | |||
} else if (this._isRetriable(res.statusCode)) { | |||
try { | |||
if (this._statsbeat) { | |||
if (this._statsbeat && res.statusCode != 429) { |
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.
I believe 429 is also a retry scenario.
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.
So, we send Statsbeat metrics for throttle and retry, @heyams can you confirm?
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.
we do retry on 429. no retry on 439, which is throttling.
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.
can you only add 439 to throttling count in Statsbeat and then count 429 as failure? that's what we do in java.
@hectorhdzg here is breeze's definition of these 2 response codes:
|
Thanks @heyams, this PR will actually fix issue where we do not retry in Throttle as well, as other PR was using wrong code #910
|
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.
👍
@hectorhdzg let's chat tomorrow if we want to send two count to breeze.. daily quota exceeded vs. monthly quota exceeded. |
No description provided.