diff --git a/packages/http/httpx/kiota_http/httpx_request_adapter.py b/packages/http/httpx/kiota_http/httpx_request_adapter.py index 089199a3..aeefa45b 100644 --- a/packages/http/httpx/kiota_http/httpx_request_adapter.py +++ b/packages/http/httpx/kiota_http/httpx_request_adapter.py @@ -196,7 +196,10 @@ async def send_async( response_handler = self.get_response_handler(request_info) if response_handler: parent_span.add_event(RESPONSE_HANDLER_EVENT_INVOKED_KEY) - return await response_handler.handle_response_async(response, error_map) + handled: Optional[ModelType] = await response_handler.handle_response_async( + response, error_map + ) + return handled await self.throw_failed_responses(response, error_map, parent_span, parent_span) if self._should_return_none(response): @@ -205,10 +208,12 @@ async def send_async( if root_node is None: return None _deserialized_span = self._start_local_tracing_span("get_object_value", parent_span) - value = root_node.get_object_value(parsable_factory) - parent_span.set_attribute(DESERIALIZED_MODEL_NAME_KEY, value.__class__.__name__) - _deserialized_span.end() - return value #type: ignore + try: + value = root_node.get_object_value(parsable_factory) + parent_span.set_attribute(DESERIALIZED_MODEL_NAME_KEY, value.__class__.__name__) + finally: + _deserialized_span.end() + return value finally: parent_span.end() @@ -239,7 +244,10 @@ async def send_collection_async( response_handler = self.get_response_handler(request_info) if response_handler: parent_span.add_event(RESPONSE_HANDLER_EVENT_INVOKED_KEY) - return await response_handler.handle_response_async(response, error_map) + handled: Optional[ + list[ModelType] + ] = await response_handler.handle_response_async(response, error_map) + return handled await self.throw_failed_responses(response, error_map, parent_span, parent_span) if self._should_return_none(response): @@ -248,14 +256,19 @@ async def send_collection_async( _deserialized_span = self._start_local_tracing_span( "get_collection_of_object_values", parent_span ) - root_node = await self.get_root_parse_node(response, parent_span, parent_span) - if root_node: - result: Optional[list[ModelType] - ] = root_node.get_collection_of_object_values(parsable_factory) - parent_span.set_attribute(DESERIALIZED_MODEL_NAME_KEY, result.__class__.__name__) + try: + root_node = await self.get_root_parse_node(response, parent_span, parent_span) + if root_node: + result: Optional[ + list[ModelType] + ] = root_node.get_collection_of_object_values(parsable_factory) + parent_span.set_attribute( + DESERIALIZED_MODEL_NAME_KEY, result.__class__.__name__ + ) + return result + return None + finally: _deserialized_span.end() - return result - return None finally: parent_span.end() @@ -287,7 +300,10 @@ async def send_collection_of_primitive_async( response_handler = self.get_response_handler(request_info) if response_handler: parent_span.add_event(RESPONSE_HANDLER_EVENT_INVOKED_KEY) - return await response_handler.handle_response_async(response, error_map) + handled: Optional[ + list[PrimitiveType] + ] = await response_handler.handle_response_async(response, error_map) + return handled await self.throw_failed_responses(response, error_map, parent_span, parent_span) if self._should_return_none(response): @@ -296,17 +312,21 @@ async def send_collection_of_primitive_async( _deserialized_span = self._start_local_tracing_span( "get_collection_of_primitive_values", parent_span ) - root_node = await self.get_root_parse_node(response, parent_span, parent_span) - if root_node: - values = root_node.get_collection_of_primitive_values(response_type) - parent_span.set_attribute(DESERIALIZED_MODEL_NAME_KEY, values.__class__.__name__) + try: + root_node = await self.get_root_parse_node(response, parent_span, parent_span) + if root_node: + values = root_node.get_collection_of_primitive_values(response_type) + parent_span.set_attribute( + DESERIALIZED_MODEL_NAME_KEY, values.__class__.__name__ + ) + return values + return None + finally: _deserialized_span.end() - return values # type: ignore - return None finally: parent_span.end() - async def send_primitive_async( + async def send_primitive_async( # pylint: disable=too-many-branches self, request_info: RequestInformation, response_type: str, @@ -335,7 +355,10 @@ async def send_primitive_async( response_handler = self.get_response_handler(request_info) if response_handler: parent_span.add_event(RESPONSE_HANDLER_EVENT_INVOKED_KEY) - return await response_handler.handle_response_async(response, error_map) + handled: Optional[ResponseType] = await response_handler.handle_response_async( + response, error_map + ) + return handled await self.throw_failed_responses(response, error_map, parent_span, parent_span) if self._should_return_none(response): @@ -344,28 +367,29 @@ async def send_primitive_async( if response_type == "bytes": return response.content # type: ignore _deserialized_span = self._start_local_tracing_span("get_root_parse_node", parent_span) - root_node = await self.get_root_parse_node(response, parent_span, parent_span) - if not root_node: - return None - value: Optional[Union[str, int, float, bool, datetime, bytes]] = None - if response_type == "str": - value = root_node.get_str_value() - if response_type == "int": - value = root_node.get_int_value() - if response_type == "float": - value = root_node.get_float_value() - if response_type == "bool": - value = root_node.get_bool_value() - if response_type == "datetime": - value = root_node.get_datetime_value() - if value is not None: - parent_span.set_attribute(DESERIALIZED_MODEL_NAME_KEY, value.__class__.__name__) + try: + root_node = await self.get_root_parse_node(response, parent_span, parent_span) + if not root_node: + return None + value: Optional[Union[str, int, float, bool, datetime, bytes]] = None + if response_type == "str": + value = root_node.get_str_value() + if response_type == "int": + value = root_node.get_int_value() + if response_type == "float": + value = root_node.get_float_value() + if response_type == "bool": + value = root_node.get_bool_value() + if response_type == "datetime": + value = root_node.get_datetime_value() + if value is not None: + parent_span.set_attribute(DESERIALIZED_MODEL_NAME_KEY, value.__class__.__name__) + return value # type: ignore + finally: _deserialized_span.end() - return value # type: ignore exc = TypeError(f"Error handling the response, unexpected type {response_type!r}") parent_span.record_exception(exc) - _deserialized_span.end() raise exc finally: @@ -392,7 +416,9 @@ async def send_no_response_content_async( response_handler = self.get_response_handler(request_info) if response_handler: parent_span.add_event(RESPONSE_HANDLER_EVENT_INVOKED_KEY) - return await response_handler.handle_response_async(response, error_map) + # NativeResponseHandler hands the native response back through this return + handled: None = await response_handler.handle_response_async(response, error_map) + return handled await self.throw_failed_responses(response, error_map, parent_span, parent_span) finally: @@ -601,33 +627,34 @@ async def get_http_response_message( _get_http_resp_span = self._start_local_tracing_span( "get_http_response_message", parent_span ) + try: + self.set_base_url_for_request_information(request_info) - self.set_base_url_for_request_information(request_info) - - additional_authentication_context = {} - if claims: - additional_authentication_context[self.CLAIMS_KEY] = claims + additional_authentication_context = {} + if claims: + additional_authentication_context[self.CLAIMS_KEY] = claims - await self._authentication_provider.authenticate_request( - request_info, additional_authentication_context - ) + await self._authentication_provider.authenticate_request( + request_info, additional_authentication_context + ) - request = self.get_request_from_request_information( - request_info, _get_http_resp_span, parent_span - ) - resp = await self._http_client.send(request) - if not resp: - raise ResponseError("Unable to get response from request") - parent_span.set_attribute(HTTP_RESPONSE_STATUS_CODE, resp.status_code) - if http_version := resp.http_version: - parent_span.set_attribute(NETWORK_PROTOCOL_NAME, http_version) - - if content_length := resp.headers.get("Content-Length", None): - parent_span.set_attribute("http.response.body.size", content_length) - - if content_type := resp.headers.get("Content-Type", None): - parent_span.set_attribute("http.response.header.content-type", content_type) - _get_http_resp_span.end() + request = self.get_request_from_request_information( + request_info, _get_http_resp_span, parent_span + ) + resp = await self._http_client.send(request) + if not resp: + raise ResponseError("Unable to get response from request") + parent_span.set_attribute(HTTP_RESPONSE_STATUS_CODE, resp.status_code) + if http_version := resp.http_version: + parent_span.set_attribute(NETWORK_PROTOCOL_NAME, http_version) + + if content_length := resp.headers.get("Content-Length", None): + parent_span.set_attribute("http.response.body.size", content_length) + + if content_type := resp.headers.get("Content-Type", None): + parent_span.set_attribute("http.response.header.content-type", content_type) + finally: + _get_http_resp_span.end() return await self.retry_cae_response_if_required(resp, request_info, claims) async def retry_cae_response_if_required( @@ -676,49 +703,50 @@ def get_request_from_request_information( _get_request_span = self._start_local_tracing_span( "get_request_from_request_information", parent_span ) - url = parse.urlparse(request_info.url) - method = request_info.http_method - if not method: - raise RequestError("HTTP method must be provided") - - otel_attributes = { - HTTP_REQUEST_METHOD: method.value, - SERVER_ADDRESS: url.hostname, - URL_SCHEME: url.scheme, - } - - # A request whose URL was set directly (PageIterator, LargeFileUploadTask) has no template - if request_info.url_template is not None: - otel_attributes["url.uri_template"] = request_info.url_template - if url.port is not None: - otel_attributes["http.port"] = str(url.port) - - if self.observability_options.include_euii_attributes: - otel_attributes.update({URL_FULL: url.geturl()}) - - request_options = { - self.observability_options.get_key(): self.observability_options, - "parent_span": parent_span, - **request_info.request_options, - } - request = self._http_client.build_request( - method=method.value, - url=request_info.url, - headers=request_info.request_headers, - content=request_info.content, - extensions={REQUEST_OPTIONS_KEY: request_options}, - ) - - if content_length := request.headers.get("Content-Length", None): - otel_attributes.update({"http.request.body.size": content_length}) + try: + url = parse.urlparse(request_info.url) + method = request_info.http_method + if not method: + raise RequestError("HTTP method must be provided") + + otel_attributes = { + HTTP_REQUEST_METHOD: method.value, + SERVER_ADDRESS: url.hostname, + URL_SCHEME: url.scheme, + } + + # A URL set directly (PageIterator, LargeFileUploadTask) has no template + if request_info.url_template is not None: + otel_attributes["url.uri_template"] = request_info.url_template + if url.port is not None: + otel_attributes["http.port"] = str(url.port) + + if self.observability_options.include_euii_attributes: + otel_attributes.update({URL_FULL: url.geturl()}) + + request_options = { + self.observability_options.get_key(): self.observability_options, + "parent_span": parent_span, + **request_info.request_options, + } + request = self._http_client.build_request( + method=method.value, + url=request_info.url, + headers=request_info.request_headers, + content=request_info.content, + extensions={REQUEST_OPTIONS_KEY: request_options}, + ) - if content_type := request.headers.get("Content-Type", None): - otel_attributes.update({"http.request.header.content-type": content_type}) - attribute_span.set_attributes(otel_attributes) # type: ignore - _get_request_span.set_attributes(otel_attributes) # type: ignore - _get_request_span.end() + if content_length := request.headers.get("Content-Length", None): + otel_attributes.update({"http.request.body.size": content_length}) - return request + if content_type := request.headers.get("Content-Type", None): + otel_attributes.update({"http.request.header.content-type": content_type}) + attribute_span.set_attributes(otel_attributes) + _get_request_span.set_attributes(otel_attributes) + return request + finally: + _get_request_span.end() async def convert_to_native_async(self, request_info: RequestInformation) -> httpx.Request: parent_span = self.start_tracing_span(request_info, "convert_to_native_async") diff --git a/packages/http/httpx/kiota_http/kiota_client_factory.py b/packages/http/httpx/kiota_http/kiota_client_factory.py index afa75eb3..3b3b30c9 100644 --- a/packages/http/httpx/kiota_http/kiota_client_factory.py +++ b/packages/http/httpx/kiota_http/kiota_client_factory.py @@ -173,11 +173,9 @@ def _load_middleware_to_client( current_transport, middleware ) if client._mounts: - mounts: dict = {} + mounts = dict(client._mounts) for pattern, transport in client._mounts.items(): - if transport is None: - mounts[pattern] = None - else: + if transport is not None: mounts[pattern ] = KiotaClientFactory._replace_transport_with_custom_kiota_transport( transport, middleware diff --git a/packages/http/httpx/kiota_http/observability_options.py b/packages/http/httpx/kiota_http/observability_options.py index 55951307..e3f96c6c 100644 --- a/packages/http/httpx/kiota_http/observability_options.py +++ b/packages/http/httpx/kiota_http/observability_options.py @@ -23,7 +23,7 @@ def enabled(self) -> bool: return self._enabled @enabled.setter - def enabled(self, value: bool): + def enabled(self, value: bool) -> None: """sets whether to enable ObservabilityOptions in the middleware chain.""" self._enabled = value diff --git a/packages/http/httpx/pyproject.toml b/packages/http/httpx/pyproject.toml index dd0133dc..d8961b40 100644 --- a/packages/http/httpx/pyproject.toml +++ b/packages/http/httpx/pyproject.toml @@ -38,6 +38,7 @@ name = "kiota_http" [tool.mypy] warn_unused_configs = true +strict = true files = "kiota_http" [tool.yapf] diff --git a/packages/http/httpx/tests/test_httpx_request_adapter.py b/packages/http/httpx/tests/test_httpx_request_adapter.py index 9867d519..cf6938fe 100644 --- a/packages/http/httpx/tests/test_httpx_request_adapter.py +++ b/packages/http/httpx/tests/test_httpx_request_adapter.py @@ -18,6 +18,7 @@ from opentelemetry.sdk.trace import SpanProcessor, TracerProvider import kiota_http.httpx_request_adapter +from kiota_http._exceptions import RequestError from kiota_http.httpx_request_adapter import HttpxRequestAdapter from kiota_http.middleware import REQUEST_OPTIONS_KEY from kiota_http.middleware.options import ResponseHandlerOption @@ -81,6 +82,80 @@ async def retry(info, span, claims): assert spans[0].events[0].name == "com.microsoft.kiota.authenticate_challenge_received" + +@pytest.mark.asyncio +async def test_http_response_message_span_ends_on_transport_failure( + auth_provider, request_info, span_exporter +): + + def request_handler(request): + raise httpx.ReadError("connection reset") + + client = httpx.AsyncClient(transport=httpx.MockTransport(request_handler)) + request_adapter = HttpxRequestAdapter(auth_provider, http_client=client) + request_info.http_method = Method.GET + request_info.url = BASE_URL + with pytest.raises(httpx.ReadError): + await request_adapter.send_no_response_content_async(request_info, None) + assert [span.name for span in span_exporter.get_finished_spans()] == [ + "get_request_from_request_information", "get_http_response_message", + "send_no_response_content_async - UNKNOWN" + ] + + +def test_request_span_ends_without_http_method( + request_adapter, request_info, mock_otel_span, span_exporter +): + request_info.url = BASE_URL + with pytest.raises(RequestError): + request_adapter.get_request_from_request_information( + request_info, mock_otel_span, mock_otel_span + ) + assert [span.name for span in span_exporter.get_finished_spans() + ] == ["get_request_from_request_information"] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("send, response_type, span_names", [ + ( + HttpxRequestAdapter.send_collection_async, MockResponseObject, + ["get_root_parse_node", "get_collection_of_object_values", "send_collection_async - UNKNOWN"] + ), + ( + HttpxRequestAdapter.send_collection_of_primitive_async, int, [ + "get_root_parse_node", "get_collection_of_primitive_values", + "send_collection_of_primitive_async - UNKNOWN" + ] + ), + ( + HttpxRequestAdapter.send_primitive_async, "int", + ["get_root_parse_node", "get_root_parse_node", "send_primitive_async - UNKNOWN"] + ), +]) +async def test_deserialization_span_ends_without_content_type( + send, response_type, span_names, request_adapter, request_info, + mock_primitive_response_with_no_content_type_header, span_exporter +): + request_adapter.get_http_response_message = AsyncMock( + return_value=mock_primitive_response_with_no_content_type_header + ) + assert await send(request_adapter, request_info, response_type, None) is None + assert [span.name for span in span_exporter.get_finished_spans()] == span_names + + +@pytest.mark.asyncio +async def test_deserialization_span_ends_when_deserialization_fails( + request_adapter, request_info, mock_user_response, span_exporter +): + root_node = Mock() + root_node.get_object_value.side_effect = ValueError("unreadable payload") + request_adapter.get_http_response_message = AsyncMock(return_value=mock_user_response) + request_adapter.get_root_parse_node = AsyncMock(return_value=root_node) + with pytest.raises(ValueError): + await request_adapter.send_async(request_info, MockResponseObject, None) + assert [span.name for span in span_exporter.get_finished_spans() + ] == ["get_object_value", "send_async - UNKNOWN"] + def test_create_request_adapter(auth_provider): request_adapter = HttpxRequestAdapter(auth_provider) assert request_adapter._authentication_provider is auth_provider