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

Onionperf updates #9

Merged
merged 8 commits into from
Jul 8, 2020
Merged

Onionperf updates #9

merged 8 commits into from
Jul 8, 2020

Conversation

ana-cc
Copy link
Contributor

@ana-cc ana-cc commented Jun 29, 2020

Hi! Here is a merge request that helps us use tgentools with onionperf. I've tried to integrate the main changes we made to the OP parser. Here is a summary:

  • To make the package uploadable to pypi, it has to have Licence and Readme files (which I've included from other places in the tgen repository) and use setuptools, the successor to distutils.
  • I've found a bug where the start of the transfer (unix_ts_start) was recorded incorrectly, resulting in very large erroneous values for everything that depended on that value. It would be good to double-check this calculation.
  • I've found a bug where the time for completing each decile of a transfer was incorrect, as the conversion between percentage and fraction was missing.
  • The elapsed_seconds key now contains two new subkeys, payload_bytes_send and payload_bytes_recv. This records the elapsed seconds for transferring 10, 20, 50, 100, 200, 500Kb and 1, 2, 5 Mb.
  • All the values under elapsed_seconds are now rounded to 6 decimals
  • Elapsed_seconds are only counted and returned as a key when the transfer did not end in error (to avoid incorrect values)
  • I've refactored a small portion of code and renamed two variables for readability

Thank you very much for your help!

Copy link
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

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

We have two options for the README:

  1. We can just move doc/Tools-Setup.md to tools/README rather than copy it. That way we can avoid having two copies of that file and accidentally updating the incorrect version. If we do this approach, then we'll have to update the link in the tgen top-level README file which currently points to doc/Tools-Setup.md.

or

  1. Just create a minimal README with no content other than a link to the Github doc/Tools-Setup.md file. That way all documentation can remain in the docs directory.

Do you have a preference?

The other code changes seem OK to me, but I didn't test it on any actual TGen output. The continuous integration does some amount of testing though, and I assume you tested on OnionPerf output to make sure it meets your needs.

@ana-cc
Copy link
Contributor Author

ana-cc commented Jul 8, 2020

Many thanks for your comments! I like the idea of having a stand-alone README page in the tgentools directory, so I'd go with Option 1.

With regards to the TGen output, I've spotted another bug where the times in the output would appear in reverse order for very small transfers, and have pushed one more commit. Everything else looks good, we can always open another PR in case we spot anything during testing.

Thank you very much for your help!

@robgjansen
Copy link
Member

OK great!

Would you be able to add a commit that moves doc/Tools-Setup.md to tools/README (i.e., delete the doc/Tools-Setup.md file) and then updates the tgen top-level README file to point to new tools/README location (rather than the old doc/Tools-Setup.md location)?

(I could do it, but since I can't push to your branch I would have to open a new PR. It would be nice to keep all of the discussion and changes on this PR.)

Thank you!

@ana-cc
Copy link
Contributor Author

ana-cc commented Jul 8, 2020

Thank you so much for the quick reply! I've made the changes and pushed the final commit!

@robgjansen robgjansen merged commit 48d684e into shadow:master Jul 8, 2020
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.

2 participants