mirage / ocaml-cohttp

An OCaml library for HTTP clients and servers using Lwt or Async
Other
713 stars 174 forks source link

Make Logging Easy! #307

Closed trevorsummerssmith closed 9 years ago

trevorsummerssmith commented 9 years ago

Problem: I would like to add a handler that outputs a single log line that describes the request and response for a given http interaction. For example http://en.wikipedia.org/wiki/Common_Log_Format I imagine many others would want this, as log aggregators, etc. are used to using standard formats. To do this, one needs access to the request, response and response body. Currently to implement this one needs to add a layer of indirection in their handler code, because Server.respond returns an abstract type. To implement this logging, I did something like:

(* indiretion to replace Server.respond *)
let respond ?(flush) ?(headers)  ?(body) (status_code) =
  return (flush, headers, body, status_code)

(* example log handler *)
let log_handler handler ~body sock request =
  handler ~body sock request >>= fun (flush, headers, body, status_code) ->
    Log.Global.info "%s %s %s %s / %s %s %d" (* ... *);
    Server.respond ?flush ?headers:headerss ?body status_code

(* main 'dispatch method' *)
let handler body sock request = 
 (* do important work ...*)
 respond `Ok

I see that a similar workaround was done in cowabloga -- https://github.com/mirage/cowabloga/blob/master/lib/dispatch.ml#L27 (note that is with the LWT implementation and I am talking about the async implementation).

Solution: It seems that it would be a great usability improvement to add a "outgoing" lifecycle call back (not a good name) to the async Server.create function. This callback function would have a signature something like:

val outgoing_callback: Request.t -> Body.t -> Response.t -> unit

This would allow one to trivially implement the logging use case, without having to refactor all of their handler code to use a layer of indirection.

Eager to hear your thoughts. Thanks.

rgrinberg commented 9 years ago

This would allow one to trivially implement the logging use case, without having to refactor all of their handler code to use a layer of indirection.

I'm not yet convinced. The type of callback that you provide to Server.create is something akin to type callback = Request.t -> Response.t Deferred. Given this type, you may implement logging completely ignoring the implementation of the callback type. E.g. val with_logs: callback -> callback where

let with_logs f request =
  Log.Global.info (Sexp.to_string (<:sexp_of<request>>));
  let response = f request in
  Log.Global.info (Sexp.to_string (<:sexp_of<response>>));
  resp

Which is not too different from how middleware is handled in opium http://rgrinberg.com/blog/2014/04/11/middleware-intro/

trevorsummerssmith commented 9 years ago

Rudi sorry for the late response. I hadn't had a chance to read through your blog post. I love what you have going on in your opium. Very cool -- your point is that it would better to have a generic interface than the single callback I suggested. Much more flexible than my original idea of the outgoing callback. I like it.

In the current Cohttp_async (and it appears in Cohttp_lwt as well?) one cannot do this due to the fact that the Response is an abstract type. If Response was made non-abstract, and generic over all response types (to include among other things a Body.t option) then the logging bits would be easy to implement, and usability of the library in general would go way up.

rgrinberg commented 9 years ago

Responses are no longer abstract (I don't think they were ever abstract in Cohttp_lwt).