- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 880
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
Support for Event Hooks #790
Comments
Right now it doesn’t have any API for that, no. It’s certainly something we want to consider, yes. It’d be really helpful to start getting some user feedback to help us figure out if event hooks are what we’d like to use here, or if we should be considering a more general middleware API. What’s your particular use case here? |
We use hooks to collect metrics about all the requests, mostly for monitoring purposes. It would be nice if the def response_hook(response, *args, **kwargs):
# Log status of response, etc...
...
def get_client():
client = httpx.AsyncClient()
client.hooks["response"].append(response_hook)
return client A more general middleware API could be a good idea and definitely solves the problem. It's not a big deal to make a workaround for our specific case, but since the other libraries supported it I thought there might be another way of doing it in HTTPX which I couldn't find. |
IMO a general middleware API like the one drafted in #783 would be the most versatile way to provide these "on request/on response" hooks, as well as address a bunch of other use cases… class MonitoringMiddleware:
def __init__(self, parent):
self.parent = parent
def __call__(self, request, scope, timeout):
request_hook(request, ...)
response = yield from self.parent(request, scope, timeout)
response_hook(response, ...)
return response
client = httpx.Client()
client.add_middleware(MonitoringMiddleware) |
Though we might want to provide a more streamlined API, that might just happen to be implemented as middleware, e.g... def monitor_requests(request):
# Process request...
response = yield request
# Process response...
client = httpx.Client(hooks=[monitor_requests]) (Somewhat inspired by encode/starlette#799.) |
Jumping on here instead of creating a new issue: I've got some project code that patched both requests and the stdlib to allow for hooks that logged requests and redirects. Went to go upstream it to requests and ended up here instead. Is there enough interest here to make a proof of concept worthwhile? |
Hey, Yes, I think we could go for a PR drafting things out... Forget the middleware stuff I mentioned above at the time, I’d agree a simpler and more integrated « register request/response callbacks » API is probably a better first step. We could start off back from #790 (comment), however I think we should consider a slightly different API design approach, in the form of registering lists of callbacks on client init (instead of mutating an attribute on the client). This is somewhat in line with Starlette’s event handlers API, and less fiddly than « expose a dict with arbitrary 'request'/'response' keys that might be mutated anytime etc ». So something like... import httpx
client = httpx.Client(
on_request=[...],
on_response=[...],
) |
Okay, so I've been thinking about this for a bit, and it's time to get this down. # Setting on __init__.
client = httpx.Client(event_hooks={'request': ..., 'response': ...})
# Mutating the property.
client.event_hooks['request'] = ...
client.event_hooks['response'] = ...
# Should be included at the top-level API
httpx.get(..., event_hooks={...}) We want to support either none, one, or many hooks on each event type, so I think I think we want to support 4 event hooks, that will give us a nice comprehensive hook into the complete request/response flow...
One thing I don't feel super absolutely clear on is for the async case what we should be expecting from the functions. I think that the sensible thing to do is always require regular callables for the sync case and always require async callables for the async case. I don't really want to get into "allow either async/sync callables for Sticking with Aside: One neat pattern that event hooks would give us is this... client = httpx.Client(event_hooks={'response': lambda r: r.raise_for_status()}) |
@tomchristie Coming from having thought telemetry support a bit more (#1264), and building upon thoughts that led me to #790 (comment), what do we think about a different (perhaps complementary?) API like the following…? (The use of the term "interceptor" is only to clarify that these probably shouldn't be event hooks with multiple possible call points, but rather only focused on the request/response cycle?) def intercept_logs(request: httpx.Request, send: Callable) -> Response:
print(request)
response = send(request)
print(response)
return response
async def async_intercept_logs(request: httpx.Request, send: Callable) -> Response:
print(request)
response = await send(request)
print(response)
return response
client = httpx.Client(interceptors=[intercept_logs])
async_client = httpx.AsyncClient(interceptors=[async_intercept_logs]) The huge benefit of something like this would be that we get persistance of contextual information for free (thanks to the function scope). Right now the hooks system wouldn't give this to us without an extra API to store request-wise information, since it's not guaranteed that the I'll also say — it's not crystal clear to me that we even need hooks into Which means we could just get away with an "interceptor" API like the above, rather than a full-fledged "hooks for arbitrary events", which has the major drawback of being callback-based (which means suboptimal developer experience). |
I would suggest letting the AsyncClient support both sync and async interceptors, as not all interceptors need to have the benefit of asynchronous I/O. Detection of whether an interceptor is an async function or not can be done with the inspect module, and awaited accordingly. Also curious -- in the concept of "middleware" the idea is for the middleware to potentially modify requests and responses. This assumes that while the middleware is processing, it's blocking the respective request/response. I think for hooks like this, which would most likely be telemetry-related rather than object manipulation-related, that perhaps awaiting asynchronous interceptors would not be appropriate -- but rather, asyncio.ensure_future would be best suited. |
Also, perhaps rather than interceptors this should be based on signal dispatching? I can imagine other events within the library that one may want to tie into for telemety, e.g. authentication flows. |
Keep in mind that 1/ we're async library agnostic, meaning that we'd have to implement and work with an equivalent of this for other async libraries (double with anyio, just not the same API), and 2/ any introduction of actually concurrent code should be limited to a strict minimum, since we know it's bound to be fiddly and full of possible bugs (especially if we don't use a structured concurrency approach, ie making sure each spawned task has a well defined scope and will always get cleaned up). So, probably "no" on this particular aspect regardless of the proposed API, especially since we can start from simple function calls and upgrade to that approach if need be (which is really not clear at this point).
I'm really curious about this... I can see that setting hooks on Auth is possible, but what would be a practical use case we could think of? I'm having a hard time finding one, meaning I'm not so sure auth hooks are actually something we'd need as a first class feature (not to say we couldn't add support later with a more involved API matching the fact that this would be an exotic use case). |
Yup well we can do that easily enough. Eg. this pattern: https://simonwillison.net/2020/Sep/2/await-me-maybe/ async def call_or_await(value):
if callable(value):
value = value()
if hasattr(value, '__await__'):
value = await value
return value
...
for hook in self._event_hooks["request"]:
await call_or_await(hook(request)) Which is all async framework agnostic. Tho let's consider that option as a follow up.
I do have a use case for these with with the CLI yup, one thing we'd really like from the hooks is to be able to expose the complete set of requests/responses that get sent. But again, let's just roll with the basic |
Would it be helpful if i posted a maximally-trimmed-down version of the the code I used to make this work with stdlib/requests? |
I'm currently using https://github.com/encode/requests-async for one of my projects and now I wanted to switch to HTTPX, but I can't find out if it supports event hooks like this https://requests.readthedocs.io/en/master/user/advanced/#event-hooks.
Or is there another easy way to add hooks onto an
AsyncClient
object to be called every request?The text was updated successfully, but these errors were encountered: