review: Would this be better set at a lower interval, or made configurable?

First of all, 29 sec looks magical :) You may need to comment on why is it exactly this.

I didn't look deeper into the review yet, but you may want to introduce 2 limits: timeout and buffer size are getting full.

Made the interval 1 second instead by default.

review: this was bugged before.

Good finding 👍 Will it be covered with tests now?

Yes, the tests added here were the impetus for this change!

review: connections_by_proto needs to be handled as jsonb[] and not jsonb[] as that ends up being passed to pq.Array which turns it into something like {{123,34,106,34,...125}}.

:)

jsonb[] and not jsonb[]

🙃

jsonb and not jsonb[]

:)

jsonb[] and not jsonb[]

Good finding 👍 Will it be covered with tests now?

Is there a reason why New(options) can't define a default instance?

No, just keeping to the existing pattern in this file.

In a typical environment, how many items are in the buffer?

Assuming 1000 workspaces, the worst-case scenario is that they all attempt to push stats at once. That's extremely unlikely however.

nit: do you think that we should flush before the buffer is full? Let's say 80% of occupancy?

What would the advantage of this be? If you're trying to avoid blocking during flush, blocking will happen regardless of when we flush. We could make a temporary copy of the buffer while we flush to avoid that, but batch-inserting 1000 entries happens fairly quickly from what I can tell.

We had a conversation about this.

We came up with passing the existing buffer to the lever and resetting the buffer. So that way the buffer is new for the next Add and the previous data is pending the db query.

Psuedo code:

	if len(b.buf.ID) == cap(b.buf.ID) {
		select{
		case b.flushLever <- struct{}{}:
		default:
			b.log.Error(context.Background(), "this should never happen, dropping agent stats :(")
		}
		b.flushLever <- struct{}{}
		b.flushLever <- &b.buf
		b.buf = make(database.InsertWorkspaceAgentStatsParams, 0, b.batchSize)
	}

Some additional nits:

• We should probably use the constant instead of cap for comparison since cap can grow. len(b.buf.ID) >= b.batchSize
• We should use a select and throw an error log if we are backing up. We should drop the stats, as if we are backing up, there is nothing we can do except log it anyway.

It ended up being simpler to do an early flush. However, I thought about this some more and I think using a buffered channel would also resolve this issue. This is a possible later enhancement.

What happens if one "stat" in the batch is faulty? Is the whole batch rejected?

Bonus points: maybe we need a test for this.

Yes, the whole batch will be rejected. That's a good idea for a test.

It would be nice to exclude the faulty item, and keep the rest of the batch, but I guess that it would be tricky to implement, or just fall back to iterative approach.

We had a conversation about this.

We came up with passing the existing buffer to the lever and resetting the buffer. So that way the buffer is new for the next Add and the previous data is pending the db query.

Psuedo code:

	if len(b.buf.ID) == cap(b.buf.ID) {
		select{
		case b.flushLever <- struct{}{}:
		default:
			b.log.Error(context.Background(), "this should never happen, dropping agent stats :(")
		}
		b.flushLever <- struct{}{}
		b.flushLever <- &b.buf
		b.buf = make(database.InsertWorkspaceAgentStatsParams, 0, b.batchSize)
	}

Some additional nits:

• We should probably use the constant instead of cap for comparison since cap can grow. len(b.buf.ID) >= b.batchSize
• We should use a select and throw an error log if we are backing up. We should drop the stats, as if we are backing up, there is nothing we can do except log it anyway.

Does the flushForced just prevent a deadlock by adding to the flushLever channel if it already has something?

Can we just use a select statement to prevent pushing to the flushLever channel if it already has an item?

I tried the select but it will still end up doing one extra unwanted flush.

Well, in theory an extra flush is lesser evil than losing data.