[metricbeat] Change behavior of Registry.Modules() to make it more consistant.

[fearful-symmetry]

This came out of both #12648 and a discussion between @andrewkroh and I.

The mb.Registry.Modules() function is currently a tad weird. It'll only report modules that have been explicitly registered in init(). Not all modules do this. However, by nature of how metricsets are initialized, all metricsets are stored in the registry.

This means that as it stands, you can use the registry to access all metricsets, but not all modules. This changes that, so Modules() returns a complete list, using the keys of the metricSet dict. The idea of just augmenting the data from r.modules and de-duplicating the list was @andrewkroh 's, out of an abundance of caution.

[exekias]

@jsoriano I wonder if this works for lightweight modules?

[fearful-symmetry]

So, the new cockroachdb module is in xpack, and the registry doesn't even know it exists. The lone metricset doesn't get registered at all, so this part of the code can't see it.

[jsoriano]

As secondary sources (as light modules) cannot register modules, I didn't modify mb.Registry.Modules(). If we are listing now all modules (registered and non-registered ones), we should also list modules in the secondary source to keep consistency. I think all the needed pieces are there, we should add something like this:

if source := r.secondarySource; source != nil {
    sourceModules, err := source.Modules()
    .... check error and append modules ...
}

We should also check that mb.Registry.String() doesn't report duplicates now.

[fearful-symmetry]

Interesting. I just noticed that MetricSets() will look for data in secondary modules, but Modules() won't.

[fearful-symmetry]

Also @jsoriano so, MetricSets() is checking secondary sources, but it can't seem to see cockroachdb. Do you know how data makes its way into secondarySource ?

[jsoriano]

Also @jsoriano so, MetricSets() is checking secondary sources, but it can't seem to see cockroachdb. Do you know how data makes its way into secondarySource ?

Light modules are only included in licensed metricbeat, on it modules are loaded from the module directory under path.home. If you build and run from x-pack/metricbeat it should find it. You may need to add --strict.perms=false.

[fearful-symmetry]

@jsoriano I mean, where in the source do light modules get registered? "normal" x-pack modules get registered just fine. I just need to make a skeleton program capable of listing all modules.

[jsoriano]

@jsoriano I mean, where in the source do light modules get registered? "normal" x-pack modules get registered just fine. I just need to make a skeleton program capable of listing all modules.

Oh, light modules are not registered, they are "lazily" loaded when required. To list available modules in a secondary source you should call its Modules() method.

[fearful-symmetry]

Okay, finally figured out how this works with light modules.

[fearful-symmetry]

Alright. Now we're properly grabbing modules from Secondary sources. I tested it with SetSecondarySources() and it works.

[jsoriano]

It looks good to me, I have added some minor comments.
My main doubt is if we want to do this. I like it, but I am not sure if there was a reason in the past to don't do it.
I'd like to see more opinions about this before merging.

[fearful-symmetry]

jenkins, test this

[fearful-symmetry]

Yah @jsoriano, as I mentioned in the initial PR, this is mostly a way to get #12648 to work, so we can use mage to get a list of all known modules & default metricsets.

[fearful-symmetry]

Looks like there's an (unrelated) filebeat test that's still failing.

[andrewkroh]

I'm pretty sure this isn't what you want. Error a field type that can be used with structured logging to optionally specify type information.

I think you want either logp.L().Error(errors.Wrap(...)) or logp.L().Errorw("Failure getting module list from secondary source", "error", err) or logp.L().Errorf("Kaboom! error: %v", err).

[fearful-symmetry]

Bah, that code was all over registry.go so I just copied it. Thinking I should make a separate PR to fix it that can be backported.

[jsoriano]

Not sure why I ended-up doing this, I was probably looking for something like logp.Err 😬

[fearful-symmetry]

jenkins, test this

[fearful-symmetry]

Alright, now we're checking for dupes in in secondary sources. Not sure how much of the checks are necessary, but the whole module registration thing seems open enough that it can't hurt to be careful.

[fearful-symmetry]

Alright, took @jsoriano 's advice, since it trims down a few lines of code.

[fearful-symmetry]

jenkins, test this

[andrewkroh]

LGTM.