fix: Send correct batch stats when SendBatchMaxSize is set #5385
Conversation
|
|
| @@ -244,17 +245,24 @@ func (bt *batchTraces) add(item interface{}) { | |||
| td.ResourceSpans().MoveAndAppendTo(bt.traceData.ResourceSpans()) | |||
| } | |||
|
|
|||
| func (bt *batchTraces) export(ctx context.Context, sendBatchMaxSize int) error { | |||
| func (bt *batchTraces) export(ctx context.Context, sendBatchMaxSize int, returnBytes bool) (int, int, error) { | |||
There was a problem hiding this comment.
question: should we do the stats.Record call within export rather than returning it? That way we don't need to make any function signature changes?
There was a problem hiding this comment.
i can see why that would be annoying because then instead of a single stats record call, you have to make one in each batch processor. What do you think?
There was a problem hiding this comment.
Yeah, I'd rather change the signature, especially since it's only used in one place, than duplicate the stats recording code.
| @@ -244,17 +245,24 @@ func (bt *batchTraces) add(item interface{}) { | |||
| td.ResourceSpans().MoveAndAppendTo(bt.traceData.ResourceSpans()) | |||
| } | |||
|
|
|||
| func (bt *batchTraces) export(ctx context.Context, sendBatchMaxSize int) error { | |||
| func (bt *batchTraces) export(ctx context.Context, sendBatchMaxSize int, returnBytes bool) (int, int, error) { | |||
There was a problem hiding this comment.
i can see why that would be annoying because then instead of a single stats record call, you have to make one in each batch processor. What do you think?
| } | ||
|
|
||
| if err := bp.batch.export(bp.exportCtx, bp.sendBatchMaxSize); err != nil { | ||
| detailed := bp.telemetryLevel == configtelemetry.LevelDetailed |
There was a problem hiding this comment.
question: should we call out that we may want to remove this given it's barely used and almost always desired in a future PR?
There was a problem hiding this comment.
It looks like the default for this setting is LevelBasic, so eliminating the check here would be a behavior change.
Codecov Report
@@ Coverage Diff @@
## main #5385 +/- ##
==========================================
- Coverage 90.89% 90.88% -0.01%
==========================================
Files 191 190 -1
Lines 11421 11446 +25
==========================================
+ Hits 10381 10403 +22
- Misses 819 822 +3
Partials 221 221
Continue to review full report at Codecov.
|
|
Thanks for having a look, @codeboten!
|
The stat was getting sent before the max batch size was taken into account.
Description:
This fixes a bug with the batch processor's
batch_send_sizeandbatch_send_size_bytesmetrics. Their values were being calculated beforeSendBatchMaxSizewas applied.We observed this issue during performance testing. With
SendBatchMaxSizeset to a small enough value that it almost always took effect, graphs ofbatch_send_sizeshowed an odd sawtooth pattern and the total values were far in excess of the number of items actually sent. Looking at some individual metrics the issue was clear - with aSendBatchMaxSizeof 100, for instance, we'd seebatch_send_sizemetrics looking like 1000, 900, 800, 700... as the full size of the batch queue was recorded but not sent each time.With this change the various
exportmethods report the actual count of items sent, and byte size sent if requested, and thesendItemsmethod records those values.Testing:
I added a test called
TestBatchProcessorSentBySize_withMaxSize, based onTestBatchProcessorSentBySizebut withSendBatchMaxSizeset and with all spans delivered in a single request so that the batch size is predictable. It does not attempt to validate thebatch_send_size_bytesmetric - the splitting of batches makes the method used in the original test fail due to different amounts of overhead.(Incidentally,
TestBatchProcessorSentBySizeitself is rather brittle. Changing eithersendBatchSizeorspansPerRequestso that the former is not a multiple of the latter makes the test fail in several ways.)