[tor-bugs] #7359 [Tor]: Design/implement method for collecting/reporting statistics

[Tor Bug Tracker & Wiki]

#7359: Design/implement method for collecting/reporting statistics
------------------------------------------------------------------------+---
 Reporter:  robgjansen                                                  |          Owner:                    
     Type:  enhancement                                                 |         Status:  needs_revision    
 Priority:  normal                                                      |      Milestone:  Tor: 0.2.5.x-final
Component:  Tor                                                         |        Version:                    
 Keywords:  performance, simulation, statistics, tor-relay, tor-client  |         Parent:  #7357             
   Points:                                                              |   Actualpoints:                    
------------------------------------------------------------------------+---
Changes (by nickm):

  * status:  needs_review => needs_revision


Comment:

 quick comments:

 General:
   * This needs to go along with a patch to control-spec.txt .

 e54d664f7bb1205162c1df3495f8ebc30c23d867
  * The last time we added K=V arguments in the middle of controller event,
 it created trouble for controllers, even though it wasn't supposed to do
 so, since the spec says we can.  Since nobody's actually hacking on
 Vidalia, would it be possible to just put the new K=V arguments at the
 end, to avoid possible trouble?

 8d1f78c556abb570bb80ea84261c954d9746cf33
   * Instead of being controlled by TestingTorNetwork, should there be a
 separate option for EVENT_CONN_BW events?
   * The documentation for n_read and n_written should say that they're
 only used for EVENT_CONN_BW events, and only turned on conditionally.
 They should also probably have a name that implies that they are only for
 stats too.
  * The new code in control_update_global_event_mask should get extracted
 into a function.


 c386d2d6ce4c4f58163acb385c7a5de1da8c5e30
   * "extern circuit_t *global_circuitlist" is really not a great way to
 mediate access to this thing; we should add an accessor function.
  * cell_command_to_string doesn't really belong in control.c
  * append_cell_stats_by_command doesn't seem to document how long the
 arrays are.  I might be a bit more comfortable if it took a structure
 instead.  This
  * Accessing num_used in smartlist_t is now how we do it.  Call
 smartlist_len like every other place in the code.
  * include_if_positive is a weird name for an unsigned type.  It can't be
 negative, after all.
  * control_event_circuit_cell_stats seems to be really huge and full of
 duplicated code.  Can it be refactored into smaller functions?
  * Is it "exitward" or "exit_ward"?  I prefer the former.
  * Prefer smartlist_add_asprintf() to tor_asprintf();smartlist_add().
  * Is this really slow in practice? It seems like it's likely to be really
 slow.
  * Ditto about having another controlling option.  We don't want
 TestingTorNetwork to do everything at once.
  * CELL_COMMAND_MAX would be a better name than CELL_MAX
  * Prefer TOR_SIMPLEQ to manual list management for new queues.
  * This data structure ... how does it work out in practice? It looks like
 it's going to be hideously inefficient.
  * This code needs unit tests and functional abstraction badly.


 dd5ce2157d8c47ffa3686b0579813e7b1aae8069
  * The documentation for the new global_*_emptied thing doesn't really say
 what they are.  Do you mean "Last time at which the ... buckets were
 emptied in msec since midnight", or something else?
  * "connection_buckets_empty_ts" -- I can't read that name as a verb
 phrase.  Our practice is to try to make function that do things have an
 active verb in their names.
  * The int cast in that function doesn't give me a great feeling.
  * Here too, TestingTorNetwork should not mean everything.
  * The code to convert time to "msec since midnight" is duplicated code.
 Let's try not to duplicate code.
  * Holy moly, it's a 13-argument function!!!!!  Please, let's be better
 than that.

 2925e2fe786dfd9a27c2dff503996712cd180de4
  * See above about the name and documentation for n_read and n_written.
  * The new code in control_update_global_event_mask should get extracted
 into a function.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7359#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online