Skip to content

Increase HTTP timeout for API requests (TECH-3849)#17

Merged
lauralorenz merged 5 commits into
masterfrom
TECH-3849-custom-timeout
Mar 19, 2018
Merged

Increase HTTP timeout for API requests (TECH-3849)#17
lauralorenz merged 5 commits into
masterfrom
TECH-3849-custom-timeout

Conversation

@elidickinson

@elidickinson elidickinson commented Mar 16, 2018

Copy link
Copy Markdown
Member

See https://industrydive.atlassian.net/browse/TECH-3849

This is a fairly disgusting monkeypatch to the HTTP request logic defined upstream in https://github.com/sailthru/sailthru-python-client/ because we have discovered that the timeout attached to all API requests of a hard 10 seconds is both too short (per Sailthru support) and not at all configurable.

Note there is separately a pending PR on the upstream client to fix this here sailthru/sailthru-python-client#67 but it does not appear likely to be merged any time soon.

The previously existing tests pass and I believe this code does what we want (though I did not explicitly test the timeout).

It's unfortunate that we have to duplicate an upstream function and alter it and then patch it into place to get the functionality we want. It means we will have to pay very close attention to any future releases of sailthru-python-client in case that are changes that could break or alter this behavior. The long term fix is to probably transition this whole repo from being a subclass of sailthru-python-client to being a proper fork, which will make the code a little easier to read and will make it easier to track and integrate changes from the upstream library.

@elidickinson

Copy link
Copy Markdown
Member Author

Ironically I believe the CircleCI is now failing because Sailthru is being slow to process API requests and reads/writes to sailthru are not Consistent (in the ACID sense).

Probably need to add a short pause between setting a value on a profile and then using it for something. Ugh.



class DiveSailthruClient(SailthruClient):
# There is some skullduggery below in order to override the hardcoded 10 second timeout on HTTP requests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 for SAT word

class DiveSailthruClient(SailthruClient):
# There is some skullduggery below in order to override the hardcoded 10 second timeout on HTTP requests
# per TECH-3849. First we copy/paste the sailthru_http_request() function that originally exists here:
# https://github.com/sailthru/sailthru-python-client/blob/521fdaa30890a29da8fbb02726e7d22ed174b878/sailthru/sailthru_http.py#L30

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this referenced version allowed for a headers parameter as well though the below copy does not include it, did you take it out here on purpose?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Honestly just wrote my version before looking at theirs. I guess it's better to expose the things that anyone might want to change as parameters, but this is an internal function anyway so it's not like the end user could use headers -- it'd be for use in subclasses of the API Client

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was worried that some higher yet still internal level method in sailthru client accepts headers and depends on being able to pass them down to this method eventually (in other words, worried about getting getting a TypeError a la "takes exactly X arguments, X+1 given" if we called something that tried to send headers all the way down). I did a browse and it doesn't seem so; only for a few high level API cases for the end user to supply (i.e. https://github.com/sailthru/sailthru-python-client/blob/17e201ccfbae747ce01b71a7fd1f8470cd51621c/sailthru/sailthru_client.py#L516) which we don't even use these higher level methods in our client/possibly anywhere ever.

@lauralorenz

lauralorenz commented Mar 19, 2018

Copy link
Copy Markdown
Contributor

@elidickinson FWIW I just tried this on datadive for the ad_campaign_2.query_sailthru_campaigns task, which is the one that is having the timeout errors,
with dive_sailthru_client installed at commit 9f51e96
and got this error:

sailthru.sailthru_error.SailthruClientError: HTTPSConnectionPool(host='api.sailthru.com', port=443): Max retries exceeded with url: /blast?json=%7B%22status%22%3A+%22sent%22%2C+%22limit%22%3A+999999%2C+%22start_date%22%3A+%222018-01-17%22%2C+%22end_date%22%3A+%222018-02-16%22%7D&api_key=XXX&sig=XXX&format=json (Caused by NewConnectionError('<requests.packages.urllib3.connection.VerifiedHTTPSConnection object at 0x7fba23282d90>: Failed to establish a new connection: [Errno -2] Name or service not known',))

@elidickinson

Copy link
Copy Markdown
Member Author

Name or service not known is a weird one. That implies to me a problem with the DNS lookup.

@lauralorenz

Copy link
Copy Markdown
Contributor

@elidickinson ok good call, I reran it again and it succeeded YAY

@lauralorenz lauralorenz merged commit 8055657 into master Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants