-
Notifications
You must be signed in to change notification settings - Fork 77
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
Capture httpx response JSON bodies #700
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 137 137
Lines 10929 11008 +79
Branches 1524 1537 +13
=========================================
+ Hits 10929 11008 +79 ☔ View full report in Codecov by Sentry. |
Deploying logfire-docs with Cloudflare Pages
|
'http.request.header.host': ('example.org',), | ||
'http.request.header.accept': ('*/*',), | ||
'http.request.header.accept-encoding': ('gzip, deflate',), | ||
'http.request.header.connection': ('keep-alive',), | ||
'http.request.header.user-agent': (IsStr(),), | ||
'http.request.header.content-length': (IsStr(),), | ||
'http.request.header.content-type': ('application/json',), | ||
'logfire.json_schema': '{"type":"object","properties":{"http.request.body.json":{"type":"object"}}}', | ||
'http.request.body.json': '{"hello":"world"}', | ||
'http.status_code': 200, | ||
'http.response.status_code': 200, | ||
'http.flavor': '1.1', | ||
'network.protocol.version': '1.1', | ||
'http.response.header.host': ('example.org',), | ||
'http.response.header.accept': ('*/*',), | ||
'http.response.header.accept-encoding': ('gzip, deflate',), | ||
'http.response.header.connection': ('keep-alive',), | ||
'http.response.header.user-agent': (IsStr(),), | ||
'http.response.header.content-length': (IsStr(),), | ||
'http.response.header.content-type': ('application/json',), | ||
'http.response.header.traceparent': ('00-00000000000000000000000000000001-0000000000000003-01',), |
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.
Isn't it a bit weird that you have both the information about the request headers, body and response headers in a single span, but the response body on another one? 🤔
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.
By the time the body is read, the main span is already closed, which is why a new span is needed.
The best we can do for now is to copy the attributes from the main span into the new span so that the new span has everything. But we can't put the body in the main span.
await run_async_hook(hook, span, request, response) | ||
|
||
return new_hook | ||
|
||
|
||
def capture_response_json(logfire_instance: Logfire, response_info: ResponseInfo, is_async: bool) -> None: | ||
headers = cast('httpx.Headers', response_info.headers) | ||
if not headers.get('content-type', '').lower().startswith('application/json'): |
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 think the lower
is redundant.
No description provided.