Skip to content

fix: Rename throughput fields from mbps to mb_s for correctness#113

Open
mcurrier2 wants to merge 2 commits intomainfrom
fix/throughput-field-naming
Open

fix: Rename throughput fields from mbps to mb_s for correctness#113
mcurrier2 wants to merge 2 commits intomainfrom
fix/throughput-field-naming

Conversation

@mcurrier2
Copy link
Copy Markdown
Collaborator

@mcurrier2 mcurrier2 commented Apr 17, 2026

Summary

  • Renames average_throughput_mbps → average_throughput_mb_s and
    peak_throughput_mbps → peak_throughput_mb_s across structs,
    serialization, and display logic
  • Corrects doc comments from "megabits per second" to
    "megabytes per second (MB/s)" to match the actual calculation
  • Removes workaround comments that acknowledged the mismatch
  • Updates the JSON example in README.md

BREAKING CHANGE: Serialized JSON field names changed from
*_mbps to *_mb_s. Downstream consumers must update accordingly.

Fixes #112

Test plan

  • cargo check compiles cleanly
  • cargo test — all 290 tests pass
  • cargo clippy --all-targets -- -D warnings — no warnings
  • cargo fmt — no formatting issues
  • Verified no remaining mbps or megabit references in
    changed files

Made with Cursor

@mcurrier2
fix: Rename throughput fields from mbps to mb_s for correctness
3176fd9 
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
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 17, 2026

📈 Changed lines coverage: 70.00% (7/10)

🚨 Uncovered lines in this PR

  • src/results.rs: 1136, 1174-1175

📊 Code Coverage Summary

