exported type InvalidMonitorProcessorsError should have comment or be unexported

exported method Registry.AddActive should have comment or be unexported

exported method Registry.GetFactory should have comment or be unexported

exported method Registry.Query should have comment or be unexported

exported method Registry.Register should have comment or be unexported

exported function NewMonitor should have comment or be unexported

exported type Monitor should have comment or be unexported

exported type JobFactory should have comment or be unexported

exported type JobRunner should have comment or be unexported

exported type Job should have comment or be unexported

exported method ReloaderFactory.Create should have comment or be unexported

exported function NewFactory should have comment or be unexported

exported type ReloaderFactory should have comment or be unexported

exported type MonitorsConfig should have comment or be unexported

exported type MonitorTaskConfig should have comment or be unexported

exported method Heartbeat.Monitors should have comment or be unexported

exported func NewFactory returns unexported type *monitors.factory, which can be annoying to use

exported method Factory.Create should have comment or be unexported

exported type MonitorPluginAlreadyExistsError should have comment or be unexported

exported const ActiveMonitor should have comment (or a comment on this block) or be unexported

exported type Type should have comment or be unexported

Could you update the heartbeat.reference.yml file? This should make it more obvious how the config will look like.

For sure

Done!

Is this file intended? doesn't seem related to the PR

no and yes. It won't go in the final one, but it's useful to keep around for manual testing for now when I switch branches.

exported function New should have comment or be unexported

exported function New should have comment or be unexported

comment on exported method Heartbeat.RunDynamicMonitors should be of the form "RunDynamicMonitors ..."

Nit but in case I miss it later: We always use .yml (history ...)

TestNewPluginsReg, I prefer not to have _ in function names.

I discussed this with @urso in a previous PR. Our conclusion was that the linter doesn't complain, so this is OK. The go project itself added support for underscores since they were a requirement for doc_test.go files. In other words, they explicitly added support in the linter for underscores, but only for test files.

These names (and parts of the test bodies), were generated with https://github.com/cweill/gotests . I'm sure underscores were a consideration in the creation of that library as well.

Good to know. Didn't know about it.

see above

consider to return the monitorReloader or pass the monitorReloader.

E.g. in beater Run:

if bt.config.ConfigMonitors.Enabled() {
  reloader := cfgfile.NewReloader(...)
  err := startReloader(reloader, ...)
  if err != nil { ... }

  defer reloader.Stop() // defer has function scope
}

with

func startReloader(reloader, factory) error {
  if err := reloader.Check(factory); err != nil {
     return err
  }
  go reloader.Run(factory)
  return nil
}

I'm a little unclear as to the why here? Is the goal to decouple the monitor creating logic from RunDynamicMonitors?

It's more about symmetry. The function that instantiates some object which needs cleanup (Close/Stop/Cleanup...) should also clean it up (more akin to RAII). Returning some cleanup function is an edge-case that should be used sparily (e.g. teardown in test code is a common pattern). This way we end up with the common create-defer close pattern:

x := New...
defer x.Close()

returning a stop function to defer on from a method named 'RunX' just looks somewhat unexpected. ;)

Ah, thanks for the heads up on that, makes sense, will change it

It's more like an 'invalid config' error. Unpacking/parsing the config into mtConf failed due to mtConf and the configuration object passed being incompatible.

Stuttering. The full type name is monitors.MonitorPluginAlreadyExistsError. Renaming it to monitors.PluginAlreadyExistsError will contain the same information.

Let's not export this symbol.

exported var DefaultWatchConfig should have comment or be unexported

Is this error check still needed now that the newMonitorManager code is gone?

Good catch

I assume thats for testing purpose

yes, exactly

Metricbeat?

oops, I think I did some copy/paste at one point.

This will probably get overwritten when you run make update. You have to modify the _meta/beat.reference.yml file

ah, thanks

This is only not required if a poll_file is specified?

Yes, this is a good point. I'll need to add in an error about that somewhere. I assume the annotations here aren't rich enough to encode that conditional?

OK, I thought I had broken this with my refactor, but currently you may not use the poll file exclusively, you must declare at least one regular host.

I'll preserve that behavior and delete this line.

Errr, revert it.

Imports fun

Nit: errors we return are normally starting lower case. I wonder why hound did not complain?

Maybe it doesn't understand errors.Wrap?

At any rate, will fix

Can this just be called task. Otherwise you repeat the package name. Same below for taskConfig and all the types / methods that contain monitor in the name.

Some comment clarifying that monitorTask is used to lift monitoring jobs into the scheduler might be helpful.

what does Tf stand for?

taskFunc. I think it should just be called jobCancelFn