Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
15 changes: 10 additions & 5 deletions logfire/_internal/integrations/httpx.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,26 +148,30 @@
)

tracer_provider = final_kwargs['tracer_provider']
instrumentor.instrument_client(client, tracer_provider, request_hook, response_hook) # type: ignore[reportArgumentType]
instrumentor.instrument_client(client, tracer_provider, request_hook, response_hook)


class LogfireHttpxInfoMixin:
headers: httpx.Headers
headers: httpx.Headers | None
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather make this an empty Headers object than let it be None. Definitely don't raise errors.


@property
def content_type_header_object(self) -> ContentTypeHeader:
return content_type_header_from_string(self.content_type_header_string)

@property
def content_type_header_string(self) -> str:
return self.headers.get('content-type', '')
if self.headers:
return self.headers.get('content-type', '')

raise ValueError(f"'headers' is None in {repr(self)}")

Check warning on line 166 in logfire/_internal/integrations/httpx.py

View check run for this annotation

Codecov / codecov/patch

logfire/_internal/integrations/httpx.py#L166

Added line #L166 was not covered by tests


class LogfireHttpxRequestInfo(RequestInfo, LogfireHttpxInfoMixin):
span: Span

def capture_headers(self):
capture_request_or_response_headers(self.span, self.headers, 'request')
if self.headers:
capture_request_or_response_headers(self.span, self.headers, 'request')

def capture_body(self):
captured_form = self.capture_body_if_form()
Expand Down Expand Up @@ -234,7 +238,8 @@
is_async: bool

def capture_headers(self):
capture_request_or_response_headers(self.span, self.headers, 'response')
if self.headers:
capture_request_or_response_headers(self.span, self.headers, 'response')

def capture_body_if_text(self, attr_name: str = 'http.response.body.text'):
def hook(span: LogfireSpan):
Expand Down
50 changes: 8 additions & 42 deletions logfire/integrations/httpx.py
Original file line number Diff line number Diff line change
@@ -1,44 +1,10 @@
from __future__ import annotations

from typing import Any, Awaitable, Callable, NamedTuple

import httpx
from opentelemetry.trace import Span

# TODO(Marcelo): When https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3098/ gets merged,
# and the next version of `opentelemetry-instrumentation-httpx` is released, we can just do a reimport:
# from opentelemetry.instrumentation.httpx import RequestInfo as RequestInfo
# from opentelemetry.instrumentation.httpx import ResponseInfo as ResponseInfo
# from opentelemetry.instrumentation.httpx import RequestHook as RequestHook
# from opentelemetry.instrumentation.httpx import ResponseHook as ResponseHook


class RequestInfo(NamedTuple):
"""Information about an HTTP request.
This is the second parameter passed to the `RequestHook` function.
"""

method: bytes
url: httpx.URL
headers: httpx.Headers
stream: httpx.SyncByteStream | httpx.AsyncByteStream | None
extensions: dict[str, Any] | None


class ResponseInfo(NamedTuple):
"""Information about an HTTP response.
This is the second parameter passed to the `ResponseHook` function.
"""

status_code: int
headers: httpx.Headers
stream: httpx.SyncByteStream | httpx.AsyncByteStream | None
extensions: dict[str, Any] | None


RequestHook = Callable[[Span, RequestInfo], None]
ResponseHook = Callable[[Span, RequestInfo, ResponseInfo], None]
AsyncRequestHook = Callable[[Span, RequestInfo], Awaitable[None]]
AsyncResponseHook = Callable[[Span, RequestInfo, ResponseInfo], Awaitable[None]]
from opentelemetry.instrumentation.httpx import (
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 be careful that these only get imported when type checking to not break for older versions of the instrumentation library.

Copy link
Author

Choose a reason for hiding this comment

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

tests break with if TYPE_CHECKING:.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you just put if TYPE_CHECKING: here then I expect breakage because another file imports from here. But what if you do the same for that file? Are these types used at runtime?

Copy link
Author

Choose a reason for hiding this comment

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

I believe so. From what I understand they are used at the httpx integration at runtime and they are also used when type checking in main.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, i see now. then I think this isn't actually worth it unless/until the httpx integration doesn't work with older versions of the otel library for other reasons.

Copy link
Author

Choose a reason for hiding this comment

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

Understood!

AsyncRequestHook as AsyncRequestHook,
AsyncResponseHook as AsyncResponseHook,
RequestHook as RequestHook,
RequestInfo as RequestInfo,
ResponseHook as ResponseHook,
ResponseInfo as ResponseInfo,
)
Loading