File Line Coverage Uncovered Lines
src/benchmark.rs 83.64%
(506/605)		 75, 78, 89, 93, 102, 105, 107, 124, 422, 427-432, 439-444, 511-514, 619, 703, 709-711, 715-717, 737, 806-808, 813, 834, 839, 857, 963, 967-970, 972, 981-984, 986, 1062, 1093, 1096, 1098-1099, 1108-1109, 1251, 1264, 1281, 1404, 1413, 1415-1416, 1419-1420, 1426-1427, 1432-1433, 1435, 1440-1441, 1445-1447, 1452-1453, 1456-1457, 1489, 1547-1551, 1553-1559, 1561, 1564, 1721, 1736
src/benchmark_blocking.rs 73.50%
(319/434)		 97, 111, 127, 263, 369, 375-377, 380-382, 402, 434, 488, 587, 600, 614, 644-647, 732-735, 754, 758, 773, 815-817, 820, 823-825, 827, 830, 832-836, 838-839, 847-851, 853-857, 860-861, 865-866, 901, 950, 1029, 1040, 1070, 1073, 1138-1143, 1145, 1200-1203, 1208, 1221, 1224-1227, 1231, 1233-1236, 1238, 1240-1241, 1243-1244, 1247, 1249-1254, 1256, 1260-1261, 1263, 1265, 1289, 1301-1306, 1308, 1328-1331
src/cli.rs 92.39%
(85/92)		 630, 729, 769, 771, 792-794
src/execution_mode.rs 100.00%
(14/14)		 ``
src/ipc/mod.rs 65.28%
(47/72)		 115, 425, 427-430, 740-741, 756-757, 775-776, 807, 810, 813, 818, 845-846, 860, 862, 882, 884, 1007-1009
src/ipc/posix_message_queue.rs 46.09%
(59/128)		 139-140, 213-215, 217, 224, 229, 332-335, 337, 345, 437, 441-442, 446, 449-452, 454-458, 539, 679, 782, 789-790, 807-808, 819-820, 831-832, 849-850, 906, 910-911, 914-919, 921-923, 927, 929-931, 933, 935-937, 941-943, 945-947, 994-995, 1017
src/ipc/posix_message_queue_blocking.rs 81.94%
(127/155)		 172, 182, 221, 251-255, 274, 325, 368, 387-390, 416-418, 422-423, 425-426, 436, 455, 457-458, 460-461
src/ipc/shared_memory.rs 69.36%
(163/235)		 61, 141, 145, 246-247, 257-258, 262, 390-391, 417-419, 421, 439-441, 443-444, 446-450, 467, 474, 480, 483-484, 488, 492, 496-497, 502-503, 666-667, 670-671, 674, 676, 681-682, 709-710, 713-714, 721-723, 725, 727-732, 734-735, 738-739, 741-745, 752, 782, 784-785, 787, 791
src/ipc/shared_memory_blocking.rs 78.87%
(209/265)		 196-198, 200-201, 204-206, 209-210, 212, 217, 219, 223-225, 230, 238-240, 243-245, 248-249, 251, 254, 257-258, 261-262, 266-267, 269, 273-274, 276, 311-312, 378-379, 403-407, 498, 506, 556, 573, 660, 726, 789, 798, 808, 830
src/ipc/shared_memory_direct.rs 83.80%
(150/179)		 372-375, 444-451, 455, 482, 506-509, 513-514, 556-557, 569, 598, 605-606, 629-630, 636
src/ipc/tcp_socket.rs 59.43%
(63/106)		 31-32, 61, 96, 113-114, 118, 124-125, 129, 136-137, 141, 147-148, 152, 171-172, 175-177, 184-185, 188, 362-363, 366-367, 370-371, 376-377, 422, 429, 447-449, 478, 480-482, 484, 487
src/ipc/tcp_socket_blocking.rs 97.62%
(82/84)		 134, 159
src/ipc/unix_domain_socket.rs 59.43%
(63/106)		 29-30, 58, 93, 103, 122-123, 127, 133-134, 138, 145-146, 150, 156-157, 161, 180-181, 184-186, 193-194, 197, 346-347, 350-351, 354-355, 360-361, 412-414, 443, 445-447, 449, 452, 468
src/ipc/unix_domain_socket_blocking.rs 94.34%
(100/106)		 276-277, 283-285, 287
src/logging.rs 100.00%
(13/13)		 ``
src/main.rs 46.11%
(166/360)		 84-86, 88, 125-126, 136-140, 144-146, 148-149, 151-152, 172-175, 199-203, 211, 217, 220, 225-228, 233-234, 240, 246, 248-250, 252, 258-259, 265, 270, 273-274, 278, 280-281, 285-286, 288, 294, 298-299, 301-306, 308-309, 312, 321, 324-325, 328, 375-378, 385, 387-391, 394-397, 399-400, 402-403, 405, 407-413, 417, 419-422, 425, 429-431, 435, 437, 440, 444, 449-452, 458-459, 465-466, 472, 474-475, 479, 481, 486-488, 492, 495-496, 498-499, 504, 506-508, 512-513, 515, 522, 527-528, 530-535, 537-538, 542, 551, 554-555, 558, 560, 579, 586, 590-592, 594, 624-625, 633, 666, 717, 721, 724-727, 783-786, 823-824, 831-832, 835, 862-863, 866, 918-919, 923-926, 948, 975, 984, 989, 994-995
src/metrics.rs 79.79%
(150/188)		 455-460, 493-494, 552, 558, 579-582, 732-734, 736, 768, 788, 833, 838, 881, 904, 923-924, 926-927, 930-932, 952, 980, 984, 1005, 1007-1008, 1013
src/results.rs 56.63%
(252/445)		 726, 735-737, 739-740, 743-744, 747, 769, 772-773, 776, 778, 781, 785-790, 800-801, 804-809, 826, 838-839, 841, 843, 846-847, 849, 853, 880, 904-906, 909-910, 914-916, 919, 945, 950, 955, 961, 980, 982-983, 985, 987-991, 993, 995-996, 1030, 1071-1072, 1075, 1081-1082, 1086, 1090-1092, 1094-1095, 1119-1123, 1126-1129, 1132-1139, 1149-1150, 1169-1170, 1172-1176, 1178, 1195-1196, 1198-1203, 1205, 1223, 1225-1230, 1248, 1251, 1267-1268, 1283-1285, 1287-1289, 1291-1292, 1294-1295, 1297-1298, 1300, 1302-1303, 1305-1308, 1310-1312, 1314-1316, 1319, 1323-1324, 1332-1337, 1339-1340, 1344-1345, 1349-1351, 1353, 1357-1358, 1367-1370, 1374-1376, 1380, 1382-1383, 1388-1389, 1394, 1401-1405, 1407, 1605-1606, 1826-1827, 1829-1830, 1835
src/results_blocking.rs 95.48%
(296/310)		 489-490, 492-493, 544, 769, 774, 779, 815, 818-819, 827-828, 886
src/utils.rs 70.73%
(29/41)		 71, 143, 147-149, 153, 159, 198-202
Total 73.46%
(2893/3938)

dustinblack
dustinblack reviewed Apr 17, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dustinblack dustinblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 unambiguous
  • average_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?

@mcurrier2
fix: Use fully verbose throughput field names per review
d191213 
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
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 17, 2026

📈 Changed lines coverage: 64.29% (9/14)

🚨 Uncovered lines in this PR

  • src/results.rs: 1136-1138, 1176-1177

📊 Code Coverage Summary

File Line Coverage Uncovered Lines
src/benchmark.rs 83.64%
(506/605)		 75, 78, 89, 93, 102, 105, 107, 124, 422, 427-432, 439-444, 511-514, 619, 703, 709-711, 715-717, 737, 806-808, 813, 834, 839, 857, 963, 967-970, 972, 981-984, 986, 1062, 1093, 1096, 1098-1099, 1108-1109, 1251, 1264, 1281, 1404, 1413, 1415-1416, 1419-1420, 1426-1427, 1432-1433, 1435, 1440-1441, 1445-1447, 1452-1453, 1456-1457, 1489, 1547-1551, 1553-1559, 1561, 1564, 1721, 1736
src/benchmark_blocking.rs 73.50%
(319/434)		 97, 111, 127, 263, 369, 375-377, 380-382, 402, 434, 488, 587, 600, 614, 644-647, 732-735, 754, 758, 773, 815-817, 820, 823-825, 827, 830, 832-836, 838-839, 847-851, 853-857, 860-861, 865-866, 901, 950, 1029, 1040, 1070, 1073, 1138-1143, 1145, 1200-1203, 1208, 1221, 1224-1227, 1231, 1233-1236, 1238, 1240-1241, 1243-1244, 1247, 1249-1254, 1256, 1260-1261, 1263, 1265, 1289, 1301-1306, 1308, 1328-1331
src/cli.rs 92.39%
(85/92)		 630, 729, 769, 771, 792-794
src/execution_mode.rs 100.00%
(14/14)		 ``
src/ipc/mod.rs 65.28%
(47/72)		 115, 425, 427-430, 740-741, 756-757, 775-776, 807, 810, 813, 818, 845-846, 860, 862, 882, 884, 1007-1009
src/ipc/posix_message_queue.rs 46.09%
(59/128)		 139-140, 213-215, 217, 224, 229, 332-335, 337, 345, 437, 441-442, 446, 449-452, 454-458, 539, 679, 782, 789-790, 807-808, 819-820, 831-832, 849-850, 906, 910-911, 914-919, 921-923, 927, 929-931, 933, 935-937, 941-943, 945-947, 994-995, 1017
src/ipc/posix_message_queue_blocking.rs 81.94%
(127/155)		 172, 182, 221, 251-255, 274, 325, 368, 387-390, 416-418, 422-423, 425-426, 436, 455, 457-458, 460-461
src/ipc/shared_memory.rs 69.36%
(163/235)		 61, 141, 145, 246-247, 257-258, 262, 390-391, 417-419, 421, 439-441, 443-444, 446-450, 467, 474, 480, 483-484, 488, 492, 496-497, 502-503, 666-667, 670-671, 674, 676, 681-682, 709-710, 713-714, 721-723, 725, 727-732, 734-735, 738-739, 741-745, 752, 782, 784-785, 787, 791
src/ipc/shared_memory_blocking.rs 78.87%
(209/265)		 196-198, 200-201, 204-206, 209-210, 212, 217, 219, 223-225, 230, 238-240, 243-245, 248-249, 251, 254, 257-258, 261-262, 266-267, 269, 273-274, 276, 311-312, 378-379, 403-407, 498, 506, 556, 573, 660, 726, 789, 798, 808, 830
src/ipc/shared_memory_direct.rs 83.80%
(150/179)		 372-375, 444-451, 455, 482, 506-509, 513-514, 556-557, 569, 598, 605-606, 629-630, 636
src/ipc/tcp_socket.rs 59.43%
(63/106)		 31-32, 61, 96, 113-114, 118, 124-125, 129, 136-137, 141, 147-148, 152, 171-172, 175-177, 184-185, 188, 362-363, 366-367, 370-371, 376-377, 422, 429, 447-449, 478, 480-482, 484, 487
src/ipc/tcp_socket_blocking.rs 97.62%
(82/84)		 134, 159
src/ipc/unix_domain_socket.rs 59.43%
(63/106)		 29-30, 58, 93, 103, 122-123, 127, 133-134, 138, 145-146, 150, 156-157, 161, 180-181, 184-186, 193-194, 197, 346-347, 350-351, 354-355, 360-361, 412-414, 443, 445-447, 449, 452, 468
src/ipc/unix_domain_socket_blocking.rs 94.34%
(100/106)		 276-277, 283-285, 287
src/logging.rs 100.00%
(13/13)		 ``
src/main.rs 46.11%
(166/360)		 84-86, 88, 125-126, 136-140, 144-146, 148-149, 151-152, 172-175, 199-203, 211, 217, 220, 225-228, 233-234, 240, 246, 248-250, 252, 258-259, 265, 270, 273-274, 278, 280-281, 285-286, 288, 294, 298-299, 301-306, 308-309, 312, 321, 324-325, 328, 375-378, 385, 387-391, 394-397, 399-400, 402-403, 405, 407-413, 417, 419-422, 425, 429-431, 435, 437, 440, 444, 449-452, 458-459, 465-466, 472, 474-475, 479, 481, 486-488, 492, 495-496, 498-499, 504, 506-508, 512-513, 515, 522, 527-528, 530-535, 537-538, 542, 551, 554-555, 558, 560, 579, 586, 590-592, 594, 624-625, 633, 666, 717, 721, 724-727, 783-786, 823-824, 831-832, 835, 862-863, 866, 918-919, 923-926, 948, 975, 984, 989, 994-995
src/metrics.rs 79.79%
(150/188)		 455-460, 493-494, 552, 558, 579-582, 732-734, 736, 768, 788, 833, 838, 881, 904, 923-924, 926-927, 930-932, 952, 980, 984, 1005, 1007-1008, 1013
src/results.rs 56.38%
(252/447)		 726, 735-737, 739-740, 743-744, 747, 769, 772-773, 776, 778, 781, 785-790, 800-801, 804-809, 826, 838-839, 841, 843, 846-847, 849, 853, 880, 904-906, 909-910, 914-916, 919, 945, 950, 955, 961, 980, 982-983, 985, 987-991, 993, 995-996, 1030, 1071-1072, 1075, 1081-1082, 1086, 1090-1092, 1094-1095, 1119-1123, 1126-1129, 1132-1141, 1151-1152, 1171-1172, 1174-1178, 1180, 1197-1198, 1200-1205, 1207, 1225, 1227-1232, 1250, 1253, 1269-1270, 1285-1287, 1289-1291, 1293-1294, 1296-1297, 1299-1300, 1302, 1304-1305, 1307-1310, 1312-1314, 1316-1318, 1321, 1325-1326, 1334-1339, 1341-1342, 1346-1347, 1351-1353, 1355, 1359-1360, 1369-1372, 1376-1378, 1382, 1384-1385, 1393-1394, 1399, 1406-1410, 1412, 1610-1611, 1831-1832, 1834-1835, 1840
src/results_blocking.rs 95.51%
(298/312)		 489-490, 492-493, 544, 769, 774, 779, 815, 818-819, 827-828, 886
src/utils.rs 70.73%
(29/41)		 71, 143, 147-149, 153, 159, 198-202
Total 73.44%
(2895/3942)

@mcurrier2
Copy link
Copy Markdown
Collaborator Author

mcurrier2 commented Apr 19, 2026

I agree. It's changed to:
average_throughput_megabytes_per_sec — verbose but unambiguous

dustinblack
dustinblack approved these changes Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dustinblack dustinblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verbose field names are unambiguous. Clean rename, CI green. Approved.

sberg-rh
sberg-rh approved these changes Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown

@sberg-rh sberg-rh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, no issues here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dustinblack dustinblack dustinblack approved these changes

@sberg-rh sberg-rh sberg-rh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix: Throughput field names incorrectly labeled as megabits (mbps) instead of megabytes (mb_s)

3 participants

@mcurrier2 @dustinblack @sberg-rh