Skip to content

ISSUE-326 Expose request timeout configuration for retrieving datafile #327

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

ISSUE-326 Expose request timeout configuration for retrieving datafile #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
031e31e
ISSUE-326 Expose request timeout configuration for retrieving datafile
May 7, 2021
a619854
ISSUE-326 - added docstring
May 8, 2021
67efddf
provided default settings in docstring
May 8, 2021
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
38 changes: 34 additions & 4 deletions optimizely/config_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,17 @@ def __init__(
error_handler=None,
notification_center=None,
skip_json_validation=False,
request_timeout=None,
):
""" Initialize config manager. One of sdk_key or url has to be set to be able to use.

Args:
sdk_key: Optional string uniquely identifying the datafile.
datafile: Optional JSON string representing the project.
update_interval: Optional floating point number representing time interval in seconds
at which to request datafile and set ProjectConfig.
at which to request datafile and set ProjectConfig. (Default is 300 secs)
blocking_timeout: Optional Time in seconds to block the get_config call until config object
has been initialized.
has been initialized. (Default is 10 secs)
url: Optional string representing URL from where to fetch the datafile. If set it supersedes the sdk_key.
url_template: Optional string template which in conjunction with sdk_key
determines URL from where to fetch the datafile.
Expand All @@ -183,6 +184,8 @@ def __init__(
skip_json_validation: Optional boolean param which allows skipping JSON schema
validation upon object invocation. By default
JSON schema validation will be performed.
request_timeout: Optional Time in seconds to block the fetch_datafile call
until datafile object retrieved. (Default is 10 secs)

"""
self._config_ready_event = threading.Event()
Expand All @@ -198,6 +201,7 @@ def __init__(
)
self.set_update_interval(update_interval)
self.set_blocking_timeout(blocking_timeout)
self.set_request_timeout(request_timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to update the docstring (comment) mentioning request_timeout.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comments, REQUEST_TIMEOUT renamed was done via refactoring into DEAFAULT_REQUEST_TIMEOUT and I have double-checked for any reminding presence with "find all".

docstring is now updated.

self.last_modified = None
self._polling_thread = threading.Thread(target=self._run)
self._polling_thread.setDaemon(True)
Expand Down Expand Up @@ -309,6 +313,32 @@ def set_blocking_timeout(self, blocking_timeout):

self.blocking_timeout = blocking_timeout

def set_request_timeout(self, request_timeout):
""" Helper method to stop waiting for a response after a given number of seconds with the timeout parameter

Args:
request_timeout: Time in seconds to block the config call.
"""
if request_timeout is None:
request_timeout = enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT
self.logger.debug('Setting config request timeout to default value {}.'.format(request_timeout))

if not isinstance(request_timeout, (numbers.Integral, float)):
raise optimizely_exceptions.InvalidInputException(
'Invalid request timeout "{}" provided.'.format(request_timeout)
)

# If request timeout is less than 0 then set it to default request timeout.
if request_timeout < 0:
self.logger.debug(
'request timeout value {} too small. Defaulting to {}'.format(
request_timeout, enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT
)
)
request_timeout = enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT

self.request_timeout = request_timeout

def set_last_modified(self, response_headers):
""" Looks up and sets last modified time based on Last-Modified header in the response.

Expand Down Expand Up @@ -346,7 +376,7 @@ def fetch_datafile(self):

try:
response = requests.get(
self.datafile_url, headers=request_headers, timeout=enums.ConfigManager.REQUEST_TIMEOUT,
self.datafile_url, headers=request_headers, timeout=self.request_timeout,
)
except requests_exceptions.RequestException as err:
self.logger.error('Fetching datafile from {} failed. Error: {}'.format(self.datafile_url, str(err)))
Expand Down Expand Up @@ -418,7 +448,7 @@ def fetch_datafile(self):

try:
response = requests.get(
self.datafile_url, headers=request_headers, timeout=enums.ConfigManager.REQUEST_TIMEOUT,
self.datafile_url, headers=request_headers, timeout=self.request_timeout,
)
except requests_exceptions.RequestException as err:
self.logger.error('Fetching datafile from {} failed. Error: {}'.format(self.datafile_url, str(err)))
Expand Down
2 changes: 1 addition & 1 deletion optimizely/helpers/enums.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class ConfigManager(object):
# Default config update interval of 5 minutes
DEFAULT_UPDATE_INTERVAL = 5 * 60
# Time in seconds before which request for datafile times out
REQUEST_TIMEOUT = 10
DEFAULT_REQUEST_TIMEOUT = 10


class ControlAttributes(object):
Expand Down
43 changes: 37 additions & 6 deletions tests/test_config_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,37 @@ def test_set_blocking_timeout(self, _):
project_config_manager.set_blocking_timeout(5)
self.assertEqual(5, project_config_manager.blocking_timeout)

def test_set_request_timeout(self, _):
""" Test set_request_timeout with different inputs. """
with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
project_config_manager = config_manager.PollingConfigManager(sdk_key='some_key')

# Assert that if invalid request_timeout is set, then exception is raised.
with self.assertRaisesRegexp(
optimizely_exceptions.InvalidInputException, 'Invalid request timeout "invalid timeout" provided.',
):
project_config_manager.set_request_timeout('invalid timeout')

# Assert that request_timeout cannot be set to less than allowed minimum and instead is set to default value.
project_config_manager.set_request_timeout(-4)
self.assertEqual(
enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT, project_config_manager.request_timeout,
)

# Assert that request_timeout can be set to 0.
project_config_manager.set_request_timeout(0)
self.assertIs(0, project_config_manager.request_timeout)

# Assert that if no request_timeout is provided, it is set to default value.
project_config_manager.set_request_timeout(None)
self.assertEqual(
enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT, project_config_manager.request_timeout,
)

# Assert that if valid request_timeout is provided, it is set to that value.
project_config_manager.set_request_timeout(5)
self.assertEqual(5, project_config_manager.request_timeout)

def test_set_last_modified(self, _):
""" Test that set_last_modified sets last_modified field based on header. """
with mock.patch('optimizely.config_manager.PollingConfigManager.fetch_datafile'):
Expand Down Expand Up @@ -388,7 +419,7 @@ def test_fetch_datafile(self, _):
mock_requests.assert_called_once_with(
expected_datafile_url,
headers={'If-Modified-Since': test_headers['Last-Modified']},
timeout=enums.ConfigManager.REQUEST_TIMEOUT,
timeout=enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT,
)
self.assertEqual(test_headers['Last-Modified'], project_config_manager.last_modified)
self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
Expand Down Expand Up @@ -424,7 +455,7 @@ def raise_for_status(self):
mock_requests.assert_called_once_with(
expected_datafile_url,
headers={'If-Modified-Since': test_headers['Last-Modified']},
timeout=enums.ConfigManager.REQUEST_TIMEOUT,
timeout=enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT,
)
mock_logger.error.assert_called_once_with('Fetching datafile from {} failed. Error: Error Error !!'.format(
expected_datafile_url
Expand Down Expand Up @@ -463,7 +494,7 @@ def test_fetch_datafile__request_exception_raised(self, _):
mock_requests.assert_called_once_with(
expected_datafile_url,
headers={'If-Modified-Since': test_headers['Last-Modified']},
timeout=enums.ConfigManager.REQUEST_TIMEOUT,
timeout=enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT,
)
mock_logger.error.assert_called_once_with('Fetching datafile from {} failed. Error: Error Error !!'.format(
expected_datafile_url
Expand Down Expand Up @@ -527,7 +558,7 @@ def test_fetch_datafile(self, _):
expected_datafile_url,
headers={'Authorization': 'Bearer {datafile_access_token}'.format(
datafile_access_token=datafile_access_token)},
timeout=enums.ConfigManager.REQUEST_TIMEOUT,
timeout=enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT,
)

self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
Expand Down Expand Up @@ -558,7 +589,7 @@ def test_fetch_datafile__request_exception_raised(self, _):
expected_datafile_url,
headers={'Authorization': 'Bearer {datafile_access_token}'.format(
datafile_access_token=datafile_access_token)},
timeout=enums.ConfigManager.REQUEST_TIMEOUT,
timeout=enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT,
)

self.assertIsInstance(project_config_manager.get_config(), project_config.ProjectConfig)
Expand All @@ -577,7 +608,7 @@ def test_fetch_datafile__request_exception_raised(self, _):
'Authorization': 'Bearer {datafile_access_token}'.format(
datafile_access_token=datafile_access_token),
},
timeout=enums.ConfigManager.REQUEST_TIMEOUT,
timeout=enums.ConfigManager.DEFAULT_REQUEST_TIMEOUT,
)
mock_logger.error.assert_called_once_with('Fetching datafile from {} failed. Error: Error Error !!'.format(
expected_datafile_url
Expand Down