Port OnionPerf to Python 3

Currently OnionPerf is written in Python 2, which is soon to be end-of-life.

(Copied from https://github.com/robgjansen/onionperf/issues/41)

Trac:
Parent Ticket: #33321 (moved)

added actualpoints::1.3 component::metrics/onionperf metrics-team-roadmap-2020 owner::karsten parent::33321 points::2.0 priority::medium resolution::fixed reviewer::phw severity::normal sponsor::59-must status::closed type::defect labels

Updating owner and CC

Trac:
Cc: karsten to karsten, acute
Status: new to assigned
Owner: hiro to metrics-team

These issues are now tracked at: https://dip.torproject.org/torproject/metrics/onionperf/issues

Trac:
Resolution: N/A to invalid
Status: assigned to closed

Moving all gitlab OP tickets back to Trac.

Trac:
Status: closed to reopened
Resolution: invalid to N/A

I made a start in commit 506ea16 in my task-29367 branch. It's probably 80% done now, but I have to stop working on this now. It would be good if we don't wait too long with this change to avoid having to resolve as many merge conflicts. If anybody else wants to pick this up before next week, please feel free to do that!

Trac:
Type: project to defect

Trac:
Keywords: N/A deleted, metrics-team-roadmap-2020 added
Sponsor: N/A to Sponsor58

Trac:
Points: N/A to 2

Trac:
Parent: N/A to #33318 (moved)

Grabbing this for a day or two.

Trac:
Owner: metrics-team to karsten
Status: reopened to assigned

I tweaked my first commit above and rebased it to master. The new commit is e5f15ea in my task-29367-2 branch. It passes unit tests and at least doesn't break in obvious ways. I just deployed a test instance and will let it run for a day to see if it produces useful results.

Having a careful review by somebody who has ported Python code from 2 to 3 before would be greatly appreciated!

Trac:
Status: assigned to needs_review

Trac:
Sponsor: Sponsor58 to Sponsor59

Replying to karsten:

Having a careful review by somebody who has ported Python code from 2 to 3 before would be greatly appreciated!

I have some experience and can take this on. A better person would be atagar (copied) but if he lacks time or interest, I'll get to it.

Trac:
Cc: karsten, acute to karsten, acute, atagar

Trac:
Cc: karsten, acute, atagar to karsten, acute, atagar, phw

Your patch looks good to me but I found two additional issues:

• I assume you're running the tests by executing run_tests.sh? The tests test_reprocessing.test_log_collection_tgen and test_reprocessing.test_log_collection_torctl fail for me because the two paths in the lists differ in their ordering. These unit tests rely on os.walk, which internally calls os.scandir, which yields files in random order. Looks like these tests are broken but we can fix this issue by calling sorted on the arbitrarily ordered file names. Here's a patch.

• I ran the tool 2to3 over all source files and it suggested turning dict_keys and dict_values objects in analysis.py into list. Here's a patch. All unit tests still pass.

Trac:
Status: needs_review to needs_revision

I forgot to mention that we should also update README.md as it still contains python 2 references. Here's a patch for that. All my patches are based on your task-29367-2 branch and should be easy to apply.

Trac:
Reviewer: N/A to phw

Thanks for the review and patches!

Regarding the second patch that turns dict keys and values into lists, I saw those changes, too, and did not make them on purpose. The reason is that we only need to iterate over these keys or values and don't need them as a list for further processing. But of course it doesn't do harm to convert them into lists, except for trivially lower performance. I think it's fine to just apply your patch.

While trying out my branch (before your patches and afterwards), I found another problem in the analysis mode: When OnionPerf opens the Tor control log files containing \r\n as newline, it converts these newlines to just \n. But that makes Stem sad which refuses to parse the contained events. I wrote a patch that enables OnionPerf to open Tor control log files with \r\n as newline and that leaves them unchanged before it passes lines to Stem. My test instance that ran over night successfully produced an analysis file with TGen and Tor control log parts.

This patch is in commit f863ee4 in my task-29367-2 branch which is based on your branch. Can you take another look at that commit? If it looks good, I'll merge everything to master.

Trac:
Status: needs_revision to needs_review

Replying to karsten:

This patch is in commit f863ee4 in my task-29367-2 branch which is based on your branch. Can you take another look at that commit? If it looks good, I'll merge everything to master.

Looks good to me.

Trac:
Status: needs_review to merge_ready

Sounds great! Merged to master and closing. Thanks!

Trac:
Points: 2 to 2.0
Status: merge_ready to closed
Resolution: N/A to fixed
Actualpoints: N/A to 1.3

Moving to Sponsor59-must, because we should really do these in order to call Sponsor59 done.

Trac:
Sponsor: Sponsor59 to Sponsor59-must

Trac:
Parent: #33318 (moved) to #33321 (moved)

closed

changed time estimate to 16h

added 10h 24m of time spent

mentioned in issue #33321 (moved)