Use newly introduced idxmgmt supporter from libbeat #1865

simitt · 2019-01-31T22:35:04Z

Update beats framework
Implement apm specific idxmgmt supporter, allowing to set an apm specific index configuration, fixing Add indices defaults in code #1480
Implement a noop strategy for ILM support, fixing Disable ILM usage on APM #1852
rework system tests to generally reflect default configuration in the tests, and only override configuration in specific tests checking that overriding works.

add changelog

jalvz

Great idea setting the index names in the system tests!

jalvz · 2019-02-04T10:36:11Z

idxmgmt/supporter_factory.go

+		return err
+	}
+
+	tmplSet := tmplCfg.Name != "" && tmplCfg.Pattern != ""


if i am reading this right, following is simpler equivalence:

templateUnset := tmplCfg.Name == "" || tmplCfg.Pattern == "" if esCfg.Index != "" && templateUnset {

jalvz · 2019-02-04T10:44:51Z

idxmgmt/supporter.go

+		return nil, err
+	}
+
+	tmplCfg, err := unpackTemplateConfig(tmplConfig)


you are unpacking the template already here: https://github.com/elastic/apm-server/pull/1865/files#diff-d9c338e612b50b33b74b4b7e9feda72aR75, is it possible to do it just once and pass the template object around?

jalvz · 2019-02-04T10:48:26Z

idxmgmt/supporter.go

+) (*supporter, error) {
+
+	// creates a Noop ILM handler for now.
+	ilmSupporter, err := ilm.MakeDefaultSupporter(log, info, ilmConfig)


why do we need to say that we want the noop supporter twice, here and in https://github.com/elastic/apm-server/pull/1865/files#diff-ff7686b39bf90dc2520886fb874371a4R77?

Because it is not passed in as an argument to the idxmgmt.MakeDefaultSupporter. Libbeat code would need to change for this. I'll see if we can improve it, but let's keep iterations on beats side out of this PR.

jalvz · 2019-02-04T10:49:59Z

idxmgmt/supporter.go

+
+	// creates a Noop ILM handler for now.
+	ilmSupporter, err := ilm.MakeDefaultSupporter(log, info, ilmConfig)
+	if err != nil {


why error checking if the noop supported error is hard-coded to nil? is it because when we have a real ILM, then we already have this code done?

Because the noop supporter is not in our code base and I don't feel confident to realize in case some internas of the supporter change. The public API defines a possible error, so I do an error check.

jalvz · 2019-02-04T11:07:28Z

idxmgmt/supporter.go

+	buildSettings := outil.Settings{
+		Key:              "index",
+		MultiKey:         "indices",
+		EnableSingleOnly: true,


what does this mean? the doc string "if enabled a selector key in config will be generated, if key is present" is not very clear to me

Afaik, EnableSingleOnly ensures to create a default selector, which is defined in index. This default selector will be used when no other selector (defined in indices) matches the event. The combination of EnableSingleOnly and FailEmpty ensures that there an error is thrown if no selector is found for the event.

jalvz · 2019-02-04T11:43:30Z

tests/system/test_integration.py

-        # check that every document is indexed once (incl.1 onboarding doc)
-        assert 14 == self.es.count(index="test-apm*")['count']
+        # check that every document is indexed once in the expected index (incl.1 onboarding doc)
+        assert 7 == self.es.count(index=self.index_name)['count']


im starting to think that if i review properly this kind of changes, i won't finish reviewing never. so for the sake of transparency, i am assuming that these numbers are correct and make sense and i am not checking the data myself.

We are indexing 4 errors, 1 span, 1 transaction (you can look at the testdata used for verifying) and then there is also an onboarding event => 7 docs.

jalvz · 2019-02-04T11:46:35Z

idxmgmt/supporter.go

+	"github.com/elastic/beats/libbeat/template"
+)
+
+// functionality largely copied from libbeat


this is unfortunate, will there be any chance in the future to have this logic only in one place, or we will always need this?

I assume we can iterate on what we share and what not. From my perspective it'd be great if we could share the manager, which is currently a private type in libbeat. @urso how do you feel about exporting the idxmgmt.manager` so we can reuse directly? (I can put up a PR if you're generally fine with it).

I didn't export the types on purpose, due to us still having to figure out how to incorporate index per modules or multiple indices in general. I was also curious to see how apm-server will differ from libbeat over time.
For example I'd like to change the template management/setup itself. By this point I'm not sure about the reusability of the manager. It might be that you want to have your own manager implementation supporting a different template per index.
If you think it makes sense for apm-server to export some more symbols from libbeat, I'm fine with a PR.

I'm fine with keeping it as is for now, as the logic we copy is not too heavy. However, please keep us tightly in the loop when you change logic in beats or especially if there are any bug fixes etc.

jalvz · 2019-02-04T11:48:01Z

idxmgmt/supporter.go

+
+// functionality largely copied from libbeat
+
+type supporter struct {


since you are copying any ways, copying also some of the documentation can help to not have to go to the library code (the terminology is a bit abstract and confusing: supporter, manager, asseter, etc)

There's no documentation around the supporter in the libbeat library. But I can add some comments.

jalvz · 2019-02-04T11:58:33Z

idxmgmt/supporter.go

+	return m.load(false, false)
+}
+
+func (m *manager) load(forceTemplate, forcePolicy bool) error {


In the same spirit as the comment #1865 (comment)
I can't follow the details in this file, but I am assuming it works.
Likewise, I don't know how all the errors in idxmgnmt package can be triggered by the user, or why the they happen and how to map to use case scenarios; but I assume there is a good reason for them (the only one I understand is the one of checking that template+index pattern have to be set if index is also set, and it is great to have it there with that error message).

So, lmk if there are specific things about this that I could test.

Generally this logic is about importing the ES template on startup or when running ./apm-server setup template, and about the index/indices handling (setting the proper defaults, but allowing to override). If you want to play around with it / test it, those are the functionalities you should check.

jalvz · 2019-02-04T12:06:24Z

idxmgmt/supporter_factory.go

+	return newSupporter(log, info, cfg.Template, cfg.ILM, cfg.Migration.Enabled())
+}
+
+func checkTemplateESSettings(tmpl *common.Config, out common.ConfigNamespace) error {


As silly as it sounds, I generally have a better time reading full words even if they are long (indexTemplate, indexManagement) rather than made up abbreviations (templ, indxmngmt, etc), with some exception (config is ok, but cfg is harder already).
It is also easier if variable names in argument declaration are the same as the variable names in calling functions (template, output).
This is not important, and I am aware that happens all over the place. It is just that i am reading a lot of code lately :)

* Set index default configuration in code, by using new supporter interface * Set ILM noop supporter to disable ILM in APM * adapt system tests to use default indices configuration by default fixes elastic#1480 fixes elastic#1852

simitt · 2019-02-04T14:31:10Z

Rebased and force pushed to squash merge the commits while keeping the beats update as a separate commit.

jalvz · 2019-02-04T15:14:26Z

Thanks for the clarifications!

simitt changed the title ~~Use newly intoduced idxmgmt supporter from libbeat~~ Use newly introduced idxmgmt supporter from libbeat Jan 31, 2019

simitt force-pushed the adapt-index-handling branch from 6499cee to ba486db Compare February 1, 2019 08:27

jalvz reviewed Feb 4, 2019

View reviewed changes

jalvz approved these changes Feb 4, 2019

View reviewed changes

simitt added 2 commits February 4, 2019 15:28

Update beats framework to 2bbf828

a65ba75

Set default indices in code and disable ILM.

91eb23d

* Set index default configuration in code, by using new supporter interface * Set ILM noop supporter to disable ILM in APM * adapt system tests to use default indices configuration by default fixes elastic#1480 fixes elastic#1852

simitt force-pushed the adapt-index-handling branch from dc6ff2f to 91eb23d Compare February 4, 2019 14:30

simitt merged commit 28b69a9 into elastic:master Feb 4, 2019

simitt deleted the adapt-index-handling branch February 11, 2019 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use newly introduced idxmgmt supporter from libbeat #1865

Use newly introduced idxmgmt supporter from libbeat #1865

simitt commented Jan 31, 2019

jalvz left a comment

jalvz Feb 4, 2019

jalvz Feb 4, 2019

jalvz Feb 4, 2019

simitt Feb 4, 2019

jalvz Feb 4, 2019

simitt Feb 4, 2019

jalvz Feb 4, 2019

simitt Feb 4, 2019

jalvz Feb 4, 2019

simitt Feb 4, 2019

jalvz Feb 4, 2019

simitt Feb 4, 2019

urso Feb 4, 2019

simitt Feb 4, 2019

jalvz Feb 4, 2019

simitt Feb 4, 2019

jalvz Feb 4, 2019 •

edited

Loading

simitt Feb 4, 2019

jalvz Feb 4, 2019

simitt commented Feb 4, 2019

jalvz commented Feb 4, 2019


		// functionality largely copied from libbeat

		type supporter struct {

Use newly introduced idxmgmt supporter from libbeat #1865

Use newly introduced idxmgmt supporter from libbeat #1865

Conversation

simitt commented Jan 31, 2019

jalvz left a 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

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

jalvz Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simitt commented Feb 4, 2019

jalvz commented Feb 4, 2019

jalvz Feb 4, 2019 •

edited

Loading