Opened 11 months ago
Closed 9 months ago
#29018 closed enhancement (fixed)
Make all statistics depend on ExtraInfoStatistics
Reported by: | teor | Owned by: | teor |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: 0.4.1.x-final |
Component: | Core Tor/Tor | Version: | |
Severity: | Normal | Keywords: | asn-merge, security-low, 040-deferred-20190220, privcount, dgoulet-merge, network-team-roadmap-2019-Q1Q2, merge-after-0403-alpha |
Cc: | Actual Points: | 2 | |
Parent ID: | Points: | 0.1 | |
Reviewer: | nickm | Sponsor: | SponsorV-can |
Description
Like #29017, when a user sets ExtraInfoStatistics 0, they probably don't want any statistics in their extra-info document.
Child Tickets
Change History (25)
comment:1 Changed 11 months ago by
Status: | assigned → needs_review |
---|
comment:2 Changed 11 months ago by
Sponsor: | → SponsorV-can |
---|
Improving the reliability and usability of statistics can fall under Sponsor V.
comment:3 Changed 11 months ago by
Reviewer: | → nickm |
---|
comment:4 follow-up: 5 Changed 11 months ago by
Status: | needs_review → merge_ready |
---|
This looks fine to me. A quick question before I merge: I see that coverage has fallen here. Do you think you would time to add a test for this stuff?
comment:5 follow-up: 6 Changed 11 months ago by
Status: | merge_ready → needs_revision |
---|
Replying to nickm:
This looks fine to me. A quick question before I merge: I see that coverage has fallen here. Do you think you would time to add a test for this stuff?
I could tweak the existing tests to run with ExtraInfoStatistics 0 and 1. That should improve coverage compared to master.
comment:6 Changed 10 months ago by
Replying to teor:
Replying to nickm:
This looks fine to me. A quick question before I merge: I see that coverage has fallen here. Do you think you would time to add a test for this stuff?
I could tweak the existing tests to run with ExtraInfoStatistics 0 and 1. That should improve coverage compared to master.
Actually, there are no existing tests that build an extrainfo document. Instead, the existing tests mock the relevant functions, replacing them with static descriptor strings.
These static descriptor strings are also out of date. I opened #29521 to address this technical debt, because I don't have time to do it in Sponsor V.
comment:7 follow-up: 8 Changed 10 months ago by
Actual Points: | 0.1 → 2 |
---|---|
Status: | needs_revision → needs_review |
Branch
I tried to merge the changes from 0.3.3 forward to master, but the merge was really complicated.
So I opened a new pull request https://github.com/torproject/tor/pull/709
It contains the:
- merged changes from #29017
- based on 0.3.3, to keep the same commit as #29017
- rebased refactor from #29017
- because the merge was complex
- rebased commits from this ticket ( https://github.com/torproject/tor/pull/639 )
- because the merge was complex
- new unit tests for #29017 and #29018
Tests
The code passes make test-network-all
with ExtraInfoStatistics 1
and ExtraInfoStatistics 0
set in chutney's common.i.
I discovered some bugs on my local machine while writing this code.
I don't know if they'll affect CI:
- #29529 util/map_anon_nofork test fails on macOS
- #29527 Division by zero: undefined behaviour in circuitpadding/circuitpadding_sample_distribution test
- #29530 Some address/get_if_addrs* tests fail when the network is unreachable
TODO
I think there might be some memory leaks in the new unit tests, I'm not sure if I caught them all. I'll check in the morning once CI has finished.
I think the coverage should have increased a bit compared with #29017 and master. But I'll need to wait for CI to make sure.
comment:8 Changed 10 months ago by
Replying to teor:
I think there might be some memory leaks in the new unit tests, I'm not sure if I caught them all. I'll check in the morning once CI has finished.
I found and fixup'd one memory leak. Hopefully it's the last one. (I need to install a newer version of clang to get good leak detection locally.)
I think the coverage should have increased a bit compared with #29017 and master. But I'll need to wait for CI to make sure.
Coveralls stopped working about 20 hours ago. Maybe it's back now?
comment:9 Changed 10 months ago by
Keywords: | 040-deferred-20190220 added |
---|---|
Milestone: | Tor: 0.4.0.x-final → Tor: unspecified |
Deferring 51 tickets from 0.4.0.x-final. Tagging them with 040-deferred-20190220 for visibility. These are the tickets that did not get 040-must, 040-can, or tor-ci.
comment:10 Changed 10 months ago by
Keywords: | privcount added |
---|---|
Milestone: | Tor: unspecified → Tor: 0.4.1.x-final |
This task is on the 0.4.1 roadmap as part of Sponsor V.
comment:11 Changed 10 months ago by
The coveralls bot is back:
https://github.com/torproject/tor/pull/709#issuecomment-465408842
These unit tests increase the overall coverage by 0.2%.
I looked at the uncovered lines: many of them are error cases or mocking. Some of them are from other pull requests.
I decided not to unit test the new router_build_fresh_unsigned_routerinfo()
function: it requires too much state. (See #29521.)
comment:12 follow-up: 13 Changed 10 months ago by
Status: | needs_review → merge_ready |
---|
This seems okay for 0.4.0. I'm not 100% sure on backporting any farther, but I wouldn't object much.
comment:13 Changed 10 months ago by
Status: | merge_ready → needs_review |
---|
Replying to nickm:
This seems okay for 0.4.0. I'm not 100% sure on backporting any farther, but I wouldn't object much.
Oops, I meant to say that on another ticket.
comment:14 follow-up: 15 Changed 9 months ago by
Keywords: | dgoulet-merge added |
---|---|
Status: | needs_review → merge_ready |
LGTM. This can go in after #29017 has been merged.
comment:15 Changed 9 months ago by
comment:16 follow-up: 18 Changed 9 months ago by
I'm acking 709, but only once 683 is merged as a part of #29017.
comment:17 Changed 9 months ago by
Keywords: | network-team-roadmap-2019-Q1Q2 added |
---|
comment:18 Changed 9 months ago by
comment:19 Changed 9 months ago by
Keywords: | asn-merge added |
---|
Oh right, asn has to merge this ticket.
comment:21 Changed 9 months ago by
asn, please don't merge until next week, nickm is doing an alpha.
comment:22 Changed 9 months ago by
Keywords: | merge-after-0403-alpha added |
---|
Tag these tickets for merge after 0.4.0.3-alpha, to make searches easier.
comment:23 Changed 9 months ago by
This ticket is a large refactor, it's not going to be backported to 0.4.0.
comment:24 Changed 9 months ago by
comment:25 Changed 9 months ago by
Resolution: | → fixed |
---|---|
Status: | merge_ready → closed |
Pushed in master. comment:23 indicates that no 040 backport is needed.
Closing this.
See my pull request https://github.com/torproject/tor/pull/639
Notes:
GeoIP file hashes might not be sensitive, but the other statistics are reasonably sensitive, particularly for private bridges.
ServerTransportPlugin lines aren't strictly statistics, but they are still potentially sensitive.
We turn off statistics in the extra-info file if we ever fail to parse the file. So we should really include all optional chunks under the write_stats_to_extrainfo check.