Opened 8 months ago

Closed 5 months ago

#28566 closed defect (implemented)

Report relays that sbws sees in the consensus, but never chooses to test

Reported by: teor Owned by: juga
Priority: Medium Milestone: sbws: 1.1.x-final
Component: Core Tor/sbws Version:
Severity: Normal Keywords: tor-bwauth, sbws-1.0-must-moved-20181128
Cc: Actual Points:
Parent ID: #28547 Points: 1
Reviewer: Sponsor:

Description (last modified by teor)

We should count:

  • the number of consensuses that sbws has seen a relay in the consensus
  • the number of times sbws chooses to test a relay
  • the number of times sbws chooses to exclude a relay (for example, because it's an authority)

If there are a large number of exclusions, we should split the exclusions into subcategories.

Child Tickets

Change History (15)

comment:1 Changed 8 months ago by teor

Description: modified (diff)

comment:2 Changed 8 months ago by teor

Description: modified (diff)

comment:3 in reply to:  description ; Changed 8 months ago by juga

Replying to teor:
..

  • the number of times sbws chooses to exclude a relay (for example, because it's an authority)

Authorities are excluded from the possible list of relays to measure (https://gitweb.torproject.org/sbws.git/tree/sbws/lib/relayprioritizer.py#n50).
Do you think that it would be needed to report the authorities seen and a keyvalue that says it's an authority?

..

comment:4 in reply to:  3 Changed 8 months ago by teor

Replying to juga:

Replying to teor:
..

  • the number of times sbws chooses to exclude a relay (for example, because it's an authority)

Authorities are excluded from the possible list of relays to measure (https://gitweb.torproject.org/sbws.git/tree/sbws/lib/relayprioritizer.py#n50).
Do you think that it would be needed to report the authorities seen and a keyvalue that says it's an authority?

..

Authorities are flagged in the consensus. So I think we can just document the authority exclusion.

comment:5 Changed 8 months ago by teor

Keywords: sbws-1.0-must-moved-20181128 added
Milestone: sbws 1.0 (MVP must)sbws 1.0.4

Moving all sbws 1.0 must planning and feature tickets to 1.0.4.

comment:6 Changed 8 months ago by teor

Milestone: sbws 1.0.4sbws 1.1

Milestone renamed

comment:7 Changed 8 months ago by teor

Milestone: sbws 1.1sbws: 1.1.x

Milestone renamed

comment:8 Changed 8 months ago by teor

Milestone: sbws: 1.1.xsbws: 1.1.x-final

Milestone renamed

comment:9 Changed 5 months ago by juga

Cc: pastly juga removed
Owner: set to juga
Points: 1
Status: newassigned

Following the proposed KeyValues in https://pad.riseup.net/p/sbws-exclude-reasons-keep:

  • any KeyValue that is based on the last bandwidth file adds more code complexity, because it would mean to read and parse the last bandwidth file
  • fetch consensus every hour (instead of every 5min) so that we can assume every consensus fetched is new
  • store the number of times a relay is in the consensus
  • created a KeyValue to report the number of time a reported relay was in the consensus
  • a header KeyValue with the total number of consensus could be calculated as the the maximum number of consensuses in a bandwidth line (maybe missing some?)
  • it can't be know the relays that were not in any consensus, since they won't be reported
  • it'd be possible to report the number relays that were not in any consensus

Branch: https://github.com/juga0/sbws/tree/ticket28566

Not creating PR, not changing to merge_ready since master is not in 1.1.X version yet.

comment:10 in reply to:  9 Changed 5 months ago by juga

Replying to juga:

  • a header KeyValue with the total number of consensus could be calculated as the the maximum number of consensuses in a bandwidth line (maybe missing some?)

Store the number of times that the consensus is fetched in branch https://github.com/juga0/sbws/tree/ticket28566_01

comment:11 Changed 5 months ago by juga

Status: assignedneeds_review

comment:13 Changed 5 months ago by nickm

Status: needs_reviewneeds_revision

Hi! I've made some small suggestions throughout the code. It's a little tricky to follow, but I think documenting functions will fix that. Also, I'd strongly suggest documenting what _exactly_ each new field should contain. Once that's done you should feel free to merge.

comment:14 Changed 5 months ago by juga

Status: needs_revisionmerge_ready

comment:15 Changed 5 months ago by juga

Resolution: implemented
Status: merge_readyclosed

Merged.

Note: See TracTickets for help on using tickets.