Skip to content

feat: fetch timeout made configurable #411

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

Merged
merged 17 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
8 changes: 7 additions & 1 deletion optimizely/helpers/sdk_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ def __init__(
segments_cache_timeout_in_secs: int = enums.OdpSegmentsCacheConfig.DEFAULT_TIMEOUT_SECS,
odp_segments_cache: Optional[OptimizelySegmentsCache] = None,
odp_segment_manager: Optional[OdpSegmentManager] = None,
odp_event_manager: Optional[OdpEventManager] = None
odp_event_manager: Optional[OdpEventManager] = None,
fetch_segments_timeout: Optional[int] = None,
odp_event_timeout: Optional[int] = None
) -> None:
"""
Args:
Expand All @@ -45,6 +47,8 @@ def __init__(
`fetch_qualified_segments(user_key, user_value, options)`.
odp_event_manager: A custom odp event manager. Required method is:
`send_event(type:, action:, identifiers:, data:)`
fetch_segments_timeout: A fetch segment timeout in seconds (optional).
odp_event_timeout: A send odp event timeout in seconds (optional).
"""

self.odp_disabled = odp_disabled
Expand All @@ -53,3 +57,5 @@ def __init__(
self.segments_cache = odp_segments_cache
self.odp_segment_manager = odp_segment_manager
self.odp_event_manager = odp_event_manager
self.fetch_segments_timeout = fetch_segments_timeout
self.odp_event_timeout = odp_event_timeout
10 changes: 7 additions & 3 deletions optimizely/odp/odp_event_api_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,14 @@
class OdpEventApiManager:
"""Provides an internal service for ODP event REST api access."""

def __init__(self, logger: Optional[optimizely_logger.Logger] = None):
def __init__(self, logger: Optional[optimizely_logger.Logger] = None, timeout: Optional[int] = None):
self.logger = logger or optimizely_logger.NoOpLogger()
self.timeout = timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.timeout = timeout
self.timeout = timeout or OdpEventApiConfig.REQUEST_TIMEOUT

Might as well just do this once.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1


def send_odp_events(self, api_key: str, api_host: str, events: list[OdpEvent]) -> bool:
def send_odp_events(self,
Copy link
Contributor

Choose a reason for hiding this comment

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

What about passing the timeout value when initializing? We do not need to send timeout for every event. It's not expected we need event-by-event control. Also a custom OdpEventApiManager can have own timeout control.

api_key: str,
api_host: str,
events: list[OdpEvent]) -> bool:
"""
Dispatch the event being represented by the OdpEvent object.

Expand All @@ -69,7 +73,7 @@ def send_odp_events(self, api_key: str, api_host: str, events: list[OdpEvent]) -
response = requests.post(url=url,
headers=request_headers,
data=payload_dict,
timeout=OdpEventApiConfig.REQUEST_TIMEOUT)
timeout=self.timeout or OdpEventApiConfig.REQUEST_TIMEOUT)

response.raise_for_status()

Expand Down
10 changes: 7 additions & 3 deletions optimizely/odp/odp_event_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,18 @@ class OdpEventManager:
def __init__(
self,
logger: Optional[_logging.Logger] = None,
api_manager: Optional[OdpEventApiManager] = None
api_manager: Optional[OdpEventApiManager] = None,
timeout: Optional[int] = None
):
"""OdpEventManager init method to configure event batching.

Args:
logger: Optional component which provides a log method to log messages. By default nothing would be logged.
api_manager: Optional component which sends events to ODP.
timeout: Optional event timeout in seconds.
"""
self.logger = logger or _logging.NoOpLogger()
self.api_manager = api_manager or OdpEventApiManager(self.logger)
self.api_manager = api_manager or OdpEventApiManager(self.logger, timeout)

self.odp_config: Optional[OdpConfig] = None
self.api_key: Optional[str] = None
Expand Down Expand Up @@ -158,7 +160,9 @@ def _flush_batch(self) -> None:

for i in range(1 + self.retry_count):
try:
should_retry = self.api_manager.send_odp_events(self.api_key, self.api_host, self._current_batch)
should_retry = self.api_manager.send_odp_events(self.api_key,
self.api_host,
self._current_batch)
except Exception as error:
should_retry = False
self.logger.error(Errors.ODP_EVENT_FAILED.format(f'Error: {error} {self._current_batch}'))
Expand Down
7 changes: 5 additions & 2 deletions optimizely/odp/odp_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ def __init__(
segments_cache: Optional[OptimizelySegmentsCache] = None,
segment_manager: Optional[OdpSegmentManager] = None,
event_manager: Optional[OdpEventManager] = None,
fetch_segments_timeout: Optional[int] = None,
odp_event_timeout: Optional[int] = None,
logger: Optional[optimizely_logger.Logger] = None
) -> None:

Expand All @@ -42,6 +44,7 @@ def __init__(

self.segment_manager = segment_manager
self.event_manager = event_manager
self.fetch_segments_timeout = fetch_segments_timeout

if not self.enabled:
self.logger.info('ODP is disabled.')
Expand All @@ -53,9 +56,9 @@ def __init__(
OdpSegmentsCacheConfig.DEFAULT_CAPACITY,
OdpSegmentsCacheConfig.DEFAULT_TIMEOUT_SECS
)
self.segment_manager = OdpSegmentManager(segments_cache, logger=self.logger)
self.segment_manager = OdpSegmentManager(segments_cache, logger=self.logger, timeout=fetch_segments_timeout)

self.event_manager = self.event_manager or OdpEventManager(self.logger)
self.event_manager = self.event_manager or OdpEventManager(self.logger, odp_event_timeout)
self.segment_manager.odp_config = self.odp_config

def fetch_qualified_segments(self, user_id: str, options: list[str]) -> Optional[list[str]]:
Expand Down
5 changes: 3 additions & 2 deletions optimizely/odp/odp_segment_api_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,9 @@
class OdpSegmentApiManager:
"""Interface for manging the fetching of audience segments."""

def __init__(self, logger: Optional[optimizely_logger.Logger] = None):
def __init__(self, logger: Optional[optimizely_logger.Logger] = None, timeout: Optional[int] = None):
self.logger = logger or optimizely_logger.NoOpLogger()
self.timeout = timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.timeout = timeout
self.timeout = timeout or OdpSegmentApiConfig.REQUEST_TIMEOUT


def fetch_segments(self, api_key: str, api_host: str, user_key: str,
user_value: str, segments_to_check: list[str]) -> Optional[list[str]]:
Expand Down Expand Up @@ -151,7 +152,7 @@ def fetch_segments(self, api_key: str, api_host: str, user_key: str,
response = requests.post(url=url,
headers=request_headers,
data=payload_dict,
timeout=OdpSegmentApiConfig.REQUEST_TIMEOUT)
timeout=self.timeout or OdpSegmentApiConfig.REQUEST_TIMEOUT)

response.raise_for_status()
response_dict = response.json()
Expand Down
8 changes: 4 additions & 4 deletions optimizely/odp/odp_segment_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,16 @@ def __init__(
self,
segments_cache: OptimizelySegmentsCache,
api_manager: Optional[OdpSegmentApiManager] = None,
logger: Optional[optimizely_logger.Logger] = None
logger: Optional[optimizely_logger.Logger] = None,
timeout: Optional[int] = None
) -> None:

self.odp_config: Optional[OdpConfig] = None
self.segments_cache = segments_cache
self.logger = logger or optimizely_logger.NoOpLogger()
self.api_manager = api_manager or OdpSegmentApiManager(self.logger)
self.api_manager = api_manager or OdpSegmentApiManager(self.logger, timeout)

def fetch_qualified_segments(self, user_key: str, user_value: str, options: list[str]
) -> Optional[list[str]]:
def fetch_qualified_segments(self, user_key: str, user_value: str, options: list[str]) -> Optional[list[str]]:
"""
Args:
user_key: The key for identifying the id type.
Expand Down
2 changes: 2 additions & 0 deletions optimizely/optimizely.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ def __init__(
self.sdk_settings.segments_cache,
self.sdk_settings.odp_segment_manager,
self.sdk_settings.odp_event_manager,
self.sdk_settings.fetch_segments_timeout,
self.sdk_settings.odp_event_timeout,
self.logger
)

Expand Down
13 changes: 13 additions & 0 deletions tests/test_odp_event_api_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@ def test_send_odp_events__valid_request(self):
data=json.dumps(self.events, cls=OdpEventEncoder),
timeout=OdpEventApiConfig.REQUEST_TIMEOUT)

def test_send_odp_events__custom_timeout(self):
with mock.patch('requests.post') as mock_request_post:
api = OdpEventApiManager(timeout=14)
api.send_odp_events(api_key=self.api_key,
api_host=self.api_host,
events=self.events)

request_headers = {'content-type': 'application/json', 'x-api-key': self.api_key}
mock_request_post.assert_called_once_with(url=self.api_host + "/v3/events",
headers=request_headers,
data=json.dumps(self.events, cls=OdpEventEncoder),
timeout=14)

def test_send_odp_ovents_success(self):
with mock.patch('requests.post') as mock_request_post:
# no need to mock url and content because we're not returning the response
Expand Down
2 changes: 1 addition & 1 deletion tests/test_odp_event_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class OdpEventManagerTest(BaseTest):
"key-3": 3.0,
"key-4": None,
"key-5": True
}
},
},
{
"type": "t2",
Expand Down
21 changes: 21 additions & 0 deletions tests/test_odp_segment_api_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,27 @@ def test_fetch_qualified_segments__valid_request(self):
data=json.dumps(test_payload),
timeout=OdpSegmentApiConfig.REQUEST_TIMEOUT)

def test_fetch_qualified_segments__custom_timeout(self):
with mock.patch('requests.post') as mock_request_post:
api = OdpSegmentApiManager(timeout=12)
api.fetch_segments(api_key=self.api_key,
api_host=self.api_host,
user_key=self.user_key,
user_value=self.user_value,
segments_to_check=["a", "b", "c"])

test_payload = {
'query': 'query($userId: String, $audiences: [String]) {'
'customer(vuid: $userId) '
'{audiences(subset: $audiences) {edges {node {name state}}}}}',
'variables': {'userId': self.user_value, 'audiences': ["a", "b", "c"]}
}
request_headers = {'content-type': 'application/json', 'x-api-key': self.api_key}
mock_request_post.assert_called_once_with(url=self.api_host + "/v3/graphql",
headers=request_headers,
data=json.dumps(test_payload),
timeout=12)

def test_fetch_qualified_segments__success(self):
with mock.patch('requests.post') as mock_request_post:
mock_request_post.return_value = \
Expand Down