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

add TooManyRequestError to sfxclient #179

Merged
merged 1 commit into from
Apr 17, 2020
Merged

add TooManyRequestError to sfxclient #179

merged 1 commit into from
Apr 17, 2020

Conversation

beanliu
Copy link
Contributor

@beanliu beanliu commented Apr 17, 2020

Some information from SignalFx could be used to help the client to decide what to do on failure:

  • 429 is only returned by the ingestion data points when a limit has been reached of metrics or event rates.

  • There is a Throttle-Type header returned with the 429 which indicates what the reason was for the 429 to be returned.

  • There is a Retry-After header returned with the 429 which indicates the backoff time which should be used.

@beanliu beanliu closed this Apr 17, 2020
@beanliu beanliu deleted the rate_limit_error branch April 17, 2020 03:50
@beanliu beanliu restored the rate_limit_error branch April 17, 2020 03:52
@beanliu beanliu reopened this Apr 17, 2020
return t.Sub(time.Now()), nil
}

// Retry-After: <delay-seconds>
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this form is the only one we emit, not the (above, and likely correct) HTTP 1.1 date.

Copy link
Contributor

Choose a reason for hiding this comment

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

(just a note, that you included the date parse above is nice of you!)

Copy link
Contributor

@cory-signalfx cory-signalfx left a comment

Choose a reason for hiding this comment

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

I like it. Adding the new error type seems appropriate as we're returning additional headers.

@cory-signalfx
Copy link
Contributor

cory-signalfx commented Apr 17, 2020

This patch will allow the client to discern a 429, but it does not adjust the behavior of the sink to honor the Retry-After header. In other words, it doesn't make it any worse than it is now, but opens the doors to change that behavior later. :)

@cory-signalfx cory-signalfx merged commit 271572a into signalfx:master Apr 17, 2020
@beanliu
Copy link
Contributor Author

beanliu commented Apr 19, 2020

@cory-signalfx thanks for the quick review 😄

unfortunately it breaks the build:

  • for go 1.12, it failed to compile because the lack of errors.As.
  • for go 1.13, it failed a few lint-checks 😓

I'll read the build script and make sure it's green before submitting another PR to fix that.

@beanliu beanliu mentioned this pull request Apr 20, 2020
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.

None yet

2 participants