Add HTTP/2 support and protocol version logging#232
Conversation
- feat: enable HTTP/2 negotiation on the synchronous httpx handler - feat: add http_version field to ResponseInfo - feat: log negotiated protocol version at DEBUG level across all request handlers
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| logger.debug( | ||
| "PubNub request completed: operation=%s protocol=%s" | ||
| % (options.operation_type, response_info.http_version) | ||
| ) |
There was a problem hiding this comment.
debug probably over informative, but we don't have other levels yet.
I would suggest to merge it with data log above.
| logger.debug( | ||
| "PubNub request completed: operation=%s protocol=%s" | ||
| % (options.operation_type, response_info.http_version) | ||
| ) |
There was a problem hiding this comment.
debug probably over informative, but we don't have other levels yet.
I would suggest to merge it with data log above.
| logger.debug( | ||
| "PubNub request completed: operation=%s protocol=%s" | ||
| % (e_options.operation_type, http_ver) | ||
| ) |
There was a problem hiding this comment.
Should we actually print it if version information available in response?
There was a problem hiding this comment.
Here's what I've been told to deliver:
Deliverables:
Debug logs such as:
PubNub request completed: operation=subscribe protocol=HTTP/2
PubNub request completed: operation=publish protocol=HTTP/1.1
I'll try to make it less noisy and merge it with data logs you suggested
There was a problem hiding this comment.
Ok, I've missed operation in the format.
| pytest-asyncio>=1.0.0 | ||
| httpx>=0.28 | ||
| h2>=4.1 | ||
| h2>=4.3 |
There was a problem hiding this comment.
This is only development dependencies, but there is actual package dependencies which needs to be updated as well in setup.py.
There was a problem hiding this comment.
This one should be instructed in a same way as AsyncClient need to specify http2=True in constructor.
There was a problem hiding this comment.
But where is http2=True in constructor?
| client_request=None, | ||
| client_response=response | ||
| client_response=response, | ||
| http_version=f"HTTP/{response.version.major}.{response.version.minor}" if response.version else None |
There was a problem hiding this comment.
Interesting, that aiohttp dopes't support HTTP/2 at all.
There was a problem hiding this comment.
Indeed, but there's nothing we can do here, and we need to ensure that our API is transport-resilient. I would keep setting a http_version field for consistency, even though it will always be HTTP/1.1. What's your take on that?
| auth_key=auth_key, | ||
| client_request=res.request | ||
| client_request=res.request, | ||
| http_version=( |
There was a problem hiding this comment.
Same as aiohttp this one native request based on urllib3 also doesn't support HTTP/2 - they fundraising for it on their webpage.
There was a problem hiding this comment.
Indeed, but there's nothing we can do here, and we need to ensure that our API is transport-resilient. I would keep setting a http_version field for consistency, even though it will always be HTTP/1.1. What's your take on that?
3 participants
feat: enable HTTP/2 negotiation on the synchronous
httpxhandlerfeat: add
http_versionfield toResponseInfofeat: log negotiated protocol version at DEBUG level across all request handlers