Addition of elixir logger and request_id #75
Conversation
| end | ||
|
|
||
| defp generate_request_id do | ||
| :crypto.strong_rand_bytes(16) |
There was a problem hiding this comment.
Just use Base.encode_16(bytes, case: :lower).
There was a problem hiding this comment.
Btw, do we need 16 bytes? Isn't that a lot? Also, do they need to be strong? (I would say they do not).
There was a problem hiding this comment.
iex(1)> :math.pow(256, 16)
3.402823669209385e3
That's how many entries we get. I would go with 4 or 5 bytes.
There was a problem hiding this comment.
I just looked into how rack did their request-id's and they went with 16. Completely arbitrary I can do 4 bytes.
|
Not sure if two plugs is a good idea since both plugs end up invoking Logger anyway. The goal of request_id is for logging, so I would go with one. @ericmj and @chrismccord, thoughts? |
| request_id = Dict.fetch(conn.req_headers, "http-x-request-id", generate_request_id) | ||
| Logger.metadata(request_id: request_id) | ||
| Conn.register_before_send(conn, fn (conn) -> | ||
| Conn.put_resp_header(conn, 'X-Request-Id', request_id) |
There was a problem hiding this comment.
Plug convention is lower case for header names.
|
I think if we have no use for request_id outside of logging we can keep it as a single plug and extract into two later if the need comes up. |
|
Thanks for the feedback. WIll merge back into one. Makes sense to me. |
|
Alright I will write tests for this and ping you later. |
|
@josevalim also once I have some tests in place I'll switch to iolists. Those things are hard to get used to and almost always feel like an optimization paid for with reduced readability. |
| end | ||
|
|
||
| defp generate_request_id do | ||
| :crypto.strong_rand_bytes(4) |
There was a problem hiding this comment.
:crypto.strong_rand_bytes(4) |> Base.encode16(case: :lower)
There was a problem hiding this comment.
Actually, let's go with: :crypto.strong_rand_bytes(6) |> Base.encode64() it has more bytes and take the same size.
|
@josevalim I've recently added tests and changed it to use iolists instead of interpolated strings. I had one question around configuration and metadata that I 'TODO'd. |
|
|
||
| def init(opts) do | ||
| #TODO I'm not sure this is the best way. Maybe the user needs to define the metadata on their own? | ||
| Logger.configure_backend(:console, colors: [enabled: false], metadata: [:request_id]) |
There was a problem hiding this comment.
We should definitely not do this. It is up to the user to configure colors, how backends should work, which metadata needs to be included, etc. :)
|
Thank you! I have added some comments! |
|
|
||
| def call(conn, _config) do | ||
| request_id = case Conn.get_req_header(conn, "x-request-id") do | ||
| [] -> generate_request_id |
There was a problem hiding this comment.
Please use generate_request_id() for clarity.
|
@josevalim Made the changes we discussed. Lemme know and I'll roll this into a single commit. |
| @@ -0,0 +1,80 @@ | |||
| defmodule Plug.Logger do | |||
| @moduledoc """ | |||
| A plug for logging basic request information. It expects no options on initialization. | |||
There was a problem hiding this comment.
Keep the first line short as it is used as summary.
|
I have added three more small comments, thank you! |
| end | ||
| end | ||
|
|
||
| defp connection_type(conn) do |
There was a problem hiding this comment.
You can pattern match on %{state: :chunked}.
|
@josevalim changes made! Thanks for the feedback. |
|
@josevalim if you think this is ready for merge I can roll this up into a single commit. |
|
Yeah, please squash! |
|
@josevalim squashed. |
|
there's no hiding @ericmj 💃 |
|
@josevalim 🌽 does this help? :) |
Addition of elixir logger and request_id
|
Merged ❤️! |
Addition of elixir logger and request_id
Adding basic logger support to plug. re phoenixframework/phoenix#235
Also adding a unique request id to each request header, response and Logger metadata.
@josevalim I wanted to make sure you were okay with the addition of two plugs/API before I tested and documented.