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

TDL-15860 Implement Request Timeouts #79

Merged
merged 20 commits into from
Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ OAuth is the default authentication method for `tap-zendesk`. To use OAuth, you
{
"access_token": "AVERYLONGOAUTHTOKEN",
"subdomain": "acme",
"start_date": "2000-01-01T00:00:00Z"
"start_date": "2000-01-01T00:00:00Z",
"request_timeout": 300
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add details of it as it is the optional parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

}
```

- `request_timeout` (integer, `300`): It is the time for which request should wait to get response. It is an optional parameter and default request_timeout is 300 seconds.
### Using API Tokens

For a simplified, but less granular setup, you can use the API Token authentication which can be generated from the Zendesk Admin page. See https://support.zendesk.com/hc/en-us/articles/226022787-Generating-a-new-API-token- for more details about generating an API Token. You'll then be able to use the admins's `email` and the generated `api_token` to authenticate.
Expand All @@ -33,8 +34,10 @@ For a simplified, but less granular setup, you can use the API Token authenticat
"email": "user@domain.com",
"api_token": "THISISAVERYLONGTOKEN",
"subdomain": "acme",
"start_date": "2000-01-01T00:00:00Z"
"start_date": "2000-01-01T00:00:00Z",
"request_timeout": 300
}
```
- `request_timeout` (integer, `300`):It is the time for which request should wait to get response. It is an optional parameter and default request_timeout is 300 seconds.

Copyright © 2018 Stitch
9 changes: 8 additions & 1 deletion tap_zendesk/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

LOGGER = singer.get_logger()

REQUEST_TIMEOUT = 300

REQUIRED_CONFIG_KEYS = [
"start_date",
Expand Down Expand Up @@ -191,10 +192,16 @@ def get_session(config):
def main():
parsed_args = singer.utils.parse_args(REQUIRED_CONFIG_KEYS)

# Set request timeout to config param `request_timeout` value.
config_request_timeout = parsed_args.config.get('request_timeout')
if config_request_timeout and float(config_request_timeout):
request_timeout = float(config_request_timeout)
else:
request_timeout = REQUEST_TIMEOUT # If value is 0, "0", "" or not passed then it sets default to 300 seconds.
# OAuth has precedence
creds = oauth_auth(parsed_args) or api_token_auth(parsed_args)
session = get_session(parsed_args.config)
client = Zenpy(session=session, **creds)
client = Zenpy(session=session, timeout=request_timeout, **creds) # Pass request timeout

if not client:
LOGGER.error("""No suitable authentication keys provided.""")
Expand Down
2 changes: 1 addition & 1 deletion tap_zendesk/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ def raise_for_error(response):
giveup=is_fatal)
@backoff.on_exception(backoff.expo,
(ConnectionError, Timeout),#As ConnectionError error and timeout error does not have attribute status_code,
max_tries=10, # here we added another backoff expression.
max_tries=5, # here we added another backoff expression.
factor=2)
def call_api(url, request_timeout, params, headers):
Copy link
Contributor

Choose a reason for hiding this comment

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

@prijendev I see all the calls to function call_api is updated due to addition of a single parameter in the function argument.
My suggestion:
Since request_timeout is initialized in the main() function, can't we directly utilize it in the call_api function instead of passing it as an argument? The benefit of this change will be, changes to call_api function can be removed. Minimal changes will be resulted in the PR. Code would be more robust and generic.

CC: @dbshah1212

Copy link
Contributor Author

@prijendev prijendev Nov 1, 2021

Choose a reason for hiding this comment

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

In this tap some of the streams using Zenpy sdk. So, to initialize sdk client we have read request_timeout in main() function. While for other streams each sync method using call_api function. Updated initialization of request_timeout to common parent Stream class to minimize the changes.

response = requests.get(url, params=params, headers=headers, timeout=request_timeout) # Pass request timeout
Expand Down
11 changes: 6 additions & 5 deletions test/unittests/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
'after_cursor': 'some_cursor'}
Copy link
Contributor

Choose a reason for hiding this comment

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

@prijendev Did we write test cases for different scenarios of request_timeout variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, We write test cases for different scenarios of request_timeout variable in test_request_timeout.py file.

}

REQUEST_TIMEOUT = 300
def mocked_get(*args, **kwargs):
fake_response = requests.models.Response()
fake_response.headers.update(kwargs.get('headers', {}))
Expand All @@ -37,7 +38,7 @@ class TestBackoff(unittest.TestCase):
side_effect=[mocked_get(status_code=200, json=SINGLE_RESPONSE)])
def test_get_cursor_based_gets_one_page(self, mock_get, mock_sleep):
responses = [response for response in http.get_cursor_based(url='some_url',
access_token='some_token', request_timeout=300)]
access_token='some_token', request_timeout=REQUEST_TIMEOUT)]
actual_response = responses[0]
self.assertDictEqual(SINGLE_RESPONSE,
actual_response)
Expand All @@ -54,7 +55,7 @@ def test_get_cursor_based_gets_one_page(self, mock_get, mock_sleep):
def test_get_cursor_based_can_paginate(self, mock_get, mock_sleep):
responses = [response
for response in http.get_cursor_based(url='some_url',
access_token='some_token', request_timeout=300)]
access_token='some_token', request_timeout=REQUEST_TIMEOUT)]

self.assertDictEqual({"key1": "val1", **PAGINATE_RESPONSE},
responses[0])
Expand All @@ -79,7 +80,7 @@ def test_get_cursor_based_handles_429(self, mock_get, mock_sleep):
- can handle either a string or an integer for the retry header
"""
responses = [response for response in http.get_cursor_based(url='some_url',
access_token='some_token', request_timeout=300)]
access_token='some_token', request_timeout=REQUEST_TIMEOUT)]
actual_response = responses[0]
self.assertDictEqual({"key1": "val1", **SINGLE_RESPONSE},
actual_response)
Expand Down Expand Up @@ -294,7 +295,7 @@ def test_call_api_handles_timeout_error(self,mock_get, mock_sleep):
pass

# Verify the request retry 5 times on timeout
self.assertEqual(mock_get.call_count, 10)
self.assertEqual(mock_get.call_count, 5)

@patch('requests.get')
def test_call_api_handles_connection_error(self,mock_get, mock_sleep):
Expand All @@ -306,5 +307,5 @@ def test_call_api_handles_connection_error(self,mock_get, mock_sleep):
pass

# Verify the request retry 5 times on timeout
self.assertEqual(mock_get.call_count, 10)
self.assertEqual(mock_get.call_count, 5)

Loading