Fix swapped TX/RX byte counters (TxBytes/RxBytes mislabeled) - #112
Merged
Conversation
AddEntry takes (..., rx, tx), but the single call site passed (txBytesCount, rxBytesCount), so sent bytes were stored as rx and received bytes as tx. Downstream, TxBytes summed Tx() and RxBytes summed Rx(), leaving both metrics mirror-labeled. Because replies are discarded (rcv is a nil interface, getRxLen returns 0), the net effect was that TxBytes always reported 0 and RxBytes reported the sent-byte total. Swap the two arguments at the call site so received bytes land in Rx() and sent bytes in Rx()'s counterpart Tx(), and correct the duplicated struct comment. Tests: - benchmark_runner: AddEntry/NewCmdStat map (rx, tx) into Rx()/Tx() in order. - cmd/ftsb_redisearch: sendFlatCmd records the sent-byte count as Tx() (via a fake radix.Client); verified this test fails on the pre-fix argument order. - getRxLen sizing for string/[]string/nil/other. Closes #111
Add fake-radix.Client tests for the error and i/o-timeout branches of sendIfRequired, asserting the sent-byte count is still recorded in Tx() and the error/timeout flags are set. Raises sendIfRequired coverage 58.8% -> 88.2%.
Review-driven hardening (no behavior change): - Add TestGetTotalsMapMapsTxRxCorrectly: guards the GetTotalsMap aggregation labels (txTotalBytes -> "TxBytes", rxTotalBytes -> "RxBytes") -- the JSON the issue #111 symptom appeared in, previously untested. - TestSendFlatCmdRecordsSentBytesAsTx now sets pipeline=1 explicitly (with save/restore) so a stray package global can't make the channel receive block. - TestSendFlatCmdMarksTimeout now also asserts Tx() so it guards byte accounting on the timeout path, not just the timeout flag. - Document that bytelen is an application-payload proxy (excludes RESP framing), accurate for large payloads, approximate for many-tiny-arg commands.
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #111.
Problem
AddEntrydeclares its last two params as(rx, tx)(benchmark_runner/stat.go), but the single call site incmd/ftsb_redisearch/cmd_processor.gopassed them as(txBytesCount, rxBytesCount)— reversed. So the bytes sent to Redis were stored in therxfield and the reply bytes in thetxfield. Downstream,txTotalBytessumsTx()andrxTotalBytessumsRx(), so the reportedTxBytes/RxBytes(and their rate metrics) were mirror-labeled.Because replies are currently discarded (the receiver
rcvis a nil interface, sogetRxLenreturns 0), the practical effect was:TxBytes(andoverallTxByteRate) always reported 0.RxBytes(andoverallRxByteRate) reported the sent-byte total.Ops/sec and latency were unaffected (they derive from command count and timing).
Fix
Rx()and sent bytes inTx().// bytes receivedstruct comment on thetxfield.Tests
benchmark_runner/stat_test.go:AddEntryandNewCmdStatmap(rx, tx)intoRx()/Tx()in order.cmd/ftsb_redisearch/send_stats_test.go:sendFlatCmdrecords the sent-byte count asTx()(exercised with a fakeradix.Client, no Redis needed). Verified this test fails on the pre-fix argument order (Tx() = 0, want 4096) and passes after the fix.getRxLensizing forstring/[]string/nil/ other.Both suites are wired into CI (
make integration-testcoversbenchmark_runner;make unit-testcoverscmd/...).gofmt/go vet/go test -raceall clean.Note (out of scope)
Redis replies are not currently captured (
rcvis nil), soRxBytesreads 0 after this change (an honest 0, vs. the pre-fix value which put SENT bytes under the RX label). MakingRxBytesmeaningful is a multi-part change (pointer receiver +repliesplumbing + extendinggetRxLento the types radix returns) and is tracked, along with a--pipeline >1panic and pipelined per-command byte miscounting, in #113. This fix is strictly the mislabeling on the workingpipeline=1path.