Opened 5 weeks ago

Closed 2 weeks ago

#26121 closed defect (fixed)

BUILDTIMEOUT_SET totals are still off

Reported by: mikeperry Owned by: mikeperry
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 033-backport 034-backport tor-client timeouts performance
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:

Description

While testing and examining onion service timeout rates and trying to verify them from a controller script, I realized that our timeout rates are still off in the BUILDTIMEOUT_SET event.

The reason for the discrepancy is that we're double-counting circuits that transition into MEASUREMENT_EXPIRED. Every circuit that transitions into MEASUREMENT_EXPIRED is first counted as a timeout in circuit_build_times_mark_circ_as_measurement_only(). Of those that do not complete within the measurement window, we again count as a "closed" circuit in circuit_build_times_count_close(). If a measurement circuit succeeds, we do *not* count it as a success (see circuit_build_times_handle_completed_hop()).

This means that the total_circuits value in cbt_control_event_buildtimeout_set() should be timeouts+succeeded, and not timeouts+succeeded+closed. (Again, counting closed in the total there double-counts "closed" MEASUREMENT circuits, which were also counted as timeout circuits earlier).

Since this is just a control port stats change, it would be nice to get it into 0.3.4 (and maybe 0.3.3) so it is easier to get more accurate data about how the circuit build timeout is interacting with vanguards there.

Child Tickets

Change History (7)

comment:1 Changed 5 weeks ago by mikeperry

Status: assignedneeds_review

https://github.com/torproject/tor/pull/115 aka mikeperry/bug26121-033 (off of maint-0.3.3).

Last edited 5 weeks ago by mikeperry (previous) (diff)

comment:2 Changed 4 weeks ago by asn

Reviewer: asn

comment:3 Changed 4 weeks ago by asn

Status: needs_reviewmerge_ready

Logic and code seems sound, but please check my branch bug26121-033 for a trivial comment fix over Mike's branch.

Would be nice to somehow be able to test these assumptions and logic.

Moving this to merge_ready because this bug is probs not the right place to write such tests.

Last edited 4 weeks ago by asn (previous) (diff)

comment:4 Changed 4 weeks ago by asn

Keywords: 033-backport 034-backport tor-client timeouts performance added

Marking for possible backports.

comment:5 Changed 3 weeks ago by nickm

asn, I can't find this branch in any of your repositories that I have links to. Did you push it? Did I forget a repo?

Can we open another tickets to get some test coverage here?

comment:6 in reply to:  5 Changed 3 weeks ago by asn

Replying to nickm:

asn, I can't find this branch in any of your repositories that I have links to. Did you push it? Did I forget a repo?

Oops. Pushed it in my github repo.

Can we open another tickets to get some test coverage here?

Opened #26262 for this.

comment:7 Changed 2 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Great; squashed and merged to 0.3.3 and forward!

Note: See TracTickets for help on using tickets.