Opened 10 years ago

Closed 9 years ago

Last modified 8 years ago

#2930 closed enhancement (implemented)

Overwrite files in stats/ directory rather than appending to them

Reported by: karsten Owned by: karsten
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Right now, we append statistics to files in the stats/ directory for half of the statistics, whereas we overwrite these files for the other half. In particular, we append buffer, dirreq, and entry stats and overwrite exit, connection, and bridge stats.

Appending to files was useful when we didn't include stats in extra-info descriptors, because otherwise we'd have to copy them away to prevent Tor from overwriting them.

But now that we include statistics in extra-info descriptors, it makes no sense to keep the old statistics forever. We should change the behavior to overwriting instead of appending for all statistics.

Child Tickets

Change History (16)

comment:1 Changed 10 years ago by Sebastian

Does that mean we should disable the option to not report statistics via ei?

comment:2 in reply to:  1 Changed 10 years ago by karsten

Replying to Sebastian:

Does that mean we should disable the option to not report statistics via ei?

No. The option to report statistics in extra-info descriptors is set to 1 by default. I think we should leave this option in, so that people can run tests with the other *Statistics options. We can expect these people to copy the files in stats/ once per day if they care.

comment:3 Changed 10 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:4 Changed 9 years ago by karsten

I started working on a patch that overwrites stats/buffer-stats instead of appending to it. While being at it, I decoupled some stuff and added unit tests. This code is entirely untested yet. But if someone wants to give me some feedback, that might be helpful for working on the other *-stats files. See branch bug2930 in my public repository.

comment:5 Changed 9 years ago by nickm

Seems like a sound idea to me. As a cosmetic issue, I'd rather in the future get "change behavior" patches separately from "move code and refactor" patches: It is much easier to review either kind of change when it's isolated from the other kind.

comment:6 Changed 9 years ago by karsten

I was wondering about making separate patches, but I didn't find a clear point to do this separation. The current code is not easy to test, because it relies on (a) the file system and (b) the global list of circuits. The patch for #2930 solved (a), but I wanted to test it and therefore had to implement (b), too. On the other hand, I couldn't implement (b) without implementing (a), or I wouldn't be able to write useful tests for that code, either. Of course, I can now make a separate patch for the changes for (a) without the tests and a second patch with all the tests in a separate patch. That's not how the code was written, but it may be easier to review. Do you prefer that? (I also ask, because I have more patches for this ticket that I'd write similarly.)

comment:7 Changed 9 years ago by nickm

Hm. For future patches, the important thing for making it easier to review isn't separating tests from other stuff: the important thing is to separate code movement and refactoring from behavioral changes. So I'd suggest that maybe you divide future things of this kind into one untested patch that does *only* the code movement without adding new stuff, and another that changes the behavior and adds tests. Does that make sense, or am I missing something that makes it unreasonably hard?

comment:8 Changed 9 years ago by karsten

Ah, I didn't exactly mean to separate tests from other stuff. It's only the most reasonable separation point for this commit. I just implemented that and pushed a new branch bug2930-2 to my public repository. Do you mind having a look? (Again, it's still untested and not ready to be merged, but I'd use your input to write the following patches to stuff in geoip.c in a similar way.)

comment:9 Changed 9 years ago by karsten

After talking to rransom in #tor-dev, I came up with a branch that now has four commits. See bug2930-3 in my public repository. Feedback welcome!

comment:10 Changed 9 years ago by karsten

Status: newneeds_review

Please review branch bug2930-4 in my public repository. It implements all changes for this ticket (which turned out to be only three changed lines; ugh) and refactors a lot of code that makes the code easier to understand and that finally allows us to write unit tests for it.

This code is now tested. I ran it on a relay with reduced measurement interval of 1 hour and no inclusion of statistics in extra-info descriptors. After that, I rebased some stuff, so I'll have to test it once again once it's in master. (Please don't close the ticket after merging until I completed these tests.)

Thanks in advance for reviewing this code!

comment:11 Changed 9 years ago by nickm

Looks good to me. Thanks for splitting it up! I'm merging it now; please close it once you're done testing.

comment:12 Changed 9 years ago by karsten

Ah, note that bug2930-4 contained more code changes than bug2930. Namely, I started rewriting the buffer-stats code (which you reviewed) and then refactored the dirreq-stats, entry-stats, and bridge-stats code (which you may or may not have reviewed). I *think* there shouldn't be surprises in that new code, because I rewrote it similarly to the buffer-stats code. But better look in detail if you haven't already. Sorry if I wasn't clear about that!

I just fired up my test relay again which now runs master. I'll let you know tomorrow whether it exploded.

comment:13 Changed 9 years ago by karsten

Owner: set to karsten
Status: needs_reviewassigned

My test relay didn't explode over night. I reconfigured it to collect stats using the default 24 hour interval and to include the results in its extra-info descriptors. After the weekend we'll be more certain that the changes didn't break anything important.

comment:14 Changed 9 years ago by karsten

Resolution: implemented
Status: assignedclosed

So, my relay still works fine after the weekend. Closing.

comment:15 Changed 8 years ago by nickm

Keywords: tor-relay added

comment:16 Changed 8 years ago by nickm

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