Opened 10 days ago

Last modified 9 hours ago

#26121 merge_ready defect

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 (5)

comment:1 Changed 10 days 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 10 days ago by mikeperry (previous) (diff)

comment:2 Changed 6 days ago by asn

Reviewer: asn

comment:3 Changed 6 days 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 6 days ago by asn (previous) (diff)

comment:4 Changed 6 days ago by asn

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

Marking for possible backports.

comment:5 Changed 9 hours 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?

Note: See TracTickets for help on using tickets.