Skip to content
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

This adds 10MiB downloads hack to tornettools [Discussion] #73

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hiromipaw
Copy link
Contributor

@hiromipaw hiromipaw commented Apr 5, 2022

This adds the possibility to test 10MiB downloads with tornettools. See https://gitlab.torproject.org/jnewsome/sponsor-61-sims/-/issues/15

For the time being this is a test that is going to be used for the S61 simulation work within Tor. As mentioned in the gitlab issue we might think to add different download sizes to https:/shadow/tornettools/blob/main/tornettools/generate_defaults.py later on.

Please not that I have added the option to parse the same download size from onionperf. This is not needed at this point since we do not have onionperf clients producing these tests. I have mainly added this for consistency and in case we might consider having a few onionperf clients producing these test in the not so far future.

@@ -159,6 +159,11 @@ def __handle_stream(db, stream, day):
if goodput is not None:
db['client_goodput_5MiB'].append(goodput)

goodput = __goodput_bps(
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI I think this isn't going to find anything since this code is parsing the public tor data, which doesn't have 10 MB downloads. (It shouldn't hurt anything either, though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes did mentioned it above: "Please not that I have added the option to parse the same download size from onionperf. This is not needed at this point since we do not have onionperf clients producing these tests. I have mainly added this for consistency and in case we might consider having a few onionperf clients producing these test in the not so far future."

for torperf_db in torperf_dbs:
torperf_db['data'] = [torperf_db['dataset']['download_times'][bytes_key]]
if int(bytes_key) <= 5242880:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok for a one-off, but if the aim was to merge this we'd probably want to avoid hard coding this constant here, and instead just make this generally handle a size missing from the public data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the idea was not to merge this like this. Should I refactor this so that it can be merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's fine do this as a one-off, was just double checking.

It'd be helpful to add a note to the PR description that this PR is for discussion purposes and isn't actually a request to merge

@robgjansen
Copy link
Member

It seems this is for discussion only - do not merge.

@hiromipaw hiromipaw changed the title This adds 10MiB downloads hack to tornettools This adds 10MiB downloads hack to tornettools [Do not merge] Apr 13, 2022
@hiromipaw hiromipaw changed the title This adds 10MiB downloads hack to tornettools [Do not merge] This adds 10MiB downloads hack to tornettools [Discussion] Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants