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

feat(request_id): add custom Logger's metadata option #1247

Conversation

georgeguimaraes
Copy link
Contributor

@georgeguimaraes georgeguimaraes commented Sep 23, 2024

Add a way to configure the Logger metadata for Plug.RequestId.

Why? I need to use RequestId as

plug Plug.RequestId, http_header: "x-correlation-id", assign_as: :correlation_id

and then it feels weird to have request_id in the logs, don't you think?

With this PR, we'd have

plug Plug.RequestId, http_header: "x-correlation-id", assign_as: :correlation_id, metadata_as: :correlation_id

@josevalim
Copy link
Member

Given the plug is called RequestId, I'd really expect it to at least set a request id metadata. I also don't want it to have several different options, I'd rather make it easier to roll your own. What if we make the get_request_id(conn, header) function public? So you could build your own? Then we might even deprecate assign_as :D

@georgeguimaraes
Copy link
Contributor Author

nah, I don't think exposing get_request_id is worth it. One can have the exact same effect accessing conn.assigns[:request_id].

So, for me, if you don't accept this PR, I'm gonna create a new Plug in my app that only sets Logger.metadata with the key I want and that content.

Too hacky?

@josevalim
Copy link
Member

So, for me, if you don't accept this PR, I'm gonna create a new Plug in my app that only sets Logger.metadata with the key I want and that content.

The idea of exposing get_request_id was exactly to make it easier for you to roll your own plug. But accessing it in assigns also works. Your call!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants