Repository navigation
refactor: extract requests transport #469
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
Changes from all commits
6c77ae6
3cc3037
d9fb8e5
b4a2d6d
9d39224
de6e361
1abab9a
73b33e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| """ | ||
| HTTP layer under the Unleash API client: sends one request and returns the | ||
| response. | ||
| """ | ||
|
|
||
| from dataclasses import dataclass, field | ||
| from typing import Any, Mapping, Optional, Protocol, runtime_checkable | ||
|
|
||
| import requests | ||
| from requests.adapters import HTTPAdapter | ||
| from urllib3 import Retry | ||
|
|
||
| from UnleashClient.constants import REQUEST_TIMEOUT | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class Request: | ||
| """ | ||
| HTTP request. | ||
|
|
||
| :param method: ``"GET"`` or ``"POST"``. | ||
| :param url: Absolute url, without the query string. | ||
| :param headers: Headers to send. | ||
| :param params: Query string parameters. | ||
| :param body: Request body, or None to send none. | ||
| """ | ||
|
|
||
| method: str | ||
| url: str | ||
| headers: Mapping[str, str] | ||
| params: Mapping[str, str] = field(default_factory=dict) | ||
| body: Optional[bytes] = None | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class Response: | ||
| """ | ||
| HTTP response. Every status is a response, not an error: deciding what | ||
| a status means is the API client's job. | ||
|
|
||
| :param status: HTTP status code. | ||
| :param headers: Response headers, looked up case-insensitively. | ||
| :param body: Raw response body. | ||
| """ | ||
|
|
||
| status: int | ||
| headers: Mapping[str, str] | ||
| body: bytes | ||
|
|
||
|
|
||
| @runtime_checkable | ||
| class Transport(Protocol): | ||
| """Sends requests for the sync client.""" | ||
|
|
||
| def execute(self, request: Request) -> Response: | ||
| """ | ||
| Send ``request`` once and return the response, whatever its status. | ||
|
|
||
| :raises Exception: only when no response arrived, e.g. a connection | ||
| error or a timeout. | ||
| """ | ||
|
|
||
|
|
||
| class RequestsTransport: | ||
| """ | ||
| Sends requests with ``requests``, on a fresh session for every request. | ||
|
|
||
| :param timeout: Seconds to wait for the connection and for each read. | ||
| :param _retries: GET requests are retried this many times on a connection | ||
| error or a 500, 502 or 504, by urllib3. | ||
| :param _request_kwargs: extra keyword arguments for every ``requests`` call. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| *, | ||
| timeout: float = REQUEST_TIMEOUT, | ||
| # these two are are going away too | ||
| _retries: Optional[int] = None, | ||
| _request_kwargs: Optional[Mapping[str, Any]] = None, | ||
| ) -> None: | ||
| self._timeout = timeout | ||
| self._retries = _retries | ||
| self._request_kwargs = dict(_request_kwargs or {}) | ||
|
|
||
| def execute(self, request: Request) -> Response: | ||
| kwargs: dict = {"headers": dict(request.headers), "timeout": self._timeout} | ||
| if request.params: | ||
| kwargs["params"] = dict(request.params) | ||
| if request.body is not None: | ||
| kwargs["data"] = request.body | ||
|
|
||
| with requests.Session() as session: | ||
| if request.method == "GET" and self._retries is not None: | ||
| adapter = HTTPAdapter( | ||
| max_retries=Retry( | ||
| total=self._retries, status_forcelist=[500, 502, 504] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. legit, and this is no behavior change at this point, urllib3's Retry raises RetryError once retries run out on a 5xx |
||
| ) | ||
| ) | ||
| session.mount("https://", adapter) | ||
| session.mount("http://", adapter) | ||
|
|
||
| resp = session.request( | ||
| request.method, request.url, **kwargs, **self._request_kwargs | ||
| ) | ||
| return Response( | ||
| status=resp.status_code, headers=resp.headers, body=resp.content | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,11 @@ | |
| from typing import Any, Dict, NamedTuple, Optional, Tuple | ||
|
|
||
| import requests | ||
| from requests import Response | ||
| from requests.adapters import HTTPAdapter | ||
| from requests.exceptions import InvalidHeader, InvalidSchema, InvalidURL, MissingSchema | ||
| from urllib3 import Retry | ||
|
|
||
| from UnleashClient._event_dispatcher import _EventDispatcher | ||
| from UnleashClient._headers import _HeaderFactory | ||
| from UnleashClient._transport import Request, RequestsTransport, Response, Transport | ||
| from UnleashClient.config import PollingOptions, UnleashConfig | ||
| from UnleashClient.constants import FEATURES_URL, METRICS_URL, REGISTER_URL | ||
| from UnleashClient.events import ( | ||
|
|
@@ -78,31 +76,47 @@ def _normalized_url(url: str, path: str) -> str: | |
| return f"{url}{path}" | ||
|
|
||
|
|
||
| def _text(resp: Response) -> str: | ||
| return resp.body.decode("utf-8", errors="replace") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i am not entirely sure how much of a real life scenario this is, request also uses utf-8 be default and I think unleash server also sends utf-8 |
||
|
|
||
|
|
||
| def _log_resp_info(resp: Response) -> None: | ||
| LOGGER.debug("HTTP status code: %s", resp.status_code) | ||
| LOGGER.debug("HTTP status code: %s", resp.status) | ||
| LOGGER.debug("HTTP headers: %s", resp.headers) | ||
| LOGGER.debug("HTTP content: %s", resp.text) | ||
| LOGGER.debug("HTTP content: %s", _text(resp)) | ||
|
|
||
|
|
||
| class _UnleashApiClient: | ||
| """ | ||
| Sends the SDK's requests to the Unleash server: feature fetches, client | ||
| registration and metrics submission. | ||
| Talks to the Unleash API: feature fetches, client registration and metrics | ||
| submission. Builds each request and decides what its response means; the | ||
| transport only sends it. | ||
|
|
||
| :param config: Client configuration. | ||
| :param headers: Builds the header set each endpoint needs. | ||
| :param events: Receives ``FETCH_COMPLETED`` after every feature fetch. | ||
| :param transport: Sends the requests. Defaults to a :class:`RequestsTransport` | ||
| built from ``config.http``. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| config: UnleashConfig, | ||
| headers: _HeaderFactory, | ||
| events: Optional[_EventDispatcher] = None, | ||
| transport: Optional[Transport] = None, | ||
| ) -> None: | ||
| self._config: UnleashConfig = config | ||
| self._headers: _HeaderFactory = headers | ||
| self._events: Optional[_EventDispatcher] = events | ||
|
|
||
| self._transport: Transport = transport or RequestsTransport( | ||
| timeout=config.http.timeout, | ||
| _retries=config.http.retries, | ||
| # this is going to go away soon, just like the whole httpOptions in the client constructor | ||
| _request_kwargs=config.http.options, | ||
| ) | ||
|
|
||
| # pylint: disable=broad-except | ||
| def fetch_features(self, etag: str = "", *, options: PollingOptions) -> FetchResult: | ||
| """ | ||
|
|
@@ -146,31 +160,24 @@ def _fetch_features( | |
| if options.project_name: | ||
| base_params = {"project": options.project_name} | ||
|
|
||
| adapter = HTTPAdapter( | ||
| max_retries=Retry( | ||
| total=config.http.retries, status_forcelist=[500, 502, 504] | ||
| ) | ||
| ) | ||
| with requests.Session() as session: | ||
| session.mount("https://", adapter) | ||
| session.mount("http://", adapter) | ||
| resp = session.get( | ||
| base_url, | ||
| resp = self._transport.execute( | ||
| Request( | ||
| method="GET", | ||
| url=base_url, | ||
| headers={ | ||
| **self._headers.polling(options), | ||
| **request_specific_headers, | ||
| }, | ||
| params=base_params, | ||
| timeout=config.http.timeout, | ||
| **config.http.options, | ||
| ) | ||
| ) | ||
|
|
||
| http_status_code = resp.status_code | ||
| if resp.status_code not in [200, 304]: | ||
| http_status_code = resp.status | ||
| if resp.status not in [200, 304]: | ||
| _log_resp_info(resp) | ||
| LOGGER.warning( | ||
| "Unleash Client feature fetch failed due to unexpected HTTP status code: %s", | ||
| resp.status_code, | ||
| resp.status, | ||
| ) | ||
| raise Exception( | ||
| "Unleash Client feature fetch failed!" | ||
|
|
@@ -180,13 +187,14 @@ def _fetch_features( | |
| if "etag" in resp.headers.keys(): | ||
| fetched_etag = resp.headers["etag"] | ||
|
|
||
| if resp.status_code == 304: | ||
| if resp.status == 304: | ||
| return ( | ||
| FetchResult(None, fetched_etag, not_modified=True), | ||
| FetchStatus.NOT_CHANGED, | ||
| ) | ||
|
|
||
| return FetchResult(resp.text, fetched_etag), _fetched_status(resp.text) | ||
| raw_state = _text(resp) | ||
| return FetchResult(raw_state, fetched_etag), _fetched_status(raw_state) | ||
| except Exception as exc: | ||
| LOGGER.exception( | ||
| "Unleash Client feature fetch failed due to exception: %s", exc | ||
|
|
@@ -212,19 +220,20 @@ def register(self, payload: Dict[str, Any]) -> bool: | |
| LOGGER.info("Registering unleash client with unleash @ %s", config.url) | ||
| LOGGER.info("Registration request information: %s", payload) | ||
|
|
||
| resp = requests.post( | ||
| _normalized_url(config.url, REGISTER_URL), | ||
| data=json.dumps(payload), | ||
| headers=self._headers.base(), | ||
| timeout=config.http.timeout, | ||
| **config.http.options, | ||
| resp = self._transport.execute( | ||
| Request( | ||
| method="POST", | ||
| url=_normalized_url(config.url, REGISTER_URL), | ||
| headers=self._headers.base(), | ||
| body=json.dumps(payload).encode("utf-8"), | ||
| ) | ||
| ) | ||
|
|
||
| if resp.status_code not in {200, 202}: | ||
| if resp.status not in {200, 202}: | ||
| _log_resp_info(resp) | ||
| LOGGER.warning( | ||
| "Unleash Client registration failed due to unexpected HTTP status code: %s", | ||
| resp.status_code, | ||
| resp.status, | ||
| ) | ||
| return False | ||
|
|
||
|
|
@@ -255,19 +264,20 @@ def send_metrics(self, payload: Dict[str, Any]) -> bool: | |
| LOGGER.info("Sending messages to with unleash @ %s", config.url) | ||
| LOGGER.info("unleash metrics information: %s", payload) | ||
|
|
||
| resp = requests.post( | ||
| _normalized_url(config.url, METRICS_URL), | ||
| data=json.dumps(payload), | ||
| headers=self._headers.metrics(), | ||
| timeout=config.http.timeout, | ||
| **config.http.options, | ||
| resp = self._transport.execute( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is also legit and also fixed in a later PR register() and send_metrics() log and return False, and fetch reports UNAVAILABLE in case of any error from |
||
| Request( | ||
| method="POST", | ||
| url=_normalized_url(config.url, METRICS_URL), | ||
| headers=self._headers.metrics(), | ||
| body=json.dumps(payload).encode("utf-8"), | ||
| ) | ||
| ) | ||
|
|
||
| if resp.status_code != 202: | ||
| if resp.status != 202: | ||
| _log_resp_info(resp) | ||
| LOGGER.warning( | ||
| "Unleash Client metrics submission due to unexpected HTTP status code: %s", | ||
| resp.status_code, | ||
| resp.status, | ||
| ) | ||
| return False | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a legit concern, but transport is private at this point, and the case insensivity problem is fixed in #473, #473 fixes it in the API client rather than in Response