Change template loading configuration to be part of setup by ruflin · Pull Request #4080 · elastic/beats

ruflin · 2017-04-21T14:25:36Z

Change config option to setup.template.* from outputs.elasticsearch.template.*
Move loading logic into template package
Remove template loading logic from elasticsearch output
Changelog updated
Template tests were moved from output to template package
Documentation was updated. Will need some more work for which a follow up Github issue will be created.
Add GetVersion() to elasticsearch client.
Introduce callback registration for elasticsearch output. This should be generalised later. The template loading registers only with the output client factory which means, the template is not loaded when connecting for loading dashboards, pipeline or monitoring data which is intended.

This is only migration the existing options. New options like outputting to a json file or load additional config options will be added in a follow up PR.

Part of elastic#3654 and elastic#3921

tsg · 2017-04-25T08:30:41Z

libbeat/beat/beat.go

Hmm, if I understand it correctly, this means that the template loading is only attempted once at startup not on every connection made to ES? If this is the case, then this is a regression for two reasons:

If ES is not up when the Beat start, the Beat will fail with an error. It used to "wait" for ES to come up

If for whatever reason ES is restarted without data, but the Beat is not restarted, the template is not loaded again, resulting in bad mapping.

Good point. This just go a lot trickier to implement as it now uses its own client.

TBH this is a deal breaker for me, especially the part where Beats don't start if ES is not up is going to really annoying to users.

Ideally we'd also have the pipelines loaded on each connection. It's currently done only on startup, but that can lead to errors similar to these. I think we need some sort of generic callback mechanism to do this.

tsg · 2017-04-25T08:31:02Z

libbeat/beat/beat.go

The template should be always loaded, not only on -setup.

It should be always loaded. The check on line 472 is different from the Dashboard one.

Ok, but then this check here is not needed?

if you run -setup but have setup.template.enabled: false what is the expected behaviour?

tsg · 2017-04-25T08:32:46Z

libbeat/beat/beat.go

There's a reference to b.Config.Dasboards there, by mistake?

tsg · 2017-04-25T09:22:42Z

libbeat/outputs/elasticsearch/client.go

Having this as global means it will also be applied to non-output ES clients (e.g. the one loading the dashboards, or the one loading the pipelines or the one for monitoring). That is perhaps OK, but mentioning it in case someone else thinks of something about that.

Of course, the global is kind of last resort solution. @urso any suggestions on how we can improve this? (Not necessarily in this PR).

That is also getting interesting in the case we would allow in the setup to define an elasticsearch host to load the template potentially with a different user. We already have something similar for the metrics.

Talking about metrics, this means also for the connection to the monitoring instance it would load the template? Not sure if this is intended.

* Change config option to setup.template.* from outputs.elasticsearch.template.* * Move loading logic into template package * Remove template loading logic from elasticsearch output * Changelog updated * Template tests were moved from output to template package * Documentation was updated. Will need some more work for which a follow up Github issue will be created. * Add `GetVersion()` to elasticsearch client. * Introduce callback registration for elasticsearch output. This should be generalised later. The template loading registers only with the output client factory which means, the template is not loaded when connecting for loading dashboards, pipeline or monitoring data which is intended. This is only migration the existing options. New options like outputting to a json file or load additional config options will be added in a follow up PR. Part of elastic#3654 and elastic#3921

elasticmachine · 2017-04-28T11:33:23Z

libbeat/beat/setup.go

@@ -0,0 +1,10 @@
+package beat
+
+type TemplateConfig struct {


[golint] _{reported by reviewdog 🐶}
exported type TemplateConfig should have comment or be unexported

elasticmachine · 2017-04-28T11:33:23Z

libbeat/template/config.go

@@ -0,0 +1,17 @@
+package template
+
+type TemplateConfig struct {


[golint] _{reported by reviewdog 🐶}
exported type TemplateConfig should have comment or be unexported

elasticmachine · 2017-04-28T11:33:23Z

libbeat/template/config.go

@@ -0,0 +1,17 @@
+package template
+
+type TemplateConfig struct {


[golint] _{reported by reviewdog 🐶}
type name will be used as template.TemplateConfig by other packages, and that stutters; consider calling this Config

elasticmachine · 2017-04-28T11:33:23Z

libbeat/template/load.go

+	"github.com/elastic/beats/libbeat/paths"
+)
+
+// TemplateLoader is a subset of the Elasticsearch client API capable of


[golint] _{reported by reviewdog 🐶}
comment on exported type ESClient should be of the form "ESClient ..." (with optional leading article)

elasticmachine · 2017-04-28T11:33:23Z

libbeat/template/load.go

+	GetVersion() string
+}
+
+type Loader struct {


[golint] _{reported by reviewdog 🐶}
exported type Loader should have comment or be unexported

elasticmachine · 2017-04-28T11:33:23Z

libbeat/template/load.go

+	beatInfo common.BeatInfo
+}
+
+func NewLoader(cfg *common.Config, client ESClient, beatInfo common.BeatInfo) (*Loader, error) {


[golint] _{reported by reviewdog 🐶}
exported function NewLoader should have comment or be unexported

elasticmachine · 2017-04-28T11:33:23Z

libbeat/template/load.go

+	}, nil
+}
+
+// loadTemplate checks if the index mapping template should be loaded


[golint] _{reported by reviewdog 🐶}
comment on exported method Loader.Load should be of the form "Load ..."

ruflin added in progress Pull request is currently in progress. libbeat labels Apr 21, 2017

ruflin force-pushed the move-template-to-setup branch 2 times, most recently from 31c0e65 to c93d68c Compare April 24, 2017 11:57

ruflin changed the title ~~[WIP] Change template loading to be part of the setup~~ Change template loading to be part of the setup Apr 24, 2017

ruflin added the review label Apr 24, 2017

tsg reviewed Apr 25, 2017

View reviewed changes

ruflin force-pushed the move-template-to-setup branch from 6a14fab to 27755db Compare April 25, 2017 09:07

tsg reviewed Apr 25, 2017

View reviewed changes

ruflin force-pushed the move-template-to-setup branch from 27755db to 5133bf6 Compare April 28, 2017 11:04

ruflin removed the in progress Pull request is currently in progress. label Apr 28, 2017

elasticmachine reviewed Apr 28, 2017

View reviewed changes

tsg merged commit 4ae0b89 into elastic:master May 4, 2017

tsg added the v6.0.0-alpha1 label May 4, 2017

monicasarbu deleted the move-template-to-setup branch May 7, 2017 21:12

monicasarbu changed the title ~~Change template loading to be part of the setup~~ Change template loading configuration to be part of setup May 8, 2017

tsg mentioned this pull request Jul 24, 2017

Document breaking changes in 6.0 #4737

Closed

28 tasks

		@@ -0,0 +1,17 @@
		package template

		type TemplateConfig struct {

Conversation

ruflin commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ruflin commented Apr 21, 2017 •

edited

Loading