-
Notifications
You must be signed in to change notification settings - Fork 516
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
Move user related fields to align with ECS schema. #1783
Conversation
@@ -63,6 +63,9 @@ | |||
"started": 43 | |||
}, | |||
"type": "request" | |||
}, | |||
"user": { | |||
"id": "123user" |
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.
user
information from the metadata
is set if no transaction.context.user
information was sent.
@@ -80,7 +80,10 @@ func (m *Metadata) Merge(fields common.MapStr) common.MapStr { | |||
utility.Add(fields, "host", m.System.fields()) | |||
utility.Add(fields, "process", m.Process.fields()) | |||
utility.MergeAdd(fields, "service", m.Service.fields()) | |||
utility.MergeAdd(fields, "user", m.User.fields()) | |||
utility.AddIfNil(fields, "user", m.User.Fields()) |
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.
I don't think we should merge user
information from metadata
and context.user
but overwrite it.
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.
@graphaelli the user_agent
and the client.ip
are set, even if the context.user
information is set directly in an event, as they are nested under different root keys and not under user
. So using AddIfNil
seems proper here.
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.
Agreed on both counts. Think we need an integration test for this too?
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.
The MergeAdd
was introduced long time ago, https://github.com/elastic/apm-server/pull/687/files.
I think it is a bug to mix two potential different user
informations to one, that is why I changed it to AddIfNil
, which basically first checks if there was a user sent with the specific event, and if not sets the one from the metadata
.
The possibility to send a user
in metadata
and the event
and if this makes sense or not is a completely different topic to discuss. Please raise any concerns about this in this issue #1255.
Move `context.user.email` to `user.email` Move `context.user.id` to `user.id` Move `context.user.username` to `user.name` Move `context.user.ip` to `client.ip` Move `context.user.user-agent` to `user_agent` fixes elastic#1782
f0db328
to
dc56f56
Compare
Kibana Index PR is up elastic/kibana#28678 |
@graphaelli @jalvz is there something left you require for this PR? Otherwise I'd appreciate an approval. |
jenkins, retest this please. |
jenkins, retest this |
jenkins, retest this please |
@@ -80,7 +80,10 @@ func (m *Metadata) Merge(fields common.MapStr) common.MapStr { | |||
utility.Add(fields, "host", m.System.fields()) | |||
utility.Add(fields, "process", m.Process.fields()) | |||
utility.MergeAdd(fields, "service", m.Service.fields()) | |||
utility.MergeAdd(fields, "user", m.User.fields()) | |||
utility.AddIfNil(fields, "user", m.User.Fields()) |
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.
Agreed on both counts. Think we need an integration test for this too?
Great work! Really thorough testing 💯 |
Merging this in now as the failing test is a flaky, unrelated one. |
Move
context.user.email
touser.email
Move
context.user.id
touser.id
Move
context.user.username
touser.name
Move
context.user.ip
toclient.ip
Move
context.user.user-agent
touser_agent
fixes #1782
depends on elastic/kibana#28678