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

[Tor Bug Tracker & Wiki]

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

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:16 nickm]:
 > So, my questions about efficiency and performance are really questions,
 and not rhetorical.  I'm asking: in practice, when you turn this feature
 on and run a reasonably busy simulation, is the performance/memory hit
 acceptable or not?
 >
 > I'm asking that because I suspect that it's going to consume a whole lot
 of memory.  And if it does, we can make it more efficient.  But if it
 doesn't (and you've tested it out, so I'm hoping you will know), then
 there is not any point in putting the effort into making it more
 efficient.
 >
 > Does that make sense?

 I finally have some Shadow results.  This took much longer than expected,
 but the good news is that I found (and fixed) a segfault in my tor branch
 with Shadow.

 Please see
 [https://trac.torproject.org/projects/tor/attachment/ticket/7359/karsten-
 morestats4-mem-total.pdf attached graph] which shows memory usage over
 time.  Black is the tor commit before my first morestats4 commit, red is
 my morestats4 branch where all the new events are disabled, and blue is my
 morestats4 branch with everything enabled.  I don't see much of a
 difference in memory usage.

 > More issues on review:
 >   * You're passing struct timeval on the stack; our convention is to
 pass around a "const struct timeval *" instead.

 Fixed in my updated morestats4 branch.

 >   * What keeps the various bandwidth events from getting enabled when
 not in Testing mode? I'm seeing that for some but not all of the other new
 event types.

 The CIRC_BW event is not limited to testing mode, because it's only
 emitted for origin circuits.  Should I make this clearer somewhere?  Or do
 you think it's a problem that non-testing nodes can enable this event
 type?

 >   * The uint64_t * pointer arguments to append_cell_stats_by_command
 should be const.  That function's documentation should document the
 lengths of those arrays.

 Fixed in my updated morestats4 branch.

 > Something to do AFTER merge:
 >   * Go through all the core functions that grew new if blocks here, and
 move the bodies of those if blocks into new functions.  We have lots of
 places in Tor where ancillary functionality is crammed into core
 functions, but we shouldn't perpetuate that.

 Will do.

 > For the next branch like this:
 >   * Please learn the --fixup and --squah arguments to git commit.

 So, I now know how to use those commands, and I agree I should use them.
 But I'm not sure how you'd want the morestats4 branch to look like in the
 end.  Do you want just the five commits saying "Add XY" and the rest as
 fixup! commits?  Or the "Tweak XY" commits as squash! commits, because
 they contain somewhat useful commit messages what fixes you suggested?  Or
 something else?  In any case, I'm happy to do a rebase and push a
 morestats5 branch before you merge, using a commit history that you like.

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