Opened 6 weeks ago

Last modified 9 hours ago

#29017 needs_review defect

PaddingStatistics should be disabled when ExtraInfoStatistics is 0

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: fast-fix, 035-backport, 034-backport, 033-backport, postfreeze-ok
Cc: Actual Points: 1.3
Parent ID: Points: 0.2
Reviewer: nickm Sponsor: SponsorV-can

Description

The man page entry for PaddingStatistics says that it is disabled when ExtraInfoStatistics is 0. But that isn't the case: the statistics are published regardless of ExtraInfoStatistics.

As far as I can tell, PaddingStatistics are collected on bridges, but the man page says "Relays only."

Child Tickets

Change History (11)

comment:1 Changed 6 weeks ago by teor

Actual Points: 0.2
Status: assignedneeds_review

comment:2 Changed 6 weeks ago by teor

Sponsor: SponsorV-can

Improving the reliability and usability of statistics can fall under Sponsor V.

comment:3 Changed 6 weeks ago by dgoulet

Reviewer: nickm

comment:4 Changed 6 weeks ago by nickm

Status: needs_reviewmerge_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 Changed 6 weeks ago by teor

Actual Points: 0.21.1
Status: merge_readyneeds_review

I tried to start testing router_build_fresh_descriptor(), but it was a huge function that looked really hard to test. So I refactored it into smaller functions.

Now I've done the refactor, I can test extrainfo_dump_to_string() using just router_build_fresh_unsigned_routerinfo() and router_build_fresh_unsigned_extrainfo(). But router_build_fresh_unsigned_routerinfo() is also large, so I might re-use some of the existing fake test routerinfos.

In the interim, feel free to review my work in progress refactor in https://github.com/torproject/tor/pull/638

I don't expect that we'll backport this refactor at all: it ended up being much larger than I expected. So maybe we'll just have the tests on master.

comment:6 Changed 6 weeks ago by teor

I also opened #29053 to follow-up some other issues that make this code hard to test. But I doubt we'll have time for it in Sponsor V.

comment:7 Changed 5 weeks ago by nickm

Keywords: postfreeze-ok added

Mark some tickets as postfreeze-ok, to indicate that I think they are okay to accept in 0.4.0 post-freeze. Does not indicate that they are all necessary to do postfreeze.

comment:8 Changed 4 weeks ago by nickm

I like the refactoring here; it's looking good.

I agree that it's fine (and wise) to have the tests and refactoring only on master, and backport the original commit only.

comment:9 Changed 7 days ago by nickm

Status: needs_reviewneeds_revision

Putting this into needs_revision since I don't know if Teor considers the branch done. Teor, when you're finished, please put it into needs_review and I'll have a last look?

comment:10 Changed 42 hours ago by teor

Actual Points: 1.11.3
Status: needs_revisionmerge_ready

I force-pushed to this pull request, so it only contains the backport to 0.3.3:

I want to deal with the refactor and the extra unit tests along with the changes in #29018. They're more complicated, and they won't be backported.

comment:11 Changed 9 hours ago by teor

Status: merge_readyneeds_review

Oops, this should have gone in needs_review.

Note: See TracTickets for help on using tickets.