If I'm understanding this correctly - say you have two agent metadata definitions, one which updates every 1 second, the other which updates every 10 seconds.

In the previous request-per-metadata-key approach, this would cause (0.1 + 1) 1.1 requests per second, while in this new approach we would end up with a minimum of 1 requests per second, as the more frequently updated metadatum would cause a batch request.

I'm think we should update the documentation with this change to reflect the new behaviour. I think it would also make sense to recommend users to keep in mind the minimum metadata refresh interval when writing their templates; any metadatum with a refresh interval of 1 second will cause frequent metadata updates from what I understand here.

In the previous request-per-metadata-key approach, this would cause (0.1 + 1) 1.1 requests per second, while in this new approach we would end up with a minimum of 1 requests per second.

I'm not sure where 0.1 comes from in your example, but for simplicity sake, let's say we have 1s and 2s intervals for the metadata, and each take 0ns to execute. In the previous implementation we would approximately do:

14:00:00 POST meta1
14:00:00 POST meta2
14:00:01 POST meta1
14:00:02 POST meta1
14:00:02 POST meta2
14:00:03 POST meta1

In the new implementation, we would:

14:00:00 POST meta1, meta2
14:00:01 POST meta1
14:00:02 POST meta1, meta2
14:00:03 POST meta1

With 2s and 3s metadata, it would look like this:

Old:

14:00:00 POST meta1
14:00:00 POST meta2
14:00:02 POST meta1
14:00:03 POST meta2
14:00:04 POST meta1
14:00:06 POST meta1
14:00:06 POST meta2

New:

14:00:00 POST meta1, meta2
14:00:02 POST meta1
14:00:03 POST meta2
14:00:04 POST meta1
14:00:06 POST meta1, meta2

This is an approximation of ideal conditions, though. And perhaps we should separate collect and send triggers by, say, 500ms. This would increase the likelyhood of ideal batching.

With regards to RPS the new implementation reduces RPS and doesn't necessarily guarantee 1 RPS, it depends on interval and how long commands take to execute.

I'm think we should update the documentation with this change to reflect the new behaviour.

Are you referring to this https://coder.com/docs/v2/latest/templates/agent-metadata#db-write-load?

The write load will be about the same, but the writes will be more performant since they're batched. I suppose we could call one batch one write, even though we're updating multiple rows. I'll amend this part.

Wouldn't an agent metadatum with a refresh interval of 10 seconds cause 1 request roughly every 10 seconds (i.e. 0.1 RPS)?

Yes, this is the case in both implementations.

We don't need this condition anymore? just in case...

Previously there was a buffered channel, but that's no longer required so this artificial restriction got lifted. If we want to limit this, it should be in the coder tf provider, saying too many metadata items were added.

This behavior doesn't change, right:

If the agent fails to send the metadata, it will be lost. There is no "retry" mechanism in place now?

Correct, we don't retry since trying to send stale data wouldn't make much sense, we instead wait until the next update is available and try to send it.

Ok! I guess we can debate about the lack of source data for insights, but that's a separate issue.

Ah, this is not used for insights, so it will not have an effect on that.

There are many magic timeouts hidden in this file. I'm wondering if we shouldn't move them to the top of file, and make them more dependent. For instance: instead of 5sec -> reportTimeout * 1/6

With 5 sec, are you referring to the case where md.Timeout is not set (or zero), and we fall back to 5 sec?

I wanted to change as little about the metadata collection as possible, but I can go over this and see what can be consolidated.

This 30 second timeout was randomly picked by me as a crutch for scenarios where the network is super slow or down. It should be enough for at least something to get through, whilst avoiding sending very stale data.

I see. I'm afraid that there are too many timeouts depending on each other in the entire implementation, and it might be hard to debug potential queueing problems.

Would it change something if we increase buffers here?

Yes. They are single buffer to be non-blocking and not cause back-pressure. If we increase channel sizes then we may have back-pressure if an operation is taking longer than expected. Say collect was 10, if one iteration of starting collection took 10s then this channel would fill and afterwards 10 collections would start immediately after each other, even if they now finish in 0.1s each, possibly leading to 10 collections (and load) within a second (vs once per second).

I might be lost in the flow here... why is this 1sec timeout needed?

This also didn't change in this PR.

The timeout here can be 1s or it can be higher. I suppose the logic is that if you want metadata to update every 1s, collection should run faster than 1s, but this is likely not ideal.

We could change this in the future or if it becomes a problem.