Allow template pattern to be overwritten #4769

ruflin · 2017-07-27T06:47:28Z

With the current template loading is was not possible to overload the pattern used as name and pattern were mixed. For example in the case where someone wanted to define pattern name a that applies to the indice a this was not possible because the pattern generated was a-* so it didn't apply to the index a. This change now allows to set pattern and name separately.

To make sure index pattern and templates are in sync, the beat will not start in case the index pattern was modified but the template patterns not. We should figure out later some automated mechanisms here.

We should find a better solution for 6.x to require less config options and also include dashboard generation in this.

ruflin · 2017-07-27T06:48:01Z

libbeat/_meta/config.reference.yml

-# The version of the beat will always be appended to the given name
-# so the final name is beatname-%{[beat.version]}.
-#setup.template.name: "beatname"
+# The f


Ignore this doc and below for the moment. Take a look at the PR description on the behaviour.

ruflin · 2017-07-27T06:56:22Z

It would be nice if the template name would be based on the elasticsearch index name. the problem with that for the moment is that the index name includes the daily pattern: index: "beatname-%{[beat.version]}-%{+yyyy.MM.dd}"

We could change this to:

elasticsearch
  index: "beatname-%{[beat.version]}"
  pattern: "-%{+yyyy.MM.dd}"

This would allow the template loading to take by default the index name as the name for the template. Problem with this it's an additional break in case someone configured a different index-name.

ruflin · 2017-07-27T07:50:05Z

libbeat/_meta/config.reference.yml

+
+# Template patttern. By default the template patter is "-%{[beat.version]}-*" to apply to the default index settings.
+# The first part is the version of the beat and then -* is used to match all daily indicies.
+#setup.template.pattern: "beatname-%{[beat.version]}-*"


Do we even need the -* for the pattern or could we just use * so it would also apply do indices which are exactly like the index name?

Not sure I understand this suggestion. You mean something lie beatname-%{[beat.version]}*?

ruflin · 2017-07-28T08:03:24Z

libbeat/template/template.go

+
+	pattern := config.Pattern
+	if pattern == "" {
+		pattern = name + "-*"


Should we remove here the - and make this just *?

This is just the default, right? I'd say let's keep the -*, so that it's less likely to apply accidentally to other indices.

ruflin · 2017-07-28T08:17:13Z

libbeat/beat/beat.go

+		}
+
+		// Get ES Index name for comparison
+		esCfg := struct {


@urso Should I also check for indicies here and len==0?

ruflin · 2017-07-28T08:18:15Z

metricbeat/docs/fields.asciidoc

@@ -2790,28 +2790,28 @@ Bytes in non-idle span.


 [[exported-fields-graphite]]
-== graphite Fields
+== graphite fields


@dedemorton Seems like some of your recent changes are revert? I wonder why the previous builds went green?

tsg · 2017-07-28T09:02:26Z

libbeat/template/template.go

+	return t.name
+}
+
+// GetPattern returns the name of the template which is {index}-{version}


Comment seems out of date.

tsg · 2017-07-28T09:04:02Z

libbeat/beat/beat.go

