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 giveup function for requests #11

Closed
mdelaurentis opened this issue Mar 14, 2017 · 1 comment

Comments

@mdelaurentis
Copy link
Contributor

@mdelaurentis mdelaurentis commented Mar 14, 2017

Most of our Taps use a combination of the Python requests and backoff to make HTTP requests that retry with a backoff strategy. A typical Tap will have a bit of code that looks like this:

def giveup(error):
    response = error.response
    return not (response.status_code == 429 or
                response.status_code >= 500)


@backoff.on_exception(backoff.constant,
                      (requests.exceptions.RequestException),
                      jitter=backoff.random_jitter,
                      max_tries=5,
                      giveup=giveup,
                      interval=30)
def request(url, access_token, params={}):
    requests.request(...)

We've seen an issue with the Outbrain tap where a ConnectionException is raised because of a snapped connection. There is no HTTP response in this case, so the error argument to giveup has no response property, and giveup throws an exception when we try to access error.response.status_code.

This could be fixed with a simple change to giveup:

def giveup(error):
    response = error.response
    if response is None:
        return False
    return not (response.status_code == 429 or
                response.status_code >= 500)

I think this logic is getting complex enough that we should add an implementation of giveup that does something like the above into the singer-python library. If we don't, it's likely that every Tap will experience the same error trying to access properties on a null error.response object at some point.

However, I'm hesitant to add a hard dependency on requests and backoff. So I'm thinking that we should make a module called singer.requests that can contain helper functions like this one that are specific to the requests library. We won't need to modify setup.py to add a dependency on requests, and it's up to a Tap whether they want to import that module at all.

We should give this giveup function a specific name, like giveup_on_http_4xx_except_429, to make room for other giveup strategies.

I don't want to put the decorated request function in this library, because I think it's pretty likely that different Taps would want to use different backoff strategies.

So a Tap implementation would then look more like this:

@backoff.on_exception(backoff.constant,
                      (requests.exceptions.RequestException),
                      jitter=backoff.random_jitter,
                      max_tries=5,
                      giveup=giveup_on_http_4xx_except_429,
                      interval=30)
def request(url, access_token, params={}):
    requests.request(...)
@karstendick

This comment has been minimized.

Copy link
Contributor

@karstendick karstendick commented Mar 14, 2017

Closed by #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.