Better handling of goroutines

[dmathieu]

Right now, handling of goroutines in assets_aws is non-existent. We run every collector in a separate goroutine, but don't keep track of them.
This can be problematic, as if one of them starts taking longer than the ticks, we could start accruing their number.
We also can't report errors back to the main thread.

I have 2 proposals to better handle those goroutines:

POGC (Plain Old Golang Channels)

There's a ruby pun here 😁

We could let each collect method return an error, and setup a channel to track them.
A waitgroup would also be used to ensure every async method finished running before we start the next tick.

wg.Add(1)
go func() {
    defer wg.Done()

    err := collectEKSAssets(ctx, cfg, log, publisher)
    if err != nil {
        errCh <- err
    }
}()

Then, the main thread checks if the length of errCh is 0. If it isn't, it can process the errors.

oklog/run

We could use oklog/run, to make this easier for us, and not have to check for channels.

var g run.Group

ctx, cancel := context.WithCancel(context.Background())
g.Add(func() error {
	return collectEKSAssets(ctx, cfg, log, publisher)
}, func(error) {
	cancel()
})

[tommyers-elastic]

in favour of improving this for sure.

if you look in the aws code in metricbeat you'll see some more sophisticated worker patterns, but we should not just blindly do what has been done before. i'm all for using a 3rd party lib that makes this simpler.

for now we just wanted to get this off the ground so i didn't think too hard about it. i think it would be beneficial to have some integrations tests set up before starting this work though. what do you think?

[dmathieu]

Adding integration tests first makes sense to me.

[tommyers-elastic]

regarding data collection taking longer than the tick period.

this does happen in metricbeat for large environments and small periods. question is though - should we kick off the next collection anyway, so in the end we still report an event once-per-period? ideally, yes, but in practice we would just keep adding more and more threads as you previously mentioned. for asset collection we really shouldn't get into this position though. collection periods will likely be hours, not minutes. regardless, handling it thoughtfully is really important and we should ensure there is good logging for user visibility into what is happening.

[dmathieu]

Another thing is that right now, if the runner is canceled, the goroutines are not stopped.