+		}
+
+		if esCfg.Index != "" && (cfg.Name == "" || cfg.Pattern == "") {
+			return fmt.Errorf("setup.template.name and setup.template.pattern have to be set if index name is modified.")


Does the name really have to be changed? I'm thinking we could only check on the setup.template.pattern?

It will be kind of strange if the name is not changed. Assuming someone changes the index to test-{daily} and the pattern to test-* he then sees beatname-{version}-{daily} as the template name with test-* as pattern. In case that template is already there, it would not be overwritten and he would still not have the expected template.

tsg

Generally LGTM. Waiting for a review from @urso as well.

The version number of apm-server and the system tests have to be kept manually in sync until elastic/beats#4769 is merged.

tsg · 2017-08-09T08:51:00Z

@ruflin I tested this branch, but when using a configuration like this (which is what's suggested in the docs):

setup.template:
  name: test
  pattern: "metricbeat-%{[beat.version]}-*"

I get this in the resulting template:

{
  "test": {
    "order": 1,
    "index_patterns": [
      "metricbeat-%{[beat.version]}-*"
    ],

So it seems like the variables are not resolved as I was expecting.

urso · 2017-08-09T12:37:04Z

I don't see any use of format strings being applied. It's all strings only. When/how does beat.version get resolved? How do you handle event fields being used, that are specific to a module/prospector? e.g. %{[fields.index]}-%{+yyyy.MM.dd}? In this case you have to log an error and ask the user to explicitly configure the template pattern (if possible).

ruflin · 2017-08-10T07:45:33Z

Currently the default works when not set because it is set internal. But as @urso said there is currently no handling of the special strings. My suggestion is to support in this version everything under beat.* as that is static but not support dynamic fields from event (I would not know how we could do this). Like this the defaults also work when they are written out in the config and it should cover most of the case. We should open later a separate issue on how and if we want to support the more complex cases with fields.index for example.

urso · 2017-08-10T11:56:22Z

libbeat/template/template.go

+		}
+
+		logp.Debug("template", "Lookup key for pattern not available: %s", key)
+		return nil, nil


should we handled this as error? Compiling a pattern with missing field might give you an invalid index name.

yes, will return an error if there is a pattern but doesn't match either of the two.

urso · 2017-08-10T12:00:19Z

libbeat/template/template.go

+
+		if key == "[beat.version]" {
+			return Formatter{s: bV.String()}, nil
+		}


Date patterns start with + character. See: https://github.com/elastic/beats/blob/master/libbeat/common/fmtstr/formatevents.go#L252

That is, we can replace %{+<pattern>} with * in here.

Is there an easy way to reuse this pattern compiling?

urso · 2017-08-10T12:03:05Z

libbeat/template/template.go

@@ -167,3 +204,19 @@ func loadYaml(path string) (Fields, error) {
 	}
 	return fields, nil
 }
+
+type Formatter struct {


don't leak/export this type if you don't need it. Also template.Formatter is kind of a bad choice here.

agree, need to come up with a better name

urso · 2017-08-10T12:05:53Z

libbeat/template/template.go

@@ -41,6 +45,39 @@ func New(beatVersion string, beatName string, esVersion string, config TemplateC
 		pattern = name + "-*"
 	}

+	compiler := func(key string, ops []fmtstr.VariableOp) (fmtstr.FormatEvaler, error) {


Any plan to handle ops? Event format strings can have defaults if the key is missing in the event. See: https://github.com/elastic/beats/blob/master/libbeat/common/fmtstr/formatevents.go#L280

One can use %{[my.field]:defaultValue} in event format strings.

For now to keep it as simple as possible I suggest to return an error in case any ops are used.

tsg

Tested this one manually, looks good 👍

With the current template loading is was not possible to overload the pattern used as name and pattern were mixed. For example in the case where someone wanted to define pattern name `a` that applies to the indice `a` this was not possible because the pattern generated was `a-*` so it didn't apply to the index `a`. This change now allows to set pattern and name separately. To make sure index pattern and templates are in sync, the beat will not start in case the index pattern was modified but the template patterns not. We should figure out later some automated mechanisms here. We should find a better solution for 6.x to require less config options and also include dashboard generation in this.

ruflin · 2017-08-21T13:16:11Z

Rebased and squashed again.

ruflin · 2017-08-21T14:51:09Z

jenkins, test it

ruflin · 2017-08-21T21:16:04Z

jenkins, retest it

With the current template loading is was not possible to overload the pattern used as name and pattern were mixed. For example in the case where someone wanted to define pattern name `a` that applies to the indice `a` this was not possible because the pattern generated was `a-*` so it didn't apply to the index `a`. This change now allows to set pattern and name separately. To make sure index pattern and templates are in sync, the beat will not start in case the index pattern was modified but the template patterns not. We should figure out later some automated mechanisms here. We should find a better solution for 6.x to require less config options and also include dashboard generation in this. (cherry picked from commit 02a59a1)

Changes: * Add support for cloud id. See elastic/beats#4964 for more details. This could simplify the onboard for users that use apm-server with the Cloud in the future. * Be able to overwrite index pattern in the index template. See elastic/beats#4769 for more details. This allows to clean up our system tests.

Important changes / additions: * Add support for cloud id. See elastic/beats#4964 for more details. This could simplify the onboard for users that use apm-server with the Cloud in the future. * Be able to overwrite index pattern in the index template. See elastic/beats#4769 for more details. This allows to clean up our system tests.

dedemorton · 2018-01-22T19:09:39Z

Removing needs_docs labels because template.name and template.pattern options are now documented. If additional doc work is required, please open an issue against the docs.

With the current template loading is was not possible to overload the pattern used as name and pattern were mixed. For example in the case where someone wanted to define pattern name `a` that applies to the indice `a` this was not possible because the pattern generated was `a-*` so it didn't apply to the index `a`. This change now allows to set pattern and name separately. To make sure index pattern and templates are in sync, the beat will not start in case the index pattern was modified but the template patterns not. We should figure out later some automated mechanisms here. We should find a better solution for 6.x to require less config options and also include dashboard generation in this. (cherry picked from commit 2a8f795)

ruflin added discuss Issue needs further discussion. in progress Pull request is currently in progress. labels Jul 27, 2017

ruflin commented Jul 27, 2017

View reviewed changes

ruflin mentioned this pull request Jul 27, 2017

Change index config in elasticsearch output #4771

Closed

ruflin force-pushed the template-pattern branch from b0c373a to f05ecb1 Compare July 28, 2017 08:00

ruflin commented Jul 28, 2017

View reviewed changes

ruflin force-pushed the template-pattern branch from f05ecb1 to b7e9e00 Compare July 28, 2017 08:16

ruflin commented Jul 28, 2017

View reviewed changes

tsg reviewed Jul 28, 2017

View reviewed changes

tsg approved these changes Jul 28, 2017

View reviewed changes

ruflin force-pushed the template-pattern branch 2 times, most recently from ab01baa to 5bc30f9 Compare August 1, 2017 12:58

ruflin mentioned this pull request Aug 6, 2017

Fix broken system tests elastic/apm-server#7

Merged

ruflin force-pushed the template-pattern branch from 50d71f4 to 882e29d Compare August 7, 2017 06:38

simitt pushed a commit to elastic/apm-server that referenced this pull request Aug 7, 2017

Fix broken system tests (#7)

14e949a

The version number of apm-server and the system tests have to be kept manually in sync until elastic/beats#4769 is merged.

ruflin added review and removed discuss Issue needs further discussion. in progress Pull request is currently in progress. labels Aug 7, 2017

ruflin force-pushed the template-pattern branch from 882e29d to bb589c3 Compare August 7, 2017 07:48

ruflin mentioned this pull request Aug 8, 2017

Auto renaming of template logstash-plugins/logstash-output-elasticsearch#633

Open

urso reviewed Aug 10, 2017

View reviewed changes

monicasarbu added libbeat discuss Issue needs further discussion. labels Aug 15, 2017

tsg approved these changes Aug 21, 2017

View reviewed changes

ruflin force-pushed the template-pattern branch from a67bb03 to 3ea9c88 Compare August 21, 2017 13:16

ruflin added needs_backport PR is waiting to be backported to other branches. needs_docs labels Aug 21, 2017

urso approved these changes Aug 21, 2017

View reviewed changes

urso merged commit 02a59a1 into elastic:master Aug 21, 2017

ruflin deleted the template-pattern branch August 22, 2017 06:24

ruflin mentioned this pull request Aug 22, 2017

Cherry-pick #4769 to 6.0: Allow template pattern to be overwritten #4965

Merged

ruflin removed the needs_backport PR is waiting to be backported to other branches. label Aug 22, 2017

tsg added the needs_backport PR is waiting to be backported to other branches. label Aug 22, 2017

tsg removed the needs_backport PR is waiting to be backported to other branches. label Aug 22, 2017

ruflin mentioned this pull request Aug 23, 2017

Update beats to bd6981d elastic/apm-server#92

Merged

dedemorton removed the needs_docs label Jan 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow template pattern to be overwritten #4769

Allow template pattern to be overwritten #4769

ruflin commented Jul 27, 2017 •

edited

Loading

ruflin Jul 27, 2017

ruflin commented Jul 27, 2017

ruflin Jul 27, 2017

tsg Jul 27, 2017

ruflin Jul 27, 2017

ruflin Jul 28, 2017

tsg Jul 28, 2017

ruflin Jul 28, 2017

ruflin Jul 28, 2017

ruflin Jul 28, 2017

tsg Jul 28, 2017

ruflin Jul 28, 2017

tsg Jul 28, 2017

ruflin Jul 28, 2017

tsg left a comment

tsg commented Aug 9, 2017

urso commented Aug 9, 2017

ruflin commented Aug 10, 2017

urso Aug 10, 2017

ruflin Aug 15, 2017

urso Aug 10, 2017

ruflin Aug 15, 2017

urso Aug 10, 2017

ruflin Aug 15, 2017

urso Aug 10, 2017

ruflin Aug 15, 2017

tsg left a comment

ruflin commented Aug 21, 2017

ruflin commented Aug 21, 2017

ruflin commented Aug 21, 2017

dedemorton commented Jan 22, 2018

Allow template pattern to be overwritten #4769

Allow template pattern to be overwritten #4769

Conversation

ruflin commented Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

ruflin commented Jul 27, 2017

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

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

tsg left a comment

Choose a reason for hiding this comment

tsg commented Aug 9, 2017

urso commented Aug 9, 2017

ruflin commented Aug 10, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsg left a comment

Choose a reason for hiding this comment

ruflin commented Aug 21, 2017

ruflin commented Aug 21, 2017

ruflin commented Aug 21, 2017

dedemorton commented Jan 22, 2018

ruflin commented Jul 27, 2017 •

edited

Loading