-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fix megabytes to mbps #394
fix: fix megabytes to mbps #394
Conversation
7879317
to
4432d1c
Compare
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
src/helpers.ts
Outdated
export function megabytesToBytes(megabytes: number) { | ||
return megabytes * 1024 * 1024; | ||
export function megabitsToBytes(megabytes: number) { | ||
return megabytes * 125000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vigneshshanmugam is this the right calculation, or do we need the binary one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 MBps -> (1 * 1024 * 1024 / 8) bytes per second is the correct calculation
/test |
4432d1c
to
dbecac2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI options wording has to be renamed.
throttle network conditions for download throughput (d) in Mbps(Megabits/second), upload throughput (u) in Mpbs(Megabits/second)
downloadThroughput: megabytesToBytes(5), // megabytes/second | ||
uploadThroughput: megabytesToBytes(3), // megabytes/second | ||
downloadThroughput: megabitsToBytes(5), // megabytes/second | ||
uploadThroughput: megabitsToBytes(3), // megabytes/second | ||
latency: 20, // milliseconds, | ||
offline: false, | ||
} | ||
|
||
export function formatNetworkConditionsArgs(networkConditions: NetworkConditions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comment that we need Devtools CDP expects format to be in bytes/second to make it clear.
src/helpers.ts
Outdated
export function megabytesToBytes(megabytes: number) { | ||
return megabytes * 1024 * 1024; | ||
export function megabitsToBytes(megabytes: number) { | ||
return megabytes * 125000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 MBps -> (1 * 1024 * 1024 / 8) bytes per second is the correct calculation
src/helpers.ts
Outdated
export function bytesToMegabytes(bytes: number) { | ||
return bytes / 1024 / 1024; | ||
export function bytesToMegabits(bytes: number) { | ||
return bytes / 125000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverse of previous calc.
dbecac2
to
8dc6a6f
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still missing the changes in the parse args file.
Lines 109 to 110 in 007a705
new Option('--throttling <d/u/l>', 'List of options to throttle network conditions for download throughput (d) in megabytes/second, upload throughput (u) in megabytes/second and latency (l) in milliseconds.') | |
.default(DEFAULT_NETWORK_CONDITIONS_ARG) |
Rest looks good.
/test |
} | ||
|
||
export const DEFAULT_NETWORK_CONDITIONS: NetworkConditions = { | ||
downloadThroughput: megabytesToBytes(5), // megabytes/second | ||
uploadThroughput: megabytesToBytes(3), // megabytes/second | ||
downloadThroughput: megabitsToBytes(5), // Devtools CDP expects format to be in bytes/second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dominiqueclarke I forgot about this in last review, Does this config work with floating points? If not we need to truncate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDP does not report any errors for floating points, and the tests pass as expected when passing with floating points. I did this manually, but happy to write an explicit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need it, it was more of a question and what users would expect. We could round it off or leave it as it is, I will let you take the call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine. As far as user input, I think users will have a need to express mgbps as floating points for more precision. This will sometimes lead to floating points for the corresponding bytes value after applying the calculation. As long as CDP does not report an error, I think it's fine.
…a-fleet-heartbeat-synthetics-e2e-tests * upstream/master: [jjbb] for scheduling e2e jobs (elastic#418) docs: release notes for beta.17 (elastic#415) 1.0.0-beta.17 feat: add --playwright-options (elastic#405) fix: ensure timestamps of events are inorder (elastic#413) fix: do not stash `.nvm` folder (elastic#414) deprecate suite-params option over params (elastic#406) fix: end timestamp should reflect duration (elastic#410) docs: release notes for 1.16.0 (elastic#403) 1.0.0-beta.16 fix: revert Playwright to 1.14.0 (elastic#402) docs: release notes for beta 15 (elastic#400) 1.0.0-beta.15 chore: update deps and fix lint issues (elastic#399) fix: handle page close during response event (elastic#398) chore: require node > 14 for usage (elastic#397) fix: fix megabytes to mbps (elastic#394) feat: enable trace as part of rich-events (elastic#387) feat: use events from PW for network data (elastic#372)
Fixes #284
Fixes the default network throttling to Mbps