Disable dynamic/Implement static mappings #10638

jbudz · 2017-03-01T17:22:54Z

Currently Kibana creates most mappings for the .kibana index on the client as needed. This code is split up between separate applications, and in some cases mappings aren't defined. This makes it difficult for external tools or kibana plugins to push .kibana data with correct mappings and can cause unexpected errors. In 6.0 we will also need to reindex and a complete definition will help with this.

When a new .kibana index is created, this disables dynamic mappings for everything except .kibana/config, disables dynamic type creation, and pushes all mappings.

Scenarios from this PR:

Kibana index exists with partial mappings
- No mappings are pushed, client side code pushes mappings if needed
- If there aren't client side mappings defined, dynamic mapping is still enabled and is created if needed
- Tools that push dashboards will continue to have wrong mappings
Kibana index doesn't exist
- Push full mappings, strict mappings
- Tools that push dashboards will now have the correct mappings

Closes #4964

jbudz · 2017-03-01T17:34:25Z

src/core_plugins/elasticsearch/lib/kibana_index_mappings.js

  config: {
+    dynamic: true,


we might be able to get away with enabled: false on this, thoughts? buildNum is used but otherwise we aren't searching

epixa · 2017-03-01T17:50:23Z

@Bargs Am I dreaming, or did you also take a swing at dynamic mappings in the past?

Bargs

Can we avoid having parallel mapping definitions in the codebase e.g.

kibana/src/ui/public/index_patterns/_index_pattern.js

Line 40 in 9c28e63

const mapping = mappingSetup.expandShorthand({

?

Bargs · 2017-03-01T20:20:39Z

src/core_plugins/elasticsearch/lib/kibana_index_mappings.js

@@ -1,16 +1,223 @@
 export const mappings = {
+  '_default_': {
+    'dynamic': 'false'


This should probably be strict shouldn't it?

Added in de9d04a

Bargs · 2017-03-01T20:22:43Z

src/core_plugins/elasticsearch/lib/kibana_index_mappings.js

+  '_default_': {
+    'dynamic': 'false'
+  },
+  dashboard: {


It feels like these mappings should be a part of each individual plugin's definition rather than crammed into the elasticsearch plugin, doesn't it? Feel free to ignore me if that's already the plan and this is just a stopgap.

👍 working on this one now. The plan is to expose a registerMappings method on the elasticsearch plugin. I was concerned there could be timing issues when collecting these registrations, but it looks like it will work from the plugin's init method.

Added in 00cd9e0

Bargs · 2017-03-01T20:34:38Z

@epixa nah, you might be thinking of the work I did on string -> text/keyword.

jbudz · 2017-03-01T22:13:32Z

Agree on removing client side mappings but I left them in for 5.x reasons. Currently these will only be applied if the Kibana index doesn't exist. If it already exists but doesn't have the full set of mappings we want the client side mappings to happen. I'd like some idempotent put mapping operation to happen on startup but we'll need to get the functionality in for reindexing .kibana at the same time.

epixa · 2017-03-25T16:17:51Z

This has conflicts, FYI

jbudz · 2017-03-28T11:57:04Z

jenkins, test this

jbudz · 2017-03-28T15:44:55Z

jenkins, test this

tylersmalley · 2017-03-30T20:31:13Z

src/core_plugins/elasticsearch/lib/kibana_index_mappings.js

 };
+
+export function assignMappings(mappings) {
+  Object.assign(defaultMappings, mappings);


Are we concerned with, or should we provide protections against, someone overwriting our properties stored in the mappings?

Added in 7f333ed

spalger · 2017-03-31T00:43:58Z

This needs to make sure that each mapping exists even before creating a .kibana index. If a user upgrades to this implementation after starting kibana but before saving a dashboard, or installs a new plugin which adds new mappings, the .kibana index won't have the correct mappings.

jbudz · 2017-03-31T15:41:48Z

@spalger my thoughts were to address that with the later upgrade process. As it stands now, I left all of the client side code to create mappings if the type doesn't exist. Eventually the checks won't be needed with the upgrade process in place, so instead of transitioning the client side checks to the server I opted for not doing anything.

A couple scenarios from this PR:

Kibana index exists with partial mappings
- No mappings are pushed, client side code pushes mappings if needed
- If there aren't client side mappings defined, dynamic mapping is still enabled and is created if needed
- Tools that push dashboards will continue to have wrong mappings
Kibana index doesn't exist
- Push full mappings, strict mappings
- Tools that push dashboards will now have the correct mappings
With the upgrade tool:
- We remove client side mappings
- If the mappings are wrong, we reindex with full mappings

The only case this covers is plugins or services pushing visualizations, dashboards post startup, which isn't needed for 5.4. It's fairly low impact, but may be safer to roll together with the upgrade tool. Any thoughts on edge cases I'm missing? Or waiting to include this with a more thorough upgrade process?

tylersmalley · 2017-04-13T23:35:01Z

Jenkins, test it

Bargs · 2017-04-24T20:21:25Z

LGTM

* Disable dynamic mappings * Disable automatic type creation * Centralize .kibana mappings * Use dynamic strict instead of false * Register mappings per plugin * Check for registered mapping conflicts * Expose mappings on server scope instead of module * Stub elasticsearch mappings class * Add tests for KibanaMappings class * [mappings tests] to.an -> to.be.an * Return on mapping conflict and include plugin id if provided * [uiExports] Add mappings uiExport type * Use uiExports config in mappings tests * Clean up ui_mappings test * Revert plugin require ordering * Pull uiExports mappings from kibana object isntead of kbnServer

epixa · 2017-04-25T18:28:16Z

@jbudz Don't forget to backport this

* Disable dynamic/Implement static mappings (#10638) * Disable dynamic mappings * Disable automatic type creation * Centralize .kibana mappings * Use dynamic strict instead of false * Register mappings per plugin * Check for registered mapping conflicts * Expose mappings on server scope instead of module * Stub elasticsearch mappings class * Add tests for KibanaMappings class * [mappings tests] to.an -> to.be.an * Return on mapping conflict and include plugin id if provided * [uiExports] Add mappings uiExport type * Use uiExports config in mappings tests * Clean up ui_mappings test * Revert plugin require ordering * Pull uiExports mappings from kibana object isntead of kbnServer * [test] Update index mapping to look for keyword

jbudz · 2017-04-25T19:06:27Z

5.x: d5dda02

spalger · 2017-04-25T22:58:01Z

🎉

epixa · 2017-04-26T14:24:58Z

src/ui/ui_mappings.js

+  }
+}
+
+export { MappingsCollection };


In the future, we should not export raw objects like this. This is not valid JavaScript, it's just something that babel supports because it happens to behave similarly to CommonJS modules. Export the class itself instead.

jbudz added review v5.4.0 v6.0.0 labels Mar 1, 2017

jbudz requested review from tylersmalley and epixa March 1, 2017 17:22

jbudz added the Team:Operations Team label for Operations Team label Mar 1, 2017

jbudz commented Mar 1, 2017

View reviewed changes

jbudz added updates_needed and removed review labels Mar 1, 2017

Bargs suggested changes Mar 1, 2017

View reviewed changes

jbudz force-pushed the feature/disable-dynamic-mappings branch 2 times, most recently from a872e9c to 02e24fc Compare March 1, 2017 23:54

jbudz added review tests_needed and removed updates_needed labels Mar 2, 2017

jbudz requested a review from spalger March 21, 2017 18:24

spalger changed the title ~~Disable dynamic mappings~~ Disable dynamic/Implement static mappings Mar 21, 2017

jbudz force-pushed the feature/disable-dynamic-mappings branch 2 times, most recently from 6cf51bd to a28b530 Compare March 27, 2017 20:47

epixa added the blocker label Mar 28, 2017

tylersmalley reviewed Mar 30, 2017

View reviewed changes

jbudz force-pushed the feature/disable-dynamic-mappings branch from a28b530 to 7f333ed Compare March 31, 2017 15:16

spalger mentioned this pull request Apr 24, 2017

[WIP] Saved object apis #10967

Closed

Bargs approved these changes Apr 24, 2017

View reviewed changes

jbudz added 16 commits April 25, 2017 10:54

Disable dynamic mappings

629d210

Disable automatic type creation

ff94969

Centralize .kibana mappings

544e749

Use dynamic strict instead of false

31711f7

Register mappings per plugin

dc02bcd

Check for registered mapping conflicts

6a35d52

Expose mappings on server scope instead of module

4e3ddc4

Stub elasticsearch mappings class

355c861

Add tests for KibanaMappings class

3e490f3

[mappings tests] to.an -> to.be.an

4e73b93

Return on mapping conflict and include plugin id if provided

831e6de

[uiExports] Add mappings uiExport type

5a88afa

Use uiExports config in mappings tests

6a5d92c

Clean up ui_mappings test

d60db1d

Revert plugin require ordering

90a9796

Pull uiExports mappings from kibana object isntead of kbnServer

6adb460

jbudz force-pushed the feature/disable-dynamic-mappings branch from c46ee79 to 6adb460 Compare April 25, 2017 15:23

jbudz merged commit ea7bfcc into elastic:master Apr 25, 2017

epixa reviewed Apr 26, 2017

View reviewed changes

jimgoodwin added the release_note:enhancement label Jun 26, 2017

jbudz mentioned this pull request Sep 20, 2017

ES 5.3.0 + Kibana 5.3.0 => The [string] field is deprecated, please use [text] or [keyword] instead on [title] #10993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable dynamic/Implement static mappings #10638

Disable dynamic/Implement static mappings #10638

jbudz commented Mar 1, 2017 •

edited

Loading

jbudz Mar 1, 2017

epixa commented Mar 1, 2017

Bargs left a comment

Bargs Mar 1, 2017

jbudz Mar 1, 2017

Bargs Mar 1, 2017

jbudz Mar 1, 2017 •

edited

Loading

jbudz Mar 1, 2017

Bargs commented Mar 1, 2017

jbudz commented Mar 1, 2017

epixa commented Mar 25, 2017

jbudz commented Mar 28, 2017

jbudz commented Mar 28, 2017

tylersmalley Mar 30, 2017 •

edited

Loading

jbudz Mar 31, 2017

spalger commented Mar 31, 2017

jbudz commented Mar 31, 2017 •

edited by spalger

Loading

tylersmalley commented Apr 13, 2017

Bargs commented Apr 24, 2017

epixa commented Apr 25, 2017

jbudz commented Apr 25, 2017

spalger commented Apr 25, 2017

epixa Apr 26, 2017

Disable dynamic/Implement static mappings #10638

Disable dynamic/Implement static mappings #10638

Conversation

jbudz commented Mar 1, 2017 • edited Loading

jbudz Mar 1, 2017

Choose a reason for hiding this comment

epixa commented Mar 1, 2017

Bargs left a comment

Choose a reason for hiding this comment

Bargs Mar 1, 2017

Choose a reason for hiding this comment

jbudz Mar 1, 2017

Choose a reason for hiding this comment

Bargs Mar 1, 2017

Choose a reason for hiding this comment

jbudz Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

jbudz Mar 1, 2017

Choose a reason for hiding this comment

Bargs commented Mar 1, 2017

jbudz commented Mar 1, 2017

epixa commented Mar 25, 2017

jbudz commented Mar 28, 2017

jbudz commented Mar 28, 2017

tylersmalley Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

jbudz Mar 31, 2017

Choose a reason for hiding this comment

spalger commented Mar 31, 2017

jbudz commented Mar 31, 2017 • edited by spalger Loading

tylersmalley commented Apr 13, 2017

Bargs commented Apr 24, 2017

epixa commented Apr 25, 2017

jbudz commented Apr 25, 2017

spalger commented Apr 25, 2017

epixa Apr 26, 2017

Choose a reason for hiding this comment

jbudz commented Mar 1, 2017 •

edited

Loading

jbudz Mar 1, 2017 •

edited

Loading

tylersmalley Mar 30, 2017 •

edited

Loading

jbudz commented Mar 31, 2017 •

edited by spalger

Loading