-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Integrate Erlang's metadata and Elixir's metadata #9368
Integrate Erlang's metadata and Elixir's metadata #9368
Conversation
Elixir's Logger metadata allows to have duplicated keys, so this would be a backwards incompatible change. Said that, I'm not sure how common that is 🙂 Thoughts? |
Hi @hauleth, thanks for the PR! This also requires Erlang/OTP 21, which means we can’t merge it any sooner than the other PR. So maybe we should get all of the work done at once in that PR? |
And to clarify, I don’t want to do things conditionally, because it may lead to subtle difference of behavior as mentioned by @fertapric. If there is a need to rush, I would rather merge the complete PR and drop support for 20 sooner. |
end | ||
map when is_map(map) -> | ||
merged = Enum.into(keyword, map) | ||
metadata = filter_out_nils(merged) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intention to also filter the existing metadata or just the new ones? If only the new one, we could possibly merge & filter in one go
for {_k, v} = elem <- keyword, v != nil, into: map, do: elem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michalmuskala when sending data to the backend all nils will be removed anyway, so there is not much difference.
@fertapric while in theory having duplicated keys in metadata is possible, it is highly impropable, as almost any action on these will remove duplicates anyway. |
@josevalim I proposed it as a result of discussion during WG meeting where there was proposal to provide metadata integration sooner, before whole #9333 will be ready to merge. But in the end it is up to Elixir team how to handle that. |
We have three months if we bring the release date early and nine months otherwise, so let's try to make it go the whole way in. :) |
Thanks for the PR! |
Close erlef/observability-wg#6