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.

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

Make 7.0 like 6.7 user agent ecs #38757

Merged
merged 7 commits into from
Feb 13, 2019

Conversation

jakelandis
Copy link
Contributor

This PR moves the user_agent processor to share a code base with 6.7 as discovered in elastic/beats#10650 is required for Beats migration to 7.0. This re-introduces the ecs option and the optional legacy behavior.

This change is largely a set of revert's and cherry-picks, of which only the the most recent commits (ones without a #number) is net new code. This change is intended to be merged to 7.0.0 and then 7.x to allow the ecs option to live during the 7.x timeframe.

Additionally, this changes the default from false to true for the ecs option.

6.7: ecs : default false
7.x: ecs : default true
8.0: no option, but behaves as true

The first 5 commits here:
git revert 5b008a3 (Lee's initial 7.x implementation of user_agent ecs)
git revert cac6b8e (Jake's minor change applied to 7.x)
git cherry-pick 5dfe193 (Lee's initial 6.x implementation of user_agent ecs)
git cherry-pick ec8ddc8 (Jake's minor change applied to the 6.x )
git cherry-pick f63cbdb (Gordon's 6.x deprecation info API, with manual merge fixes)

jakelandis and others added 6 commits February 11, 2019 16:04
…8115)"

This reverts commit 5b008a3.

Related: elastic/beats#10650

Will replace this commit with the 6.7 version
elastic#37984)"

This reverts commit cac6b8e.

Related: elastic/beats#10650

Will replace this commit with the 6.7 version
This switches the format of the user agent processor to use the schema from [ECS](https://github.com/elastic/ecs).
So rather than something like this:

```
{
  "patch" : "3538",
  "major" : "70",
  "minor" : "0",
  "os" : "Mac OS X 10.14.1",
  "os_minor" : "14",
  "os_major" : "10",
  "name" : "Chrome",
  "os_name" : "Mac OS X",
  "device" : "Other"
}
```

The structure is now like this:

```
{
  "name" : "Chrome",
  "original" : "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36",
  "os" : {
    "name" : "Mac OS X",
    "version" : "10.14.1",
    "full" : "Mac OS X 10.14.1"
  },
  "device" : "Other",
  "version" : "70.0.3538.102"
}
```

This new can be configured by setting `"ecs": true` in the processor
configuration, and will be the default for 7.0. Leaving `ecs` unset or set as
`false` is deprecated.

Resolves elastic#37329

This PR is against the 6.x branch and will be forward-ported with the deprecated parts removed (I will open a subsequent PR for that).
…lastic#38121)

When the ingest node user_agent parses the device field, it
will result in a string value. To match the ecs schema
this commit moves the value of the parsed device to an
object with an inner field named 'name'. There are not
any passivity concerns since this modifies an unreleased change.

closes elastic#38094
relates elastic#37329
The User Agent ingest processor is changing to align with ECS. Users
need to be made aware that any pipelines using this processor will
behave differently in 7.0.
@jakelandis jakelandis added blocker WIP :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v7.0.0 v7.2.0 labels Feb 12, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@ruflin
Copy link
Member

ruflin commented Feb 12, 2019

@jakelandis I tested this branch against my branch here with Filebeat 6.7 and the output seems to be as expected: elastic/beats#10688

@gwbrown
Copy link
Contributor

gwbrown commented Feb 12, 2019

With this change, do we also want to adjust the deprecation warning in 6.7? It will currently warn on pipelines with ecs: false or ecs unset, whereas if we're going to support ecs: false in 7.0, it should only warn on ecs unset.

@jakelandis
Copy link
Contributor Author

With this change, do we also want to adjust the deprecation warning in 6.7? It will currently warn on pipelines with ecs: false or ecs unset, whereas if we're going to support ecs: false in 7.0, it should only warn on ecs unset.

@gwbrown
I think we are good. The specific message says "Ingest pipelines [pipeline_x, pipeline_y] will change to using ECS output format in 7.0" which is still true ! (this PR changes the default to true, but leaves the option)

@jakelandis jakelandis removed the WIP label Feb 12, 2019
@jakelandis
Copy link
Contributor Author

@dakrone @gwbrown Could I get your reviews on this ?

The only commits that should require a review are:
0786e76 - cherry pick, but manually fixed merge conflicts
3a0993e - actual changes here , sets default to true tweaks the deprecation
0bc5186 - fixes the integration test.

@gwbrown
Copy link
Contributor

gwbrown commented Feb 12, 2019

@jakelandis and I talked synchronously about this and we now agree that with this change to 7.0, the Deprececation Info API in 6.7 will give a misleading deprecation warning for pipelines with ecs: false set: in particular, it will return a message that says this:
Ingest pipelines [pipeline_with_ecs_false] will change to using ECS output format in 7.0

Even though, with this PR, pipeline_with_ecs_false will continue to work the same way in 7.0 as in 6.7. In order to be in line with this PR, the Deprecation API should warn only about pipelines with the user_agent processor that do not have ecs set explicitly.

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM assuming CI is happy

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

LGTM as well.

@jakelandis jakelandis merged commit 5d8a980 into elastic:7.0 Feb 13, 2019
jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Feb 13, 2019
This change reverts the initial 7.0 commits and replaces them
with the 6.7 variant that still allows for the ecs flag. 
This commit differs from the 6.7 variants in that ecs flag will 
now default to true. 

6.7: `ecs` : default `false`
7.x: `ecs` : default `true`
8.0: no option, but behaves as `true`

* Revert "Ingest node - user agent, move device to an object (elastic#38115)"
This reverts commit 5b008a3.

* Revert "Add ECS schema for user-agent ingest processor (elastic#37727) (elastic#37984)"
This reverts commit cac6b8e.

* cherry-pick 5dfe193 
Add ECS schema for user-agent ingest processor (elastic#37727)

* cherry-pick ec8ddc8 
Ingest node - user agent, move device to an object (elastic#38115) (elastic#38121)
  
* cherry-pick f63cbdb (with manual merge fixes)
Dep. check for ECS changes to User Agent processor (elastic#38362)

* make true the default for the ecs option, and update 7.0 references and tests
jakelandis added a commit that referenced this pull request Feb 13, 2019
Forward port of #38757

This change reverts the initial 7.0 commits and replaces them
with the 6.7 variant that still allows for the ecs flag. 
This commit differs from the 6.7 variants in that ecs flag will 
now default to true. 

6.7: `ecs` : default `false`
7.x: `ecs` : default `true`
8.0: no option, but behaves as `true`

* Revert "Ingest node - user agent, move device to an object (#38115)"
This reverts commit 5b008a3.

* Revert "Add ECS schema for user-agent ingest processor (#37727) (#37984)"
This reverts commit cac6b8e.

* cherry-pick 5dfe193 
Add ECS schema for user-agent ingest processor (#37727)

* cherry-pick ec8ddc8 
Ingest node - user agent, move device to an object (#38115) (#38121)
  
* cherry-pick f63cbdb (with manual merge fixes)
Dep. check for ECS changes to User Agent processor (#38362)

* make true the default for the ecs option, and update 7.0 references and tests
jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Feb 21, 2019
PR elastic#38757 changed 7.0 user-agent behavior for the ecs flag
to more closely resemble 6.7. This commit updates the forward
looking comments and deprecation notices to be more accurate
now that this flag will not be removed until 8.0

related elastic#38757
jakelandis added a commit that referenced this pull request Feb 21, 2019
PR #38757 changed 7.0 user-agent behavior for the ecs flag
to more closely resemble 6.7. This commit updates the forward
looking comments and deprecation notices to be more accurate
now that this flag will not be removed until 8.0

related #38757
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement v7.0.0-beta1 v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants