Fix rebatching when the id is in the current batch #46004
+177
−12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
csmarchbanks
force-pushed
the
fix-rebatching-from-current-batch
branch
from
080babe to
b1e7258
Compare
There is a bug in MoveToEarlierBatch where if the trace id is still in the current batch it will not be properly removed from the current batch causing a decision to be made for the trace twice. This does not break any functionality but will cause the trace dropped too early metric to be improperly incremented if a decision cache is used. Signed-off-by: Chris Marchbanks <[email protected]>
csmarchbanks
force-pushed
the
fix-rebatching-from-current-batch
branch
from
b1e7258 to
a4ca32c
Compare
csmarchbanks
commented
Feb 11, 2026
| // processed before the proposed batch it will do nothing. Returns the | ||
| // batch that the trace will now be a part of (which may stay the same). | ||
| MoveToEarlierBatch(id pcommon.TraceID, currentBatch, batchesFromNow uint64) uint64 | ||
| MoveToEarlierBatch(id pcommon.TraceID, traceCurrentBatch, batchesFromNow uint64) uint64 |
Contributor
Author
There was a problem hiding this comment.
This rename is to avoid confusion withcurrentBatch in the batcher struct.
atoulme
reviewed
Feb 11, 2026
| } | ||
|
|
||
| func (b *batcher) MoveToEarlierBatch(id pcommon.TraceID, currentBatch, batchesFromNow uint64) uint64 { | ||
| func (b *batcher) MoveToEarlierBatch(id pcommon.TraceID, traceCurrentBatch, batchesFromNow uint64) uint64 { |
Contributor
Author
There was a problem hiding this comment.
I wanted to differentiate between the batch the trace is currently in and b.currentBatch/currentBatchID.
Signed-off-by: Chris Marchbanks <[email protected]>
portertech
approved these changes
Feb 11, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There is a bug in
(*batcher).MoveToEarlierBatchwhere if the trace id is still in the current batch it will not be properly removed from the current batch causing a decision to be made for the trace twice. This does not break any functionality but will cause the trace dropped too early metric to be improperly incremented if a decision cache is used while also using the newdecision_wait_after_root_receivedoption.I also noticed that we aren't updating the batch id on
actualDataafter moving a trace so fixed that too. It shouldn't make too much of a difference as it shouldn't be possible to receive two root spans, but better to be safe.Testing
New tests added that reproduce the failure and cover 100% of code paths in
id_batcher.go. I have also deployed this to a couple of internal environments and verified that traces dropped too early metric is no longer incremented.