Opened 3 months ago

Closed 2 days ago

Last modified 2 days ago

#29017 closed defect (fixed)

PaddingStatistics should be disabled when ExtraInfoStatistics is 0

Reported by: teor Owned by: teor
Priority: Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: fast-fix, 035-backport, 034-backport, postfreeze-ok, 040-can, dgoulet-merge, 033-backport-unreached, network-team-roadmap-2019-Q1Q2, consider-backport-after-0404
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 (23)

comment:1 Changed 3 months ago by teor

Actual Points: 0.2
Status: assignedneeds_review

comment:2 Changed 3 months ago by teor

Sponsor: SponsorV-can

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

comment:3 Changed 3 months ago by dgoulet

Reviewer: nickm

comment:4 Changed 3 months 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 3 months 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 3 months 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 3 months 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 3 months 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 2 months 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 2 months 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 2 months ago by teor

Status: merge_readyneeds_review

Oops, this should have gone in needs_review.

comment:12 Changed 2 months ago by dgoulet

Keywords: 040-can added
Priority: MediumLow

Bug triage of 0.4.0 tickets. These are now in the "CAN" section. Lower priority than "040-must".

comment:13 Changed 2 months ago by nickm

Status: needs_reviewmerge_ready

This seems okay for 0.4.0. I'm not 100% sure on backporting any farther, but I wouldn't object much.

comment:14 in reply to:  13 Changed 8 weeks ago by teor

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.

My feeling is that getting it in 0.3.5 would be nice. Putting it in 0.3.4 seems less important, and 0.3.3 is not supported in 3 days' time.

I'll check it against our backport policy when I do my next backport run.

comment:15 Changed 8 weeks ago by nickm

Keywords: dgoulet-merge added

comment:16 Changed 8 weeks ago by dgoulet

Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final

Merged to 040 and master. Moving this to 035 Milestone for backport decision.

comment:17 Changed 7 weeks ago by teor

Keywords: 033-backport-unreached added; 033-backport removed

These merge_ready tickets did not get backported to 0.3.3, and 0.3.3 is now unsupported.

comment:18 Changed 6 weeks ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:19 Changed 6 weeks ago by teor

Keywords: consider-backport-after-0404-alpha added

These tickets are low-risk changes that were marked for backport after the release of 0.4.0.2-alpha. I want them to get testing in at least one alpha release, so I'll look at them after 0.4.0.4-alpha is released.

comment:20 Changed 4 days ago by teor

Keywords: consider-backport-after-0404 added; consider-backport-after-0404-alpha removed

Drop the -alpha from backport tags

comment:21 Changed 2 days ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final

Merged to 0.3.4 and merged forward.

Merged #23790, #29665, #29017, #27199, #29144, #13221, #28698.

comment:22 Changed 2 days ago by teor

Resolution: fixed
Status: merge_readyclosed

comment:23 in reply to:  description Changed 2 days ago by arma

Replying to teor:

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

For the historical record: bridge relays are a kind of relay, so "relays only" includes bridges.

(If we want to change this terminology, that's fine, but I bet there will be plenty more places, from way back, where it's used this way.)

Note: See TracTickets for help on using tickets.