Opened 8 months ago

Closed 4 months ago

Last modified 3 months ago

#28567 closed defect (implemented)

Report relays that sbws wants to test, but the test doesn't work

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, sbws-11x-final-removed-20190312, sbws-110-proposed, changes-version-minor
Cc: Actual Points:
Parent ID: #28547 Points: 1
Reviewer: nickm Sponsor:

Description

We should count:

  • the number of times sbws tries to test a relay
  • the number of times the test works and fails

If there is a high number of failures, we should also count:

  • the number of times that pair selection works and fails
  • the number of times that connecting works and fails
    • if there are a lot of these, count the hop that fails
  • the number of times that data transfer works and fails

Child Tickets

Change History (19)

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

Milestone: sbws 1.0.4sbws 1.1

Milestone renamed

comment:3 Changed 8 months ago by teor

Milestone: sbws 1.1sbws: 1.1.x

Milestone renamed

comment:4 Changed 8 months ago by teor

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

Milestone renamed

comment:5 Changed 5 months ago by juga

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
  • i covered all the possible errors in measure_relay and also store any error in callback error, so now any measurement attempt could be calculated as the sum of all measurements for a given relay
  • i created additional bandwidth lines KeyValues to report the reasons why the relays fail to be measured

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

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

comment:6 Changed 5 months ago by juga

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

comment:7 in reply to:  5 Changed 5 months ago by juga

Replying to juga:

  • i covered all the possible errors in measure_relay and also store any error in callback error, so now any measurement attempt could be calculated as the sum of all measurements for a given relay

I thought it was not a good idea and actually it'd be better to don't have a callback error. Created new 2 commits explaining that.

comment:8 Changed 4 months ago by juga

Status: assignedneeds_review

comment:9 Changed 4 months ago by asn

Reviewer: teor

comment:10 Changed 4 months ago by asn

Reviewer: teorasn

comment:11 Changed 4 months ago by juga

Keywords: sbws-11x-final-removed-20190312 sbws-110-proposed changes-version-minor added
Milestone: sbws: 1.1.x-finalsbws: 1.1.0

Move tickets that imply a minor version change to 1.1.0.

comment:12 Changed 4 months ago by asn

Reviewer: asnnickm

comment:13 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

I think this is okay, but PLEASE document all functions, classes, fields, errors, etc. Right now I can only check that the code looks at least sort of reasonable -- not that it does what you want it to do. Documentation would let me know what you want the code to do, so I can check that it does that.

comment:14 Changed 4 months ago by juga

Status: needs_revisionneeds_review

I added documentation. Let me know if it can be understood now.

comment:15 Changed 4 months ago by juga

i added one more commit (not fixup) explaining that the counters are not reset when the scanner stops.
I'm unable to decide whether is better like this or to reset them.
The measurement results are always accumulated.

comment:16 Changed 4 months ago by nickm

Status: needs_reviewmerge_ready

Thanks, juga! This is much better now. LGTM; I'd say go ahead and merge.

I don't have a strong opinion about whether to reset the counters on restart.

comment:17 Changed 4 months ago by juga

Resolution: implemented
Status: merge_readyclosed

Thanks, Merged.

comment:18 Changed 4 months ago by teor

If we count all the times a relay has ever failed, that's ok.
If we count all the times the relay failed in the last 5 days, that's also ok.

We just need to document the counters in the bandwidth-file-spec, I opened #29775 so we remember to do the documentation.

comment:19 Changed 3 months ago by teor

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

Move sbws 1.1.0 tickets into 1.1.x-final, to match Tor's milestone scheme.

Note: See TracTickets for help on using tickets.