Metricsets being created two times at the start of the beat

[Thiago_Ruiz]

Hello there!

I'm creating some new modules and metricsets and saw something odd, maybe a bug, or at least a possibility of an improvement

The possible problem: When we use the config "reload.enabled: false" (on the "metricbeat.config.modules" section, the default) all the metricset's New Methods are called two times.

What causes that, I think: "metricbeat/beater/metricbeat.go", in the Run method:

    if bt.config.ConfigModules.Enabled() {
        		moduleReloader := cfgfile.NewReloader(b.Publisher, bt.config.ConfigModules) 

        		if err := moduleReloader.Check(factory); err != nil { //me: this line ends up calling initMetricSets, that calls the New method
        			return err
        		}

        		go moduleReloader.Run(factory) //also me: this, too, ends up calling initMetricSets
        		wg.Add(1)
        		go func() {
        			defer wg.Done()
        			<-bt.done
        			moduleReloader.Stop()
        		}()
        	}

Sorry if I'm missing something, but is there a reason for the moduleReloader.Check realy create the metricset all the way through (metricbeat/mb/module/wrapper.go's NewWrapper calling mb.NewModule)? If we are just trying to anticipate a config error, wouldn't the reloader just do the same?

Best Regards

[ChrsMark (Chris Mark)]

Hi!

I think that here a check is performed initially so as to fail instantly if a config is not valid and then moduleReloader is getting started to run in the background. Maybe this approach makes it easier to report these kind of errors early on instead of having the background routine to report them.

Do you find any specific issue with this implementation? If so feel free to open a PR of Github issue proposing a change.

Thanks!

[Thiago_Ruiz]

HI, Mark

Thanks for the reply

The only issue that I see is that when you use the New method of the metricset to change something that should be changed just one time in the beat run, like my new metricset was doing, and think that you can undo on the Stop method. But this can easily be avoided.
(Maybe I should not do that on a new method, i dont know...)

I'll investigate this design further, just not for now