Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#1746 closed defect (fixed)

Allow statistics options to be changed without restarting Tor

Reported by: Sebastian Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: ln5, ioerror, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Please see branch stats in my repo.

Child Tickets

Change History (19)

comment:1 Changed 9 years ago by Sebastian

Component: - Select a componentTor - Relay

comment:2 Changed 9 years ago by ln5

Shouldn't the test in the log_info() call be the same as the one for determining whether to increase int_start or not? I.e. old_options->ORPort == options->ORPort rather than just old_options->ORPort.

comment:3 Changed 9 years ago by Sebastian

Good point. stats_v2 in my repo from now on please :)

comment:4 Changed 9 years ago by ln5

I would probably s/was_relay/was_relay_flag/g but that's just me.

comment:5 Changed 9 years ago by nickm

Keywords: review removed
Status: newneeds_review

comment:6 Changed 9 years ago by karsten

How's the review going? I'm soon going to add two new metrics that we promised for August 15 (http://archives.seul.org/or/dev/Jul-2010/msg00016.html). It'd be grand if we could use the same logic to turn these metrics on/off while Tor is running. Thanks!

comment:7 Changed 9 years ago by nickm

Reviewing now. Some notes:

  • 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
  • Maybe we should use booleans to tell if stats collection types are initialized/running/whatever instead of doing stuff like
    +  if (!start_of_entry_stats_interval)
    +    return 0; /* Not initialized. */
    
  • 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?
  • 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.
  • wrt the message "We are no longer acting as a bridge. Forgetting GeoIP stats.", what code actually does that?

comment:8 Changed 9 years ago by Sebastian

Some comments on your notes:

  • 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.
  • 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.
  • The way we are "forgetting" geoip stats is that we re-initialize them to be empty when they are enabled next time. This re-initialization sets start_of_bridge_stats_interval in geoip.c to now, and in geoip_bridge_stats_write() everything that was recoreded before start_of_bridge_stats_interval gets removed. That does not look like fantastic design, but should probably get cleaned up in a later patch.

comment:9 Changed 9 years ago by nickm

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.

What I'm confused about is ... where did the arrays in country_t go? Why did the arrays go?

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.

comment:10 Changed 9 years ago 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.

comment:11 Changed 9 years ago by nickm

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?

The order doesn't greatly matter, but adding another patch to clean up code would always be neat.

We don't collect data for more than 1 interval anymore, but only for complete 24 hour intervals

This is a new change with this patch, or an old change? And if it's a new change with this patch, what's the rationale? (If it was an old change, do you remember why we did it?)

Should we separate the code refactoring parts from the bugfixing parts?

In the future, yes: it's way easier to review a commit if you know "this commit isn't supposed to change functionality." For this patch, it probably isn't necessary, since we've already been reviewing it.

comment:12 in reply to:  11 Changed 9 years ago by karsten

Replying to nickm:

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?

The order doesn't greatly matter, but adding another patch to clean up code would always be neat.

Added a patch for that.

We don't collect data for more than 1 interval anymore, but only for complete 24 hour intervals

This is a new change with this patch, or an old change? And if it's a new change with this patch, what's the rationale? (If it was an old change, do you remember why we did it?)

It's an old change (see commit 54c97c91). The rationale is that we might give out too much information by reporting values for smaller intervals than 24 hours. If you compare the values of request periods 1 to N to the values of periods 2 to N (after period 1 got dropped), you can derive the values of period 1. The other reason is that we are fine with daily updated stats, so we should collect just those data.

Should we separate the code refactoring parts from the bugfixing parts?

In the future, yes: it's way easier to review a commit if you know "this commit isn't supposed to change functionality." For this patch, it probably isn't necessary, since we've already been reviewing it.

Okay, leaving this as it is.

See branches stats_v4 and stats_v4_rebased in my public repository. The first branch moves around the exit stats code, whereas the second branch further rebases everything to master and squashes my original patch with my minor changes.

comment:13 Changed 9 years ago by karsten

I should say I updated the stats_v4_rebased branch yesterday, because it didn't compile after rebasing. Stupid or.h splitting. ;) I hope the branch is fine now.

comment:14 Changed 9 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks good; merged the stats_v4_rebased branch.

comment:15 Changed 9 years ago by karsten

Resolution: implemented
Status: closedreopened

I forgot to update the man page. See branch stats-manpage in my public repository.

comment:16 Changed 9 years ago by Sebastian

Status: reopenedneeds_review

Looks good.

comment:17 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Agreed; merging.

comment:18 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:19 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.