fix(buffers): buffer_id handling in buffer usage metrics reporting

[vparfonov]

Summary

This change improves how the buffer_id is used in buffer usage metrics. It ensures the buffer_id is properly owned and has the right lifetime to be safely included as a label in emitted metrics.

Change Type

• Bug fix
• New feature
• Non-functional (chore, refactoring, docs)
• Performance

Is this a breaking change?

• Yes
• No

Does this PR include user facing changes?

• Yes. Please add a changelog fragment based on our guidelines.
• No. A maintainer will apply the no-changelog label to this PR.

References

• Related: fix(buffers): Prevent negative buffer size and event gauges #23453

[bruceg]

I've dug into the counter data usage here, and I'm wondering if the whole BUFFER_COUNTERS data couldn't be moved into the async reporting loop in BufferUsage::install, where it would not need any index on the buffer_id. It might not even need atomics, since there would not be conflicting usage.

Thinking further, this would suggest those values could be lifted into struct CategoryMetrics, with the update mechanism integrated into the consume method there.

What do you think?

[vparfonov]

I've dug into the counter data usage here, and I'm wondering if the whole BUFFER_COUNTERS data couldn't be moved into the async reporting loop in BufferUsage::install, where it would not need any index on the buffer_id. It might not even need atomics, since there would not be conflicting usage.

Thinking further, this would suggest those values could be lifted into struct CategoryMetrics, with the update mechanism integrated into the consume method there.

What do you think?

Maybe it makes sense — the main win would be removing AtomicI64.
But in that case, metrics would only update every 2 seconds in the reporting loop, instead of on each event.
Not sure if that trade‑off is worth it.

[bruceg]

Maybe it makes sense — the main win would be removing AtomicI64.

Actually, the main I was thinking of is removing the global Dashmap, which removes a layer of locking and hashing.

But in that case, metrics would only update every 2 seconds in the reporting loop, instead of on each event. Not sure if that trade‑off is worth it.

If I'm not mistaken, the events that update the counters are only emitted from the reporting loop, and so such a move does not change the behavior. Are the counters updated elsewhere?

[vparfonov]

Maybe it makes sense — the main win would be removing AtomicI64.

Actually, the main I was thinking of is removing the global Dashmap, which removes a layer of locking and hashing.

But in that case, metrics would only update every 2 seconds in the reporting loop, instead of on each event. Not sure if that trade‑off is worth it.

If I'm not mistaken, the events that update the counters are only emitted from the reporting loop, and so such a move does not change the behavior. Are the counters updated elsewhere?

i mean this one:

vector/lib/vector-buffers/src/buffer_usage_data.rs

Line 240 in 7b8445d

/// Metrics are reported every 2 seconds.

[bruceg]

Right. I was saying that that loop is the only place that the metrics I am referring to are emitted. That is, it appears they are currently only emitted every two seconds, and I didn't see anything in the changes here or previously that alter that behavior.

[vparfonov]

Right. I was saying that that loop is the only place that the metrics I am referring to are emitted. That is, it appears they are currently only emitted every two seconds, and I didn't see anything in the changes here or previously that alter that behavior.

Ok, let me review the code again to be sure. From what I understand, since metrics are only emitted every 2 seconds (regardless of when the underlying counts change), we could move the counters inside the loop. I need some time to figured out how to avoid using HashMap. It does sound reasonable and should make code better
I’ll think more about how to wire that up — thanks a lot for clarifying, I really appreciate the explanation.

[bruceg]

To be clear, I think this PR addresses a real deficiency in the solution to #21702 that was merged in #23453, and as such it should possibly be merged even with the above deficiencies. I just think there is a much cleaner solution hiding in the combination of these two PRs.

[vparfonov]

To be clear, I think this PR addresses a real deficiency in the solution to #21702 that was merged in #23453, and as such it should possibly be merged even with the above deficiencies. I just think there is a much cleaner solution hiding in the combination of these two PRs.

@bruceg
Yes, I agree — let’s move forward with this. As a next step, your proposal sounds reasonable. Since it involves more significant changes, I’ll open a draft PR with my proposal so we can continue the discussion there.

Please take a look #23518

[pront]

Per #23507 (comment) - approving this to make sure we have the fix in before the next release. I do agree this PR #23518 addresses the issue in a better/cleaner way. We can obviously review that PR independently later.