fix(instrument): Buffer counter underflowed (#23872) #23973
Conversation
|
Hi @sialais, thank you for the PR! Please add a changelog explaining the change. |
sialais
force-pushed
the
bugfix/fix_buffer_counter_underflowed
branch
from
fcc55dd to
b222f23
Compare
| true, | ||
| ); | ||
| } | ||
| was_dropped = true; |
There was a problem hiding this comment.
Not directly related to your change, but should this be setting sent_to_base = false; as well here?
There was a problem hiding this comment.
It is not necessory to keep two flags send_to_base, was_dropped. The event will either sent_to_base or dropped.
I've updated my commit, and sent_to_base is removed.
There was a problem hiding this comment.
Hey @sialais, thank you for your work on this. It would be best if you don't force push to this branch to make incremental reviews easier. Thanks!
There was a problem hiding this comment.
Hey @sialais, thank you for your work on this. It would be best if you don't force push to this branch to make incremental reviews easier. Thanks!
Gotted!
| } | ||
| } | ||
| WhenFull::Overflow => { | ||
| if let Some(item) = self.base.try_send(item).await? { |
There was a problem hiding this comment.
I'm not totally familiar with the implementation here, so pardon if this is a naive question. The other two sends call increment_received_event_count_and_byte_size before sending, this one never calls it. Is that an omission?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry for that, My previous commit did indeed contain an error. I've never used Overflow before, and after my build is tested, I've forgot the last case of WhenFull::Overflow, apologize.
Because resolving this issue requires updating the counter before sending the event to the buffer, when handling the Overflow branch—if the overflow logic is triggered and the message is sent to overflow buffer—the counter needs to be decremented. However, neither using send nor drop to perform this countdown is quite appropriate. Therefore, to address the problem I was facing, I chose to ignore this particular scenario for temporarily.
Now that I think about it again, maybe using drop to update the counter is actually reasonable—since the event is effectively "dropped" into the overflow buffer?
I've updated my commit, but not tested.
sialais
force-pushed
the
bugfix/fix_buffer_counter_underflowed
branch
from
b222f23 to
34da1d1
Compare
sialais
force-pushed
the
bugfix/fix_buffer_counter_underflowed
branch
from
34da1d1 to
dedcf6a
Compare
3 participants
Summary
Fix issue #23872
How did you test this PR?
Because the issue is hard to reproduce, (I can only reproduce it in my production environment, with huge data), I put my build on my production environment, and the issue fixed.
This bug will make the self monitoring metrics incorrect: when buffer counter undeflowed, metrics will be reset.
Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.References