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.

Sign up for GitHub

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

Jump to bottom

Keep unparsed user agent information in user_agent.original #8537

Merged
merged 5 commits into from
Oct 5, 2018
Merged

Keep unparsed user agent information in user_agent.original #8537

merged 5 commits into from
Oct 5, 2018

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Oct 2, 2018

user_agent.raw has been renamed to user_agent.original. As this field is not yet released I am renaming it to follow conventions.

@kvch kvch requested a review from jsoriano October 2, 2018 10:00
@kvch kvch added review Filebeat Filebeat labels Oct 2, 2018
@ruflin
Copy link
Contributor

ruflin commented Oct 2, 2018

Change LGTM. I think it will need a Changelog entry.

I have second thoughts if we should backport this to 6.x or not (not only raw to original, but the overall change).

@kvch
Copy link
Contributor Author

kvch commented Oct 3, 2018

Why? I think it can provide useful information is case of exotic user agents. I assume when someone investigates weird events happening in his/her network, it's possible that the person who might be lurking around leaves behind "unconventional" user agents.

@kvch kvch removed the request for review from jsoriano October 3, 2018 09:47
@kvch kvch force-pushed the rename-user-agent-raw-to-original branch from bc1e9e9 to 3b450ed Compare October 3, 2018 09:49
@kvch
Copy link
Contributor Author

kvch commented Oct 3, 2018

Added changelog entry && rebased the branch

jsoriano
jsoriano approved these changes Oct 3, 2018
View reviewed changes
CHANGELOG.asciidoc Outdated
@@ -123,6 +123,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
- Add tag "multiline" to "log.flags" if event consists of multiple lines. {pull}7997[7997]
- Add haproxy module. {pull}8014[8014]
- Release `docker` input as GA. {pull}8328[8328]
- Rename user_agent.raw to user_ageint.original to follow ECS conventions. {pull}8537[8537]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this hasn't been released yet, what about editing the previous entry instead of adding two for the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@ruflin
Copy link
Contributor

ruflin commented Oct 3, 2018

@kvch Few thoughts around this that recently came up:

  • Should we actually index user_agent.original? Is it mean to be searchable or more for reprocessing or looking into it when an event is found for the user (it will still be part of _source). This could save quite a bit of space on indexing
  • The user_agent field will be a common field in 7.0 because of ECS. So the local fields will probably disappear. Should we add more fields now or wait for 7.0 until it's unified?

@kvch kvch changed the title Rename user_agent.raw to user_agent.original to follow ECS conventions Keep unparsed user agent information in user_agent.original Oct 3, 2018
@kvch
Copy link
Contributor Author

kvch commented Oct 3, 2018

  • +1 for not indexing it
  • I would wait for 7.0. I have already renamed a few fields, because ECS is still a work in progress. I would rather minimize this overhead. If unavoidable, I would do it in a bulk before 7.0 is released.

@kvch
Copy link
Contributor Author

kvch commented Oct 4, 2018

@ruflin are you ok with merging this as is?

ruflin
ruflin approved these changes Oct 5, 2018
View reviewed changes
filebeat/module/apache2/access/_meta/fields.yml
@@ -85,10 +85,11 @@
type: keyword
description: >
The name of the operating system.
- name: raw
- name: original
type: text
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we set index: false it should not matter here what type is defined. In ECS it seems we put keyword. Only reason I mention this is we should check later what shows up in the docs for non indexed fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks for the info.

@ruflin
Copy link
Contributor

ruflin commented Oct 5, 2018

@kvch LGTM. Did an additional commit to resolve a CHANGELOG conflict.

@kvch
Copy link
Contributor Author

kvch commented Oct 5, 2018

Failing tests are unrelated.

@kvch kvch merged commit f3e0801 into elastic:master Oct 5, 2018
@ruflin ruflin mentioned this pull request Oct 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@jsoriano jsoriano jsoriano approved these changes

Assignees
No one assigned
Labels
Filebeat Filebeat review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kvch @ruflin @jsoriano