[tor-bugs] #1746 [Tor - Relay]: Allow statistics options to be changed without restarting Tor

[Tor Bug Tracker & Wiki]

#1746: Allow statistics options to be changed without restarting Tor
-------------------------+--------------------------------------------------
 Reporter:  Sebastian    |       Owner:              
     Type:  defect       |      Status:  needs_review
 Priority:  normal       |   Milestone:              
Component:  Tor - Relay  |     Version:              
 Keywords:               |      Parent:              
-------------------------+--------------------------------------------------

Comment(by karsten):

 Thanks for the reviews so far!

 Nick wrote:
 > The geoip and rephist stats have gotten really complicated. It would be
 good to have a list at the head of each file explaining what stats are
 stored there, and to have all the variables that relate to storing one
 kind of thing put together
 I updated the \brief documentations at the heads of these two files. We
 might further move the exit port statistics code in rephist.c after
 rep_hist_free_all() and before cell statistics. Should we do that as a
 separate commit logically preceding this patch?

 > Maybe we should use booleans to tell if stats collection types are
 initialized/running/whatever instead of doing stuff like ...
 I added comments saying that an interval start of 0 means we're not
 collecting these stats. Let me know if you want something else.

 > I don't see what replaces the old "have N intervals of data per country"
 logic in per-country request counts, or why it's taken out. What does that
 have to do with making the stats settings changeable?
 We don't collect data for more than 1 interval anymore, but only for
 complete 24 hour intervals. Once we have 24 hours of data, we write them
 to disk and forget about the data in memory. This change is not related to
 making the stats setting changeable, I just happened to clean up the N
 interval thing when going through geoip.c to write this patch. Should we
 separate the code refactoring parts from the bugfixing parts? I might even
 remember the Git uber-commands to split a commit, but I think this will
 break the history of our current review branch, right?

 > When you're making a near-1000 line diff, it would be good if the commit
 message explained not only the intent of the change, but also the
 mechanism. As it stands, I am trying to infer design from code, which
 shouldn't be necessary.
 I made the commit message for my last commit more verbose. This commit
 message should survive the final commit squashing, if possible.

 Sebastian wrote:
 > I think that's a good idea, but probably for a later patch. I was aiming
 for that in my heartbeat work, but that got interrupted by
 microdescriptors as an actual deliverable.
 What is "that" in your first sentence?

 > per-country (or geoip or entrystatistics) (all mean the same thing, and
 yes this is very confusing) seem to use the same behaviour as bridge
 statistics now. Not sure if that is intended.
 I stumbled over this a couple of times, too. But I think it's probably the
 best way to have these three stats (dirreq, entry, bridge) implemented by
 the same code. Not sure how we can improve documentation to make that more
 clear.

 >> The way we are "forgetting" geoip stats is that we re-initialize them
 to be empty when they are enabled next time. [...] That does not look like
 fantastic design, but should probably get cleaned up in a later patch
 > Hm. I'd really like it if it were cleaned up in a patch in this branch.
 If that seems non-doable, we should open a bug targetted for
 0.2.2.x-final. When we tell the user "forgetting X", we should really take
 pains to forget X immediately and reliably.
 I added a geoip_bridge_stats_term() that forgets about all seen clients
 immediately.

 See stats_v3 in my public repository.

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