Skip to content
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

Closed
martsime opened this issue Jan 22, 2020 · 13 comments · Fixed by #1246
Closed

Support for Event Hooks #790

martsime opened this issue Jan 22, 2020 · 13 comments · Fixed by #1246
Labels
enhancement New feature or request requests-compat Issues related to Requests backwards compatibility
Milestone

Comments

@martsime
Copy link

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?

@tomchristie
Copy link
Member

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?

@martsime
Copy link
Author

We use hooks to collect metrics about all the requests, mostly for monitoring purposes. It would be nice if the AsyncClient supported it similar to how Session in requests and requests_async does. So one could do something like this:

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.

@tomchristie tomchristie added the enhancement New feature or request label Jan 22, 2020
@florimondmanca
Copy link
Member

florimondmanca commented Jan 23, 2020

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)

@florimondmanca
Copy link
Member

florimondmanca commented Jan 23, 2020

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.)

@florimondmanca florimondmanca added the requests-compat Issues related to Requests backwards compatibility label Jan 28, 2020
@jbrockmendel
Copy link

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?

@florimondmanca
Copy link
Member

florimondmanca commented Jun 29, 2020

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=[...],
)

@tomchristie tomchristie added this to the v1.1 milestone Jul 30, 2020
@tomchristie tomchristie modified the milestones: v1.1, v1.0 Aug 7, 2020
@tomchristie
Copy link
Member

Okay, so I've been thinking about this for a bit, and it's time to get this down.
Here's what I'm thinking for the design here...

# 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 event_hooks should be a mapping type that always returns lists.

I think we want to support 4 event hooks, that will give us a nice comprehensive hook into the complete request/response flow...

  • request - Signature: func(request). Called after everything is prepared on the request, including any initial authentication.
  • auth - Signature: func(response, request). For most folks this is a corner case. Only called if the auth flow invokes additional requests beyond the original outgoing request.
  • redirect - Signature: func(response, request). Called if the redirect flow invokes multiple requests. Not called for allow_redirects=False.
  • response - Signature: func(response). Called once we're done & happy. Obvs.

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 AsyncClient and we'll magically introspect which you're using".

Sticking with event_hooks on AsyncClient being mandated as always being async function is at least simple & clear.

Aside: One neat pattern that event hooks would give us is this...

client = httpx.Client(event_hooks={'response': lambda r: r.raise_for_status()})

@florimondmanca
Copy link
Member

florimondmanca commented Sep 6, 2020

@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 request object stays the same across an HTTP call (it might change, eg due to auth or redirects).

I'll also say — it's not crystal clear to me that we even need hooks into auth and redirect. At least, telemetry clients don't seem to support such fine-grained insight into existing HTTP clients (Requests, aiohttp). And I can't think of practical use cases for plugging into those events in particular (though this doesn't mean no use case exists — can you think of any?).

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).

@dustydecapod
Copy link

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.

@dustydecapod
Copy link

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.

@florimondmanca
Copy link
Member

asyncio.ensure_future would be best suited

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).

one may want to tie into for telemety, e.g. authentication flows.

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).

@tomchristie
Copy link
Member

I would suggest letting the AsyncClient support both sync and async interceptors

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.

it's not crystal clear to me that we even need hooks into auth and redirect

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 request/response hooks to start with and then follow up.

@jbrockmendel
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request requests-compat Issues related to Requests backwards compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants