Add support of Logger.metadata #22
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`Plug.RequestId` supports print request_id to console with following
setting:
config :logger, :console, metadata: [:request_id]
Reference:
https://github.com/elixir-plug/plug/blob/3193ab3a8e1973e2fcc37b33417724a45dac857c/lib/plug/request_id.ex#L11
It would be nice if remote_ip support this feature:
config :logger, :console, metadata: [:request_id, :remote_ip]
So, here is it. ;)
|
Excellent idea! Thanks for the contribution. |
ajvondrak
added a commit
that referenced
this pull request
Jan 23, 2021
ajvondrak
added a commit
that referenced
this pull request
Jan 23, 2021
With the functionality added by #22, I want to add tests to make sure the Logger metadata gets set correctly. Before I can do that, I have to (once again) overhaul the design of the top-level RemoteIp tests. To be clear, this commit DOES NOT test the Logger.metadata yet. But it's refactored in such a way that we'll be able to make the right assertions later. The rewrite just makes a noisy enough diff that I'm committing it separately. Most of the motivation is that I'm uncomfortable testing call/2 and from/2 together now. The metadata should only be added in the call/2 case, whereas from/2 should most likely not interfere with the current process's logs (since it could well be invoked outside of the request cycle). We could test this via the single function as-is by ordering things carefully, like assert RemoteIp.from(...) == ... assert Logger.metadata[:remote_ip] == nil assert RemoteIp.call(...) = ... assert Logger.metadata[:remote_ip] == ... but that really just feels finicky; like it's trying to be too smart. Furthermore, the behavior of call/2 is also practically different from from/2 because the Plug.Conn *should* always have its :remote_ip set. That means we should be testing whether the default case of call/2 leaves the :remote_ip intact. By contrast, from/2 can return nil. The way the tests were previously written, I'd swept the distinction under the rug by effectively only ever using %Plug.Conn{remote_ip: nil} structs, thus forcing a false equivalence between the call/2 and from/2. So, I specifically factored out the essential single-header cases into the call/2 and from/2 describe blocks. In my latest bid to try to reduce the copy/paste endemic to this test suite, I pulled these examples out into loops over the module attributes. The two blocks can then make their separate assertions: call/2 defaults to the incoming peer address and adds metadata, whereas from/2 returns nil and sets no metadata. However, the examples around specific :clients and :proxies configurations were too convoluted to move into these loops. There were just too many moving pieces, and the previous examples were being tacitly overloaded to test all sorts of niggling things. So rather than try to jam those into the loopy call/2 & from/2 describe blocks, I factored out specific describe blocks for each of the options. These spell out the exact interactions expected of the configurations and doesn't get bogged down in call/2 vs from/2 behavior. In fact, they exclusively use RemoteIp.from/2 for their assertions, just because it's more ergonomic than making a %Plug.Conn{}. I figure we don't lose out on much by doing this, because the basic metadata & return value semantics are codified by the call/2 & from/2 blocks.
ajvondrak
added a commit
that referenced
this pull request
Jan 23, 2021
The problem with the implementation from #22 is that it fails to set the logger metadata when an IP can't be parsed out of the forwarding headers. I think it's safe to say that the less surprising behavior would be to go ahead and add the metadata anyway, even though it's coming from the original :remote_ip and not the :req_headers. These tests have been deliberately added to enforce this expected behavior, although the implementation currently fails.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Plug.RequestIdsupports print request_id to console with followingsetting:
Reference:
https://github.com/elixir-plug/plug/blob/3193ab3a8e1973e2fcc37b33417724a45dac857c/lib/plug/request_id.ex#L11
It would be nice if remote_ip support this feature, too:
So, here is it. ;)