Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  closed
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:  implemented
 Keywords: |  Actual Points:  1
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+
Changes (by acute):

 * actualpoints:  0.9 => 1


Comment:

 Thanks very much for the reviews! Adding my last 0.1 points to yours :)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  closed
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:  implemented
 Keywords: |  Actual Points:  0.9
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+
Changes (by karsten):

 * status:  needs_review => closed
 * resolution:   => implemented
 * actualpoints:  0.8 => 0.9


Comment:

 Looks good to me, including that minor argparse hack. Rebased and pushed
 to master. Closing. Thanks so much!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  needs_review
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:  0.8
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+
Changes (by karsten):

 * status:  needs_revision => needs_review


Comment:

 Thanks, will take another look soon!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  needs_revision
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:  0.8
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+

Comment (by acute):

 Replying to [comment:8 karsten]:
 > Quick thought: How about we accept a single `PATH` argument (followed by
 the `LABEL` argument) but accept a file or directory or (quoted) path
 pattern expanded by OnionPerf rather than the shell? The command in your
 example would then be: `onionperf visualize -d "onionperf-2019-11/*/*nl*"
 nov2019 -d ...` (note the quotes). Untested! After all, if the user wants
 to use spaces in their labels, they'll have to quote the label, too.
 This could work - the only possible issues would be shell compatibility,
 and, of course, the user would still have to create directories if wanting
 to aggregate measurements over say, different instances or files without
 similar names.

 I pushed fixes to address the label and usage concerns you had previously:
 https://github.com/ana-cc/onionperf/commits/34191

 However, if there are still objections to accepting multiple paths, I can
 explore the option you suggest here.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  needs_revision
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:  0.8
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+
Changes (by karsten):

 * actualpoints:  0.5 => 0.8


Comment:

 Adding my 0.3 actual points that I worked on this ticket so far to yours.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  needs_revision
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:  0.5
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+

Comment (by karsten):

 Quick thought: How about we accept a single `PATH` argument (followed by
 the `LABEL` argument) but accept a file or directory or (quoted) path
 pattern expanded by OnionPerf rather than the shell? The command in your
 example would then be: `onionperf visualize -d "onionperf-2019-11/*/*nl*"
 nov2019 -d ...` (note the quotes). Untested! After all, if the user wants
 to use spaces in their labels, they'll have to quote the label, too.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  needs_revision
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:  0.5
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+

Comment (by acute):

 Replying to [comment:6 karsten]:
 > Thanks for working on this and providing a branch! I read your comment
 and the code but did not test anything yet. Two concerns:

 Thank you very much for the review!
 >
 >  - I'm not sure if we can make it work with a variable number of
 arguments to the `-d` parameter. The usage string is really misleading by
 saying `-d PATH [LABEL ...]`, indicating that it accepts a single `PATH`
 and zero or more `LABEL`s. We would want it to say `-d PATH [PATH ...]
 LABEL` there. Do you know how to override that string? I didn't find
 something and stopped looking at options that felt like hacking argparse.
 From the top of my head, I think this is what `METAVAR` does in
 `argparse`, will revise.
 >
 >  - Regarding the limitation that the `LABEL` cannot be a path anymore, I
 think that's problematic. For one, it's turning this change into a
 backward incompatible one. And it's also not intuitive for new users who
 don't know how the parameter works today. Imagine a case where somebody
 puts all files for OnionPerf instance op-hk into one directory `op-hk/`
 and all files for op-ab into another one `op-ab/`. If they want to
 visualize these files using OnionPerf instance names as labels, they'd
 either have to pick different labels or rename the directories. Doable,
 but for sure surprising.
 >
 That is a very good point!  We could turn the error into a warning to
 allow the user to run the command anyway. We could not show this warning
 if the label is a path already included the list of inputs specified by
 the user. What do you think?
 > Maybe it's better to keep this simple by allowing just two arguments as
 we do right now. Users will understand that they have to put all files for
 one data set into a directory. No surprises, they'll get what they want.
 What do you think?


 What happens when we want to produce analyses comparing several months,
 for each instance? Currently, with the data as it is downloaded from
 collector, files from all the instances are in the same month folder,
 which makes producing a graph like the one just described hard - you'd
 have to create several directories, move files around and then move them
 back if you want to look at the data aggregated in a different way.
 With this, you can just do:
 `onionperf visualize -d onionperf-2019-11/*/*nl* nov2019 -d
 onionperf-2019-12/*/*nl* dec2019 -d onionperf-2020-01/*/*nl* jan2020`

 The point is to avoid manually moving things around in the file system,
 which is where mistakes happen.
 >
 > Apart from these concerns about the user interface, the patch looks good
 to me. The decisions about using `*json*` as pattern (you might want to
 use `*onionperf.analysis.json*` here, now that I think about it) or using
 functionality from the reprocessing module look good to me. I'd say if we
 can figure out the potential user interface issues, this will be a quick
 review. Thanks!
 I will move to using the pattern you suggested, thank you!

 I will work on this patch as my next task.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  needs_revision
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:  0.5
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+
Changes (by karsten):

 * status:  needs_review => needs_revision


Comment:

 Thanks for working on this and providing a branch! I read your comment and
 the code but did not test anything yet. Two concerns:

  - I'm not sure if we can make it work with a variable number of arguments
 to the `-d` parameter. The usage string is really misleading by saying `-d
 PATH [LABEL ...]`, indicating that it accepts a single `PATH` and zero or
 more `LABEL`s. We would want it to say `-d PATH [PATH ...] LABEL` there.
 Do you know how to override that string? I didn't find something and
 stopped looking at options that felt like hacking argparse.

  - Regarding the limitation that the `LABEL` cannot be a path anymore, I
 think that's problematic. For one, it's turning this change into a
 backward incompatible one. And it's also not intuitive for new users who
 don't know how the parameter works today. Imagine a case where somebody
 puts all files for OnionPerf instance op-hk into one directory `op-hk/`
 and all files for op-ab into another one `op-ab/`. If they want to
 visualize these files using OnionPerf instance names as labels, they'd
 either have to pick different labels or rename the directories. Doable,
 but for sure surprising.

 Maybe it's better to keep this simple by allowing just two arguments as we
 do right now. Users will understand that they have to put all files for
 one data set into a directory. No surprises, they'll get what they want.
 What do you think?

 Apart from these concerns about the user interface, the patch looks good
 to me. The decisions about using `*json*` as pattern (you might want to
 use `*onionperf.analysis.json*` here, now that I think about it) or using
 functionality from the reprocessing module look good to me. I'd say if we
 can figure out the potential user interface issues, this will be a quick
 review. Thanks!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  needs_review
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:  0.5
Parent ID:  #33321 | Points:  0.5
 Reviewer:  karsten|Sponsor:  Sponsor59-must
---+
Changes (by karsten):

 * reviewer:   => karsten


Comment:

 I'll take a look, but probably later today or tomorrow. Thanks!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  needs_review
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:  0.5
Parent ID:  #33321 | Points:  0.5
 Reviewer: |Sponsor:  Sponsor59-must
---+
Changes (by acute):

 * status:  accepted => needs_review
 * actualpoints:   => 0.5


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  accepted
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:
Parent ID:  #33321 | Points:  0.5
 Reviewer: |Sponsor:  Sponsor59-must
---+

Comment (by acute):

 This is now ready to review at: https://github.com/ana-
 cc/onionperf/tree/34191

 The visualize command now accepts a mix of one or more multiple files or
 directories in each dataset!
 {{{
 onionperf visualize -d file1 file2 label1 -d file3 file4 label2

 onionperf visualize -d dir1 label1

 onionperf visualize -d dir1 dir2 label1 -d dir3 dir4 label2

 onionperf visualize -d dir1 file1 file2 label1 -d file3 file4 dir2 label2
 ...etc
 }}}

 There have been quite a few changes:

   * We reuse the existing command `reprocessing.collect_logs` in the
 reprocessing module, with the search pattern `*json*` to find analysis
 files in supplied directories; I thought about using `*json.xz` as a
 pattern as well, but it probably does not matter too much. This seems to
 work nicely!

   * All the analysis files given by the user or found in directories are
 collected in a list of 'analyses'

   * The visualization module now iterates through the list of supplied
 'analyses' to load data

  * The label can no longer be a path - that is, if we detect the last
 argument supplied to this command is a path, we complain. This is to avoid
 the case where the user forgets to supply a label, and instead the last
 path supplied is used as a label instead:

 {{{
 $ onionperf visualize -d file1 file2 file3
 usage: onionperf visualize [-h] -d PATH [LABEL ...] [-p STRING]
 onionperf visualize: error: argument -d/--data: The supplied label cannot
 be a path
 }}}

 Let me know if you have any questions, this is quite a sizeable change!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+
 Reporter:  karsten|  Owner:  acute
 Type:  enhancement| Status:  accepted
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal | Resolution:
 Keywords: |  Actual Points:
Parent ID:  #33321 | Points:  0.5
 Reviewer: |Sponsor:  Sponsor59-must
---+
Changes (by acute):

 * status:  new => accepted
 * owner:  metrics-team => acute


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

[tor-bugs] #34191 [Metrics/Onionperf]: Combine multiple analysis files into single data set

[Tor Bug Tracker & Wiki]

#34191: Combine multiple analysis files into single data set
---+--
 Reporter:  karsten|  Owner:  metrics-team
 Type:  enhancement| Status:  new
 Priority:  Medium |  Milestone:
Component:  Metrics/Onionperf  |Version:
 Severity:  Normal |   Keywords:
Actual Points: |  Parent ID:
   Points:  0.5|   Reviewer:
  Sponsor:  Sponsor59-must |
---+--
 OnionPerf's visualize mode currently supports specifying multiple data
 sets to be graphed with different labels for each data set. However, for
 each data set it's only possible to specify exactly one analysis results
 file. If we ever want to graph a data set covering more than a single day
 and compare that to another data set, we cannot do that at the moment.

 Here's a possible user interface extension to allow specifying one or more
 analysis results files for each data set:

 {{{
 diff --git a/onionperf/onionperf b/onionperf/onionperf
 index cb1899c..957c6df 100755
 --- a/onionperf/onionperf
 +++ b/onionperf/onionperf
 @@ -326,8 +326,9 @@ files generated by this script will be written""",
  visualize_parser.set_defaults(func=visualize,
 formatter_class=my_formatter_class)

  visualize_parser.add_argument('-d', '--data',
 -help="""Append a PATH to a onionperf.analysis.json analysis
 results file, and a LABEL that we
 -should use for the graph legend for this dataset""",
 +help="""Append a file or directory PATH to an
 onionperf.analysis.json
 +analysis results file or directory of such files, and a
 LABEL
 +that we should use for the graph legend for this
 dataset""",
  metavar=("PATH", "LABEL"),
  nargs=2,
  required="True",
 }}}

 I didn't write any other code for this yet.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs