fix: Rename throughput fields from mbps to mb_s for correctness#113
Merged
Conversation
The throughput fields were named average_throughput_mbps and peak_throughput_mbps, which conventionally means "megabits per second." However, the calculation divides bytes_per_second by 1,000,000, producing megabytes per second (MB/s). This rename corrects the field names to match the actual unit. Changes: - Rename average_throughput_mbps → average_throughput_mb_s - Rename peak_throughput_mbps → peak_throughput_mb_s - Update doc comments from "megabits" to "megabytes per second (MB/s)" - Remove workaround comments that acknowledged the mismatch - Update JSON example in README.md - All tests passing, clippy clean Fixes: #112 BREAKING CHANGE: Serialized JSON field names changed from *_mbps to *_mb_s. Downstream consumers must update accordingly. AI-assisted-by: Claude Opus 4 (Cursor agent) Made-with: Cursor
📈 Changed lines coverage: 70.00% (7/10)🚨 Uncovered lines in this PR
📊 Code Coverage Summary
|
dustinblack
reviewed
Apr 17, 2026
dustinblack
left a comment
dustinblack
left a comment
Collaborator
There was a problem hiding this comment.
The problem is clear — the fields say "mbps" but the calculation produces megabytes, not megabits. The workaround comments in the code confirm this has been a known issue.
However, I'm not sure mb_s obviously solves the ambiguity. In a snake_case field name, the mb vs MB distinction (lowercase = megabits, uppercase = megabytes per SI) is lost, so average_throughput_mb_s could still be read either way.
Can we consider alternatives?
average_throughput_megabytes_per_sec— verbose but unambiguousaverage_throughput_bytes_per_sec— report raw bytes, let consumers convert (most precise, no unit confusion possible)- Keep the current field names but fix the doc comments and document the unit as MB/s in the JSON schema
Any of these would more clearly resolve the ambiguity. What do you think?
Rename average_throughput_mb_s → average_throughput_megabytes_per_sec and peak_throughput_mb_s → peak_throughput_megabytes_per_sec for unambiguous field naming in serialized output. AI-assisted-by: Claude Opus 4 (Cursor agent) Made-with: Cursor
📈 Changed lines coverage: 64.29% (9/14)🚨 Uncovered lines in this PR
📊 Code Coverage Summary
|
Collaborator
Author
|
I agree. It's changed to: |
dustinblack
approved these changes
Apr 20, 2026
dustinblack
left a comment
dustinblack
left a comment
Collaborator
There was a problem hiding this comment.
Verbose field names are unambiguous. Clean rename, CI green. Approved.
sberg-rh
approved these changes
Apr 20, 2026
sberg-rh
left a comment
sberg-rh
left a comment
Contributor
There was a problem hiding this comment.
Looks good, no issues here.
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
3 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.
Summary
average_throughput_mbps→average_throughput_mb_sandpeak_throughput_mbps→peak_throughput_mb_sacross structs,serialization, and display logic
"megabytes per second (MB/s)" to match the actual calculation
BREAKING CHANGE: Serialized JSON field names changed from
*_mbpsto*_mb_s. Downstream consumers must update accordingly.Fixes #112
Test plan
cargo checkcompiles cleanlycargo test— all 290 tests passcargo clippy --all-targets -- -D warnings— no warningscargo fmt— no formatting issuesmbpsormegabitreferences inchanged files
Made with Cursor