Setting user_agent as root key for all events, and canonicalize http headers. #1966
Conversation
Copies user-agent information from `context.request.headers` information to root level user_agent key, and apply ua pipeline on this field. fixes elastic#1960
|
@watson @felixbarny @jahtalab after some offline discussions I adapted the current handling for enabling user-agent pipelines, to ensure the user-agent information is always indexed within the top level key |
model/context.go
Outdated
| if http == nil || http.Request == nil || http.Request.Headers == nil { | ||
| return nil | ||
| } | ||
| for _, ua := range []string{"user-agent", "User-Agent"} { |
There was a problem hiding this comment.
I'm a bit torn on whether this is good enough or if we should use the same approach the stdlib does. Another option is to decode headers specially and pull the user agent one out while we're decoding anyway, thoughts?
There was a problem hiding this comment.
I like the ideas, investigating a bit.
There was a problem hiding this comment.
I really like where you've taken this. One side effect of this change is that all headers are canonicalized where they weren't before (right?). I think that's a great change (appearance and opt-in searchability wise should a user choose to index any of these) but should be documented - can we add http.request.headers to the fields doc?
Also, what do you think about having the UI display these specially? As a user, I'd expect single valued headers not to be displayed as lists.
with this change we'd have:
I think a better display would be:
Array: foo
Array: bar
Array: baz
User-Agent: Mozilla Chrome Edge
and then there is no difference between repeated headers and single valued ones.
There was a problem hiding this comment.
If I understand your suggestion regarding showing the headers right, then this would involve work from @elastic/apm-ui.
I think it is ok to label canonicalizing headers as a bugfix. However, I was thinking if we also need to address this in the migration script? From my perspective, we don't have to, as the data would still be readable from the UI. Would appreciate your thoughts on this.
I also added the restrictions on the Intake API to the context.response.headers as it belongs to both.
There was a problem hiding this comment.
If I understand your suggestion regarding showing the headers right, then this would involve work from @elastic/apm-ui.
Yes, exactly. This work could follow in any future release.
I think it is ok to label canonicalizing headers as a bugfix. However, I was thinking if we also need to address this in the migration script? From my perspective, we don't have to
Agreed on bugfix and also that the migration script does not need (or even could, well) to handle this.
There was a problem hiding this comment.
This sound good to me. @formgeist do you have any opinion on this?
There was a problem hiding this comment.
The values would be displayed as cells when we merge elastic/kibana#32199, correct? I think that's the right way to go.
There was a problem hiding this comment.
Currently they would be displayed as Lists yes. I second @graphaelli 's suggestion to better display as
Array: foo
Array: bar
Array: baz
User-Agent: Mozilla Chrome Edge
I don't see this urgent, but rather as an UI improvement.
There was a problem hiding this comment.
There was a problem hiding this comment.
Right, I didn't see it was another nested level. I reckon we should allow for that, it's nicer presentation.
simitt
changed the title
…eaders. (elastic#1966) Copies user-agent information from `context.request.headers` information to root level user_agent key, and apply ua pipeline on this field. Canonicalizes the http headers stored in ES. fixes elastic#1960
…eaders. (elastic#1966) Copies user-agent information from `context.request.headers` information to root level user_agent key, and apply ua pipeline on this field. Canonicalizes the http headers stored in ES. fixes elastic#1960
Copies user-agent information from
context.request.headersinformationto root level user_agent key, and apply ua pipeline on this field.
fixes #1960
This should be backported as a bugfix to 7.x and 7.0 branches.