#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: <https://trac.torproject.org/projects/tor/ticket/34191#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online