Add a reloadable objects registry

[exekias]

This change allows us to register reloadable blocks of configurations while instantiating them. Central management will use this registry to retrieve reloadable blocks and handle their configs.

[ph]

@exekias Can it be possible to not make it a package variable and instead contains everything within the type/instance of the type? I don't know all the implementation details but as an information we are trying to remove any package variable and global variable from the core so when we see them we always ask do we need really need them?

[exekias]

That's an option, but in practice, that would mean we need to pass an instance of the registry to all the running parts of the beat. Is there any plan to introduce something like pkg/context for this?

[ph]

@exekias we want to introduce this, but we are a bit far from that, I am toying with the idea in beatless.

Let me clarify, when I first read your PR, I thought why not use the global feature registry to hold that and just wrap it in some function calls to do the type assertions, this is what I am doing in the beatless PR.

But after taking a step back I thought it's probably better to have this specific registry which deal with alive config changes. But I am not sure how you want to structure it with the application but I would expect some kind of listener that will trigger events? Maybe a bit of details or pointers of the usage would help me?

With that in mind I think what we can do without introducing a pkg/context is to make sure that the registry you create is instead a type that your Registry variable instantiate, this is what you almost do.

But instead of exposing the internals of the registry in the exposed function like the following you do like the next example which hide the implementation details but provide package accessable function. Will help if you add more logic to the class to be able to unit test it without having side effects on the running code.

func MustRegister(name string, r Reloadable) {
	register.Lock()
	defer register.Unlock()
	if r == nil {
		panic("Got a nil object")
	}
	if _, ok := register.confs[name]; ok {
		panic(fmt.Sprintf("%s configuration is already registered", name))
	}
	register.confs[name] = r
}

var Registry ...

func MustRegister(name string, r Reloadable) {
    err := Registry.Register(name, r)
    if err != nil {
     panic(".... something bad happened report the error.")
    }
}

[exekias]

I think I got it, will do the changes! Thanks for your input 👍

[ph]

Uh I didn't know that assert!

[ruflin]

What about returning an error instead?

[exekias]

All the register calls should happen while initializing the beat, if an error happens here it means we did something really wrong, it would be interesting to know that early, I think we follow this pattern in many other registries?

[ruflin]

ok, lets follow the common pattern.

[ph]

It's good practice to offer both workflow.

[exekias]

I added another commit to offer both in a non-global fashion

[ruflin]

What about returning an error instead?

[ruflin]

See above, same for next panic.

[jsoriano]

Lowercase error messages

[exekias]

Thanks everyone for the reviews, I think this is ready for a second look!

[jsoriano]

Not used?

[jsoriano]

Btw, is there a reason to have two interfaces, one for a single entity and another one for lists of entities?
Methods could accept config ...*ConfigWithMeta and single entities could be stored as lists of one element.
Also, is it expected to have entities and list of entities with the same name?

[exekias]

I don't expect entities sharing the name, these should map to a config key, like metricbeat.modules or output.

The problem with having a single definition is that then, the objects have to handle possible errors in the Reload call. Like for instance, having the output object receiving a list of configs. This split is semantic, as the two entities behave differently.

[ph]

The suggestion from @jsoriano has some values, because currently we could call Register and RegisterList with the same name (same map key). Having a single interface for both concept would mean that we only have one map.