WIP - multiple templates #9247
WIP - multiple templates #9247
Conversation
| // Array to store dynamicTemplate parts in | ||
| dynamicTemplates []common.MapStr | ||
|
|
||
| defaultFields []string | ||
| ) | ||
|
|
||
| type Templates []Template |
There was a problem hiding this comment.
exported type Templates should have comment or be unexported
| ) | ||
|
|
||
| func DefaultTemplateCfg() TemplateConfig { |
There was a problem hiding this comment.
exported function DefaultTemplateCfg should have comment or be unexported
| Overwrite bool `config:"overwrite"` | ||
| } | ||
|
|
||
| type TemplatesConfig struct { |
There was a problem hiding this comment.
exported type TemplatesConfig should have comment or be unexported
| "github.com/elastic/beats/libbeat/common" | ||
| ) | ||
|
|
||
| func Unpack(c *common.Config) (*TemplatesConfig, error) { |
There was a problem hiding this comment.
exported function Unpack should have comment or be unexported
There was a problem hiding this comment.
We need to export this?
Why no func (tc *TemplatesConfig) Unpack(c *common.Config) error ? This would be used by go-ucfg.
There was a problem hiding this comment.
Basically because I didn't see a lot of value in manually creating an empty TemplatesConfig outside and then call the unpack function on it. But I see how this makes sense from a consistency point, so will change.
| for _, name := range names { | ||
| data, ok := FieldsRegistry[beat][name] | ||
| if !ok { | ||
| return nil, errors.New(fmt.Sprintf("No fields available for %s", name)) |
There was a problem hiding this comment.
should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)
| setup.template.templates: | ||
| - name: "metricbeat-mix" | ||
| pattern: "*" | ||
| modules: "aerospike,apache,system" |
There was a problem hiding this comment.
Could we name this more generic. Perhaps call it fields: ["transaction"]` instead. This matches better the APM case I would hope.
Not sure if I would already introduce support for a list. We could start with just supporting one entry and migrating to a list later on if needed should not be a breaking change.
| pattern: "*" | ||
| modules: "aerospike,apache,system" | ||
| overwrite: true | ||
| - name: "metricbeat-docker" |
There was a problem hiding this comment.
If multiple templates are used, should we always postfix it by the version?
All the multi template options I think are great for the default cases in Beats and APM but if users want to use additional very advanced use cases, they should manually set it up. There are too many things that can go wrong.
Historically a lot of users have remove the version number from indices and templates which now becomes an issue during migrations so I would rather limit the options and flexibility we give the users by deafult.
There was a problem hiding this comment.
I would not introduce adding some auto-magical version to templates and change how the version in indices is added with this PR. It is already very closely related to ILM and general config changes. I suggest we discuss version handling completely separate.
There was a problem hiding this comment.
In the current ILM PR versions are added magically and the users can't remove them (on purpose).
There was a problem hiding this comment.
I am not a fan of auto magic either, but the version is really helpful when you do migration from beats version.
@ruflin in the example config for the name we could use a string with field reference, so its not automagic but we give them the tips.
There was a problem hiding this comment.
+1 on giving them tips if it does not complicate things.
Not sure what you meant by "but the version is really help when you do migration from beats"? Sounds like we are on the same page users should not be able to remove the version?
| #setup.template.fields: "${path.config}/fields.yml" | ||
| #setup.template.enabled: false | ||
| #setup.template.overwrite: true | ||
| setup.template.templates: |
There was a problem hiding this comment.
I would use setup.templates:
There was a problem hiding this comment.
I wanted to have it under the same namespace. However, this configs were mainly meant to allow playing around with multiple templates for now and show how they can be used.
As there are ongoing discussions around how to group together configs that belong together, #7963 (comment), we might end up putting the templates under some completely different group.
My focus was on the internal implementation and we could still keep the logic I built when the user facing config changes. That's actually the reason I introduced the Setup https://github.com/elastic/beats/pull/9247/files/7ff9c687751ca970678a2856fe19fc14ec6f1356#diff-b01453e5fd277f7039381e366330a2d2R24 method.
There was a problem hiding this comment.
Concerning the naming @urso and myself had few followup discussion where it should be, but not clear plan yet.
I think setup.templates is good choice.
There was a problem hiding this comment.
We probably need to change it again when combining with ILM. Will think about it a bit more.
| func GetFieldsFor(beat string, names []string) ([]byte, error) { | ||
| var fields []byte | ||
| for _, name := range names { | ||
| data, ok := FieldsRegistry[beat][name] |
There was a problem hiding this comment.
An idea I just had: What if we can allow to define dependencies when registering fields. For example the system fields depend on the common fields. And common fields depend on the ecs fields. Like this when generating the fields we could loop through the dependencies and get always the full result.
There was a problem hiding this comment.
I'll look into that when implementing it, but generally not planning on making any dependencies configurable.
There was a problem hiding this comment.
No need to look into it now. I think as you mentioned, outside of the scope of this PR.
| @@ -64,7 +64,7 @@ type Beat struct { | |||
|
|
|||
| BeatConfig *common.Config // The beat's own configuration section | |||
|
|
|||
| Fields []byte // Data from fields.yml | |||
| //Fields []byte // Data from fields.yml | |||
There was a problem hiding this comment.
Why remove this? The libbeat/beat package is supposed to be the (most...) stable API beats devs are facing/interacting with. We should try to reduce the change of breakages... Maybe we've been better off if Fields would have been an interface or not been added at all to the Beats structure.
Was Fields even used?
There was a problem hiding this comment.
Afaics Fields was only used for creating the template, so it is not necessary any more with those changes.
There was a problem hiding this comment.
In case you consider removing this a breaking change, I can leave it. It wouldn't be used any more though, so to avoid confusion and errors, I prefer removing it.
There was a problem hiding this comment.
I did a quick look, I don't see it used outside of creating the template.
So it introduce a breaking change on the beat struct, we should have defined an interface for this to limit the risk.
Lets say that I've used Fields in my code how I move it to the new workflow?
Or How do I have access to the same data?
| "github.com/elastic/beats/libbeat/common" | ||
| ) | ||
|
|
||
| func Unpack(c *common.Config) (*TemplatesConfig, error) { |
There was a problem hiding this comment.
We need to export this?
Why no func (tc *TemplatesConfig) Unpack(c *common.Config) error ? This would be used by go-ucfg.
| ) | ||
|
|
||
| func DefaultTemplateCfg() TemplateConfig { |
| config := DefaultConfig | ||
| // NewESLoader creates a new Elasticsearch template loader | ||
| func NewESLoader(cfg *common.Config, client ESClient, beatInfo beat.Info) (*Loader, error) { | ||
| return newLoader(cfg, client, beatInfo, client.GetVersion()) |
There was a problem hiding this comment.
Once you rebase onto master client.GetVersion() will not return a string anymore.
There was a problem hiding this comment.
Already changed that, but haven't pushed yet.
| @@ -51,7 +48,7 @@ type Template struct { | |||
| } | |||
|
|
|||
| // New creates a new template instance | |||
| func New(beatVersion string, beatName string, esVersion string, config TemplateConfig) (*Template, error) { | |||
| func NewTemplate(beatVersion string, beatName string, esVersion string, config TemplateConfig) (*Template, error) { | |||
There was a problem hiding this comment.
It is more aligned with the general libbeat code, and since the template code needs to be touched anyways, it is a good opportunity to adapt this.
There was a problem hiding this comment.
I would keep the same name as before, we are in the template package so no need to repeat it in the constructor.
template.New instead of template.NewTemplate.
|
I implemented it as non breaking change to be backwards compatible. The config together with ILM needs to be figured out, but I'd like to do this in a next step, as separate templates can be seen as an independent pre-condition for ILM. |
|
@simitt quick question w.r.t:
Is this a current requirement (i.e., is the default policy we use in beats not good for APM?) or is it a preparation for an option that people may want to do different things with different indices? |
|
The current handling in beats only allows one policy for all index patterns. In APM we have different event types (transactions, spans, errors, ..). Spans hold more detailled information than transactions and need more storage. Therefore it makes sense to allow to have a different retention policy for spans than for transactions. For this we need multiple templates per beat, since the ilm policy is set in the template. For a first start we could use the default policy from libbeat, but we are aiming for having different policies already from the beginning, or at least the setup for it. |
| @@ -215,7 +209,7 @@ func NewBeat(name, indexPrefix, v string) (*Beat, error) { | |||
| Hostname: hostname, | |||
| UUID: id, | |||
| }, | |||
| Fields: fields, | |||
| //Fields: fields, | |||
There was a problem hiding this comment.
We should remove this.. following the change in the struct.
There was a problem hiding this comment.
Sure, once we decided to remove in the struct, I'll also remove it here. ATM both is commented out as discussions are going on.
| @@ -66,67 +75,92 @@ func NewLoader(cfg *common.Config, client ESClient, beatInfo beat.Info, fields [ | |||
| // template is written to index | |||
| func (l *Loader) Load() error { | |||
|
|
|||
There was a problem hiding this comment.
we can probably remove that space :)
| } | ||
| // Check if template already exist or should be overwritten | ||
| loaded := l.templateLoaded(templateName) | ||
| if !loaded || cfg.Overwrite { | ||
|
|
| @@ -64,7 +64,7 @@ type Beat struct { | |||
|
|
|||
| BeatConfig *common.Config // The beat's own configuration section | |||
|
|
|||
| Fields []byte // Data from fields.yml | |||
| //Fields []byte // Data from fields.yml | |||
There was a problem hiding this comment.
I did a quick look, I don't see it used outside of creating the template.
So it introduce a breaking change on the beat struct, we should have defined an interface for this to limit the risk.
Lets say that I've used Fields in my code how I move it to the new workflow?
Or How do I have access to the same data?
| Fields string `config:"fields"` | ||
| Overwrite bool `config:"overwrite"` | ||
| Settings map[string]string `config:"settings"` | ||
| } |
There was a problem hiding this comment.
Could we use type aliasing, this struct was move to the template package.
| func (l *Loader) templateLoaded(templateName string) bool { | ||
| if l.client == nil { | ||
| return false | ||
| } | ||
| status, _, _ := l.client.Request("HEAD", "/_template/"+templateName, "", nil, nil) |
There was a problem hiding this comment.
Any reason to have changed the visibility of this method?
I understand the name could be better.
There was a problem hiding this comment.
It is not used anywhere else, so I there is no reason for exporting it.
| logp.Debug("template", "Try loading template with name: %s", templateName) | ||
| if l.client == nil { | ||
| if _, err := os.Stdout.WriteString(template.StringToPrint() + "\n"); err != nil { | ||
| return fmt.Errorf("Error writing template: %v", err) |
There was a problem hiding this comment.
No capitalization in error string.
Also, I see that we check in both places that client is set before doing an actual call. Is there any reason why the client is could not have been set before using that type?
There was a problem hiding this comment.
When exporting the template to Stdout with the ./beat export template cmd no ES client would be set. Thus, this nil check is necessary to see if we should export to ES or to Stdout.
So far the whole template loading functionality was implemented twice, once here and once in the export code. I refactored to reuse the same code for both.
| @@ -51,7 +48,7 @@ type Template struct { | |||
| } | |||
|
|
|||
| // New creates a new template instance | |||
| func New(beatVersion string, beatName string, esVersion string, config TemplateConfig) (*Template, error) { | |||
| func NewTemplate(beatVersion string, beatName string, esVersion string, config TemplateConfig) (*Template, error) { | |||
There was a problem hiding this comment.
I would keep the same name as before, we are in the template package so no need to repeat it in the constructor.
template.New instead of template.NewTemplate.
| #setup.template.fields: "${path.config}/fields.yml" | ||
| #setup.template.enabled: false | ||
| #setup.template.overwrite: true | ||
| setup.template.templates: |
There was a problem hiding this comment.
Concerning the naming @urso and myself had few followup discussion where it should be, but not clear plan yet.
I think setup.templates is good choice.
| pattern: "*" | ||
| modules: "aerospike,apache,system" | ||
| overwrite: true | ||
| - name: "metricbeat-docker" |
There was a problem hiding this comment.
I am not a fan of auto magic either, but the version is really helpful when you do migration from beats version.
@ruflin in the example config for the name we could use a string with field reference, so its not automagic but we give them the tips.
|
Thanks for all the feedback. Closing this for now, will reopen when ready for the next round of review. |
When using ILM for APM Server we need to be able to have a separate template for every index so we can adapt the rollover policy per index, see elastic/apm-server#1483.
This is the early stage POC related allowing to have multiple templates for one beat.
TODO:
fields.goper module to either include common fields or merge them when creating the templateThis is early stage, with the intention to get some high level feedback to see if this conflicts with anything beats specific. @ruflin , @urso feedback very welcome.