-
Notifications
You must be signed in to change notification settings - Fork 172
updated httpx integration, patches #1102 #1109
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
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
ResponseHook = Callable[[Span, RequestInfo, ResponseInfo], None] | ||
AsyncRequestHook = Callable[[Span, RequestInfo], Awaitable[None]] | ||
AsyncResponseHook = Callable[[Span, RequestInfo, ResponseInfo], Awaitable[None]] | ||
from opentelemetry.instrumentation.httpx import ( |
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.
Need to be careful that these only get imported when type checking to not break for older versions of the instrumentation library.
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.
tests break with if TYPE_CHECKING:
.
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.
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?
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.
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.
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.
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.
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.
Understood!
|
||
class LogfireHttpxInfoMixin: | ||
headers: httpx.Headers | ||
headers: httpx.Headers | None |
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.
Rather make this an empty Headers object than let it be None. Definitely don't raise errors.
Closing since I think we can't actually make this change for a while based on discussion |
References #1102. I deleted the branch by mistake and so I am reopening the pull request.
Context
I was working on #1059 when I came across a TODO in the httpx integration (logfire/integrations/httpx.py) made by @Kludex that was waiting on this merge. The merged happened so I went ahead and made the changes.
How I tested it
I ran
uv run pytest -s tests/otel-integrations/test_httpx.py
and all tests passed.Types of Changes