Update template to use ES 5.x mapping #462

suyograo · 2016-08-04T01:15:13Z

Fixes #386

Fixes logstash-plugins#386

untergeek · 2016-08-04T01:18:31Z

LGTM™. Let's see if the tests pass 😄

untergeek · 2016-08-04T01:19:05Z

Super excited about this, btw. Definitely going to tweak some upgraders, but +1 for the clean start.

suyograo · 2016-08-04T02:35:03Z

hmm, I have to use some rspec tag trickery to fix this.

jordansissel · 2016-08-04T19:10:26Z

Let's block this until elastic/elasticsearch#19812 is resolved.

suyograo · 2016-08-04T19:39:06Z

Aye. Stalling

jordansissel · 2016-08-04T21:32:59Z

A few of us (@suyograo, @andrewvc, @talevy, etc) had discussions about this offline, and there were two main areas for discussion about the keyword name:

The keyword name in this template was chosen to be consistent with Elasticsearch's use of the word. Further, the name keyword is maybe not the best word.
The change from raw to keyword may negatively impact users who upgrade LS from 2.x to 5.x.

FOr the first part, we are discussing in elastic/elasticsearch#19812.

For the second part, we might leave this field named as raw to relieve stress on the upgrade experience. We may move to be consistent with Elasticsearch at a later date after we figure out how to minimize pains of upgrades.

untergeek · 2016-08-04T21:42:32Z

In all honesty, I'd multi-field and have both raw and keyword. This forces users to adjust the template themselves if they're space conscious, and gives us both reverse and forward compatibility, should Kibana standardize on .keyword in future releases.

suyograo · 2016-08-04T21:52:01Z

In all honesty, I'd multi-field and have both raw and keyword.

I am -1 on this. I wouldn't want users to incur extra disk space because of this. This multi-field is for every string, which is quite a bit.

jordansissel · 2016-08-08T21:03:22Z

Did some historical digging. Logstash v1.3.0 (December 2013) is the first version which shipped a template. Changing .raw has the opportunity to break schema with possibly 2.5 years of events stored in Elasticsearch with Logstash.

We discussed this last week somewhere I forget, but we had some thoughts for Logstash 5.0 timeframe:

Premise:

1.By default, Logstash will only install the template if there is no template already there.
2. We want to ship a template that more closely aligns with Elasticsearch's naming and defaults.
3. Elasticsearch now makes JSON strings dynamically mapped as both text and a multifield .keyword as keyword mapping type.

Therefore, I summarize a proposal as follows:

We change the default template to change .raw to .keyword
Advertise loudly in the changelog and release notes about this change.
Document an upgrade path for existing 2.x users to upgrade to the new template, including instructions on how to reindex data to effect a rename of .raw to .keyword

Expected impact of default behavior as proposed:

New users: Get .keyword, align with Elasticsearch.
Upgrading users: Keep .raw, misalign with Elasticsearch.

suyograo · 2016-08-12T20:02:08Z

@jordansissel I've changed this PR to reflect the proposal in #462 (comment). Can you take a look please? /cc @talevy

untergeek · 2016-08-17T17:04:26Z

.travis.yml

 - INTEGRATION=true ES_VERSION=1.7.5
- INTEGRATION=true ES_VERSION=5.0.0-alpha1
+- INTEGRATION=true ES_VERSION=2.3.4
+- INTEGRATION=true ES_VERSION=5.0.0-alpha4


Shouldn't this be alpha5 now?

bump version to alpha5?

This should be increased to alpha5 since it has been released.

Hahahaha. All of us saw the same thing 👍

untergeek · 2016-08-17T17:05:53Z

LGTM™

talevy · 2016-08-17T17:06:33Z

lib/logstash/outputs/elasticsearch/elasticsearch-template-es5x.json

            "fields" : {
-              "raw" : {"type": "string", "index" : "not_analyzed", "ignore_above" : 256}
+              "keyword" : { "type": "keyword" }


I thought the idea was to let ES run through its defaults in the case that a stringed field is being dynamically mapped

Yes, but we need "norms" : false, to be set, which ES does not by default. That's why I had to explicitly add this rule for strings.

See 2 lines above.

ah, I see. sounds good

talevy · 2016-08-17T17:07:14Z

spec/integration/outputs/pipeline_spec.rb

@@ -1,6 +1,6 @@
 require_relative "../../../spec/es_spec_helper"

-describe "Ingest pipeline execution behavior", :integration => true, :version_5x => true do
+describe "Ingest pipeline execution behavior", :integration => true, :version_greater_than_5x => true do


greater_than? and equal to?

talevy · 2016-08-17T17:19:53Z

lgtm

In PR logstash-plugins#462 (comment) we used `.keyword` to match ES's change in default behavior. Both approaches, using `.keyword` to match ES for consistency and using `.raw` to match existing logstash data has pros/cons. I think at this time it is wise to err on the side of easy-upgrade-path so reverting this back to using `.raw` as before Fixes logstash-plugins#466

Fix typo Update template to use ES 5.x mapping (logstash-plugins#462) * Update template to use ES 5.x mapping Fixes logstash-plugins#386 remove `force` as valid version type parameter remove trailing comma

Update template to use ES 5.x mapping

fc43eb2

Fixes logstash-plugins#386

suyograo mentioned this pull request Aug 4, 2016

let ES 5.x handle unanalyzed dynamically mapped strings #459

Closed

suyograo added breaking-compatibility template changes labels Aug 4, 2016

suyograo assigned untergeek and andrewvc Aug 4, 2016

suyograo added 2 commits August 3, 2016 18:38

Duplicate spec for 5.x for new mapping use

3dfad93

turn off 5x for old template tests

9b22019

suyograo added 4 commits August 4, 2016 10:37

rename spec and tags to match versions

d1d9208

Rename spec and use proper version tags

f912a8c

Push all changes files

053473d

use correct tags

2646919

jordansissel mentioned this pull request Aug 4, 2016

The keyword name is confusing to me. elastic/elasticsearch#19812

Closed

jordansissel mentioned this pull request Aug 4, 2016

sub keyword field to string dynamic mappings - name and intent discussion elastic/elasticsearch#18195

Closed

suyograo added 2 commits August 12, 2016 12:14

Use rspec tag trickery to run tests for all ver

4dfa67e

Update docs, bump version to 5.0

9ab35eb

untergeek reviewed Aug 17, 2016
View reviewed changes

Use ES alpha5 in tests

b0df891

talevy reviewed Aug 17, 2016
View reviewed changes

Change name of rspec tag to greater_than_equal_to

670503c

suyograo merged commit b8c336d into logstash-plugins:master Aug 17, 2016

suyograo deleted the fix/386 branch August 17, 2016 20:13

clintongormley mentioned this pull request Aug 26, 2016

Continue to use .raw instead of .keyword #466

Closed

suyograo mentioned this pull request Sep 8, 2016

revert to use .raw instead of .keyword #474

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update template to use ES 5.x mapping #462

Update template to use ES 5.x mapping #462

suyograo commented Aug 4, 2016

untergeek commented Aug 4, 2016

untergeek commented Aug 4, 2016

suyograo commented Aug 4, 2016

jordansissel commented Aug 4, 2016

suyograo commented Aug 4, 2016

jordansissel commented Aug 4, 2016

untergeek commented Aug 4, 2016

suyograo commented Aug 4, 2016

jordansissel commented Aug 8, 2016 •

edited

Loading

suyograo commented Aug 12, 2016

untergeek Aug 17, 2016

talevy Aug 17, 2016

suyograo Aug 17, 2016

jsvd Aug 17, 2016

untergeek Aug 17, 2016

untergeek commented Aug 17, 2016

talevy Aug 17, 2016

suyograo Aug 17, 2016

suyograo Aug 17, 2016

talevy Aug 17, 2016

talevy Aug 17, 2016

suyograo Aug 17, 2016

talevy commented Aug 17, 2016

Update template to use ES 5.x mapping #462

Update template to use ES 5.x mapping #462

Conversation

suyograo commented Aug 4, 2016

untergeek commented Aug 4, 2016

untergeek commented Aug 4, 2016

suyograo commented Aug 4, 2016

jordansissel commented Aug 4, 2016

suyograo commented Aug 4, 2016

jordansissel commented Aug 4, 2016

untergeek commented Aug 4, 2016

suyograo commented Aug 4, 2016

jordansissel commented Aug 8, 2016 • edited Loading

suyograo commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untergeek commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talevy commented Aug 17, 2016

jordansissel commented Aug 8, 2016 •

edited

Loading