This message is based on what has been described in the RFC - and has been mutually discussed and approved by Vincent and@DanielleMaywood

self review : We have this intermediate function in order to have an extra layer, and simplicify for testing purpose.
As all arguments are injected as parameters toPushResourcesMonitoringWithConfig we can use the mocked versions.

self review : There's three situations on the which we do not want to run the go-routine :

• If we can not fetch the configuration from Coderd.
• If the resources_monitoring is not enabled.
• If we can not instantiate the clistat client (which is the one used to fetch resources usage.)

self review : This function is just here as a placeholder - we are fine keeping it this way for now as it will be replaced by@DanielleMaywood PR.

self review : We prefer to log instead of returning an error in case of problem.

UsingTickerFunc from quartz, returning an error would just shutdown the whole ticker and stop it. Returning nil stops the current tick, but the next one will still trigger.

Looking at the code coverage, we're missing a few test-cases here:

• disabled config pushes nothing
• errors when fetching memory/volume info
• errors when pushing to coderd

I improved a lot the testability of this block by creating a struct for the resources_fetcher part - so we can mock it.

Also added test cases for all the missing cases. Should be way better now.

This implementation does not seem to match the RFC design.

Once the buffer fills up, it'll send the full buffer to the control plane. However, it'll keep doing so everyTickInterval thereafter since the buffer only ever replaces the oldest value.

The RFC specifies that the payload should only be delivered every 15s for the 20 datapoints; it's currently performing collection and submission in the same ticker with no distinction between them.

We also seem to not be adding theUNKNOWN datapoints when values cannot be retrieved.

Was this a conscious choice? Is the RFC out-of-date perhaps?

About theUNKNOWN status - this is handled directly on the coderd side.
It will be visible in the part currently being done by Danielle - from the configuration stored in the DB, and what we return on the Agent side, coderd (and the processing component) decide if the resource, for the given datapoint, isOK,NOK orUNKNOWN.

About the tick logic - that's maybe a misunderstanding in the RFC , but when I defined it to be delivered every 15s, it was based on theTickInterval - considering it was set to 15. having both theTickInterval and another value set independently would require some sync to avoid sending two times the datapoints without having a refresh or fetching datapoints too frequently and so not pushing data to coderd at each tick.

TL;DR - I think the best solution is to have theTickInterval handling both the refresh of resources and push to coderd so we are sure each push will always have the last data.

About the UNKNOWN status - this is handled directly on the coderd side.
It will be visible in the part currently being done by Danielle - from the configuration stored in the DB, and what we return on the Agent side, coderd (and the processing component) decide if the resource, for the given datapoint, is OK, NOK or UNKNOWN.

Can you point me to some specific code I can look at please? Will you be inferring that a datapoint was missed by looking at the time of each datapoint? I wonder why we opted for this approach rather than explicitly sending a noop datapoint to indicate a problem.

TL;DR - I think the best solution is to have the TickInterval handling both the refresh of resources and push to coderd so we are sure each push will always have the last data.

This doesn't really address what I'm asking, though.

We're going to be sending alln datapoints everyTickInterval once the queue fills up since we never drain the queue but only shifting off the oldest element.

This should be an interface. With a bare function type, IDE tooling has a hard time finding implementations of this function. If you make it an interface, you can find implementations and see what this is.

I removed thefunc parts andResourcesMonitorConfigurationFetcher to have an interface - should be way better this way. 👀

Instead of passing a "fetcher", you should just pass the config, and do the fetching before you call this function. It will make testing easier and you can avoid definingResourcesMonitorConfigurationFetcher which is a real mouthful.

✅ changed too.

I think this would be a lot more readable with aResourceMonitor struct that includes things like the resources fetcher and queue as members, instead of this nested function style. It makes it hard to keep track of the scope of things, that is, which things are per tick and which last between ticks.

You set up the long lived struct, then start the ticker calling a method on the struct. That way it's very clear what is part of the tick and what outlives it. It also means you don't have to pass thefetcher to helpers likefetchResourceMonitoredVolume.

I changed a lot the way things were organised based on the comments - so now :

• Resources monitor logic is in its own package
• It works with a struct and methods to make it an entity
• members are used to store the queue & config

🙏

I think Go style would be to move this into it's own package so you don't have to include some variant of "resource" and "monitor" in the name of everything to keep things clear.

I moved it to its own package ✅ should indeed simplify a lot the naming.

nit:

I think we could avoid using theAgentFn pattern here. We only need the Agent's ID, but this function makes a call to the database, using the Agent's ID, to fetch the agent. We could instead have theResourcesMonitoringAPI have anAgentID field and just pass in theAgentID fromOptions.

A bit surprised there's a so simple option , but makes sense and changed - Thanks ! ✅

Nit: since these all have the same config, consider creating a const to improve readability.
Alternatively (I'd prefer this) you should change these configs up so that you avoid accidentally overfitting for this specific config.

Nit: I don't think you need this here since you have adefer cancel() already which will be invoked after this.

Nit: this feels awkward; the assertions should be happening in the test body, not the test case definitions.
Don't we always want to check that the returned values match expectations?