Issue 90: Add additional instrumentation for flash messages. #173
Conversation
example/w-flash/README.md
Outdated
| @@ -48,6 +48,8 @@ into cookies and reads them back out: | |||
|
|
|||
| ```ocaml | |||
| let () = | |||
| Dream.initialize_log ~level:`Info (); | |||
There was a problem hiding this comment.
AFAIK this call shouldn't be necessary. Is it? If set_log_level requires it, set_log_level should be altered to do a lazy internal initialize_log.
There was a problem hiding this comment.
When I tested this against the w-flash example it did appear to be necessary; unfortunately I don't fully understand why. I've updated set_log_level.
example/w-flash/flash.eml.ml
Outdated
| | `Ok ["text", text] -> | ||
| let () = Dream.put_flash "Info" text request in | ||
| | `Ok ["text", text] -> | ||
| let () = Dream.put_flash "Info" text request in |
There was a problem hiding this comment.
Why have these lines had an indentation-only change?
Also, where does this kind of indentation come from? It is wasteful of the left margin.
src/middleware/flash.ml
Outdated
| let existing = Dream.cookie flash_cookie request in | ||
| log.debug (fun log -> | ||
| let current = | ||
| flash request |
There was a problem hiding this comment.
Shouldn't this display the contents of outbox, rather than reading the cookie, which AFAIK is the flash messages returned in the request? If you'd like to display incoming flash messages as I think this is doing, I think it should be done before the call to inner_handler.
There was a problem hiding this comment.
In the issue, @thangngoc89 wrote that it was "frustrating to debug why flash messages aren't rendered". I thought it made sense to log the flash messages from the current request's cookie because those are the ones available for rendering a response. This also appears to match the behavior of the example code in the issue description.
I've fixed the ordering with inner_handler; that was an oversight on my part.
There was a problem hiding this comment.
The question is, in the original ordering, why read the cookie on the request (flash request), rather than reading outbox? Aren't the messages in the outbox the ones that will be put in the response?
There was a problem hiding this comment.
Ah okay, I understood you. I got double-negative-confused by myself :) Okay, I now think this is doing what #90 was asking, and the ordering makes sense now.
There was a problem hiding this comment.
Could you move the debug print all the way to the top of the middleware? It doesn't seem to depend on any of the values bound before it now, and having them bound there makes it seem like it might depend. It's a clean print of the flash messages being returned to the server. It will especially make it clear that the logging does not depend on the rebinding of request to one that has the outbox attached.
|
Okay, I realized I have only trivial nits left, so I merged. Thanks for doing this with the several steps it took! |
This change addresses #90. It introduces a new debug log point that records the current state of the flash message cookie on every request. Using the custom log levels introduced in #171, developers can easily set the
dream.flashlog source to debug level to trace their flash messages. The flash message example (w-flash) has been updated to illustrate how to do this.On some browsers (for example, Firefox) cookies are limited to a maximum of 4096 bytes; above this limit, a browser may drop the cookie. Since this can prevent flash messages from appearing, I've also included a warning log point to alert developers when their message content is too big.