Opened 10 months ago

Closed 6 months ago

#28355 closed defect (fixed)

Disable measurement timing rules on sbws by default

Reported by: juga Owned by: juga
Priority: Medium Milestone: sbws: 1.1.x-final
Component: Core Tor/sbws Version: sbws: 1.0.2
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description (last modified by teor)

Currently sbws include less relays in its report, mostly due to https://trac.torproject.org/projects/tor/ticket/27338#comment:6.

While sbws is not changed to include more relays, maybe the notice message about
"Bandwidth authorities have a substantially different number of measured entries" should not be sent.

sbws should disable these restrictions, so it matches torflow.

Child Tickets

Change History (18)

comment:2 Changed 10 months ago by teor

I don't understand what the issue is here. You linked to a very large comment in a ticket that is closed as fixed.

If #27338 is fixed in sbws master, then we should ask the dirauths to upgrade.

If #27338 is not fixed in sbws master, and the difference in reported relays is significant, then we should fix it before sbws 1.0.

If the difference in reported relays is not significant, then we should lower the threshold for this alarm.

comment:3 Changed 10 months ago by teor

Status: assignedneeds_information

comment:4 in reply to:  2 ; Changed 10 months ago by juga

Replying to teor:

I don't understand what the issue is here. You linked to a very large comment in a ticket that is closed as fixed.

I meant the comment6, specifically:
<< If any of these things are true, do not put the relay in the bandwidth file:

there are less than 2 sbws measured bandwidths
all the sbws measured bandwidths are within 24 hours of each other
there are less than 2 descriptor observed bandwidths
all the descriptor observed bandwidths are within 24 hours of each other

If #27338 is fixed in sbws master, then we should ask the dirauths to upgrade.

If #27338 is not fixed in sbws master, and the difference in reported relays is significant, then we should fix it before sbws 1.0.

It is fixed in master and in 1.0, which has been already released.

If the difference in reported relays is not significant, then we should lower the threshold for this alarm.

Right now the threshold is 0.8 (https://gitweb.torproject.org/doctor.git/tree/consensus_health_checker.py#n807).

If we lower that to 0.7, there won't be alarm. Sounds this solution reasonable?

comment:5 in reply to:  4 ; Changed 10 months ago by teor

Replying to juga:

Replying to teor:

I don't understand what the issue is here. You linked to a very large comment in a ticket that is closed as fixed.

I meant the comment6, specifically:
<< If any of these things are true, do not put the relay in the bandwidth file:

there are less than 2 sbws measured bandwidths
all the sbws measured bandwidths are within 24 hours of each other
there are less than 2 descriptor observed bandwidths
all the descriptor observed bandwidths are within 24 hours of each other

Ok, so sbws filters out more relays than torflow?

If #27338 is fixed in sbws master, then we should ask the dirauths to upgrade.

If #27338 is not fixed in sbws master, and the difference in reported relays is significant, then we should fix it before sbws 1.0.

It is fixed in master and in 1.0, which has been already released.

If the difference in reported relays is not significant, then we should lower the threshold for this alarm.

Right now the threshold is 0.8 (https://gitweb.torproject.org/doctor.git/tree/consensus_health_checker.py#n807).

If we lower that to 0.7, there won't be alarm. Sounds this solution reasonable?

Why can't we modify sbws so it does what torflow does?
Is sbws more correct?
Is torflow including useless relays?

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

Replying to teor:

Replying to juga:

Replying to teor:

I don't understand what the issue is here. You linked to a very large comment in a ticket that is closed as fixed.

I meant the comment6, specifically:
<< If any of these things are true, do not put the relay in the bandwidth file:

there are less than 2 sbws measured bandwidths
all the sbws measured bandwidths are within 24 hours of each other
there are less than 2 descriptor observed bandwidths
all the descriptor observed bandwidths are within 24 hours of each other

Ok, so sbws filters out more relays than torflow?

yes

If #27338 is fixed in sbws master, then we should ask the dirauths to upgrade.

If #27338 is not fixed in sbws master, and the difference in reported relays is significant, then we should fix it before sbws 1.0.

It is fixed in master and in 1.0, which has been already released.

If the difference in reported relays is not significant, then we should lower the threshold for this alarm.

Right now the threshold is 0.8 (https://gitweb.torproject.org/doctor.git/tree/consensus_health_checker.py#n807).

If we lower that to 0.7, there won't be alarm. Sounds this solution reasonable?

Why can't we modify sbws so it does what torflow does?

i think you suggested to add those restrictions because torflow obtains the relays descriptors every hour while sbws only obtains a relay descriptor in the moment it does the measurement for that relay.

IIRC the restrictions can be disabled by cli options. I can run sbws without those. Should i?

Is sbws more correct?

correct in which sense?.

You have proposed different restrictions for sbws 1.1 [0] which i still don't know how they would affect the number of results.

Is torflow including useless relays?

useless in which sense?.

i think torflow is just including more relays because it doesn't check the restrictions we added.

[0] https://trac.torproject.org/projects/tor/query?milestone=sbws+1.1

comment:7 Changed 10 months ago by teor

Component: Core Tor/DocTorCore Tor/sbws
Description: modified (diff)
Milestone: sbws 1.0 (MVP must)
Summary: Do not notify about bwauths out of syncDisable measurement timing rules on sbws by default
Type: taskdefect

i think torflow is just including more relays because it doesn't check the restrictions we added.

We have a policy that sbws should match torflow, unless we have a very good reason. (Like a bad bug in torflow.)

Adding the restrictions to sbws in #27338 has caused a major bug: it does not match torflow.

Here's what I think we've tried so far:

  • fixing the sbws scheduler
  • disabling rtt

Are these fixes in longclaw's version of sbws?

If we can't get the restrictions to work, we should disable them by default, so sbws matches torflow.
Then we should open a ticket to review them in a later release.

comment:8 Changed 10 months ago by teor

Status: needs_informationnew
Version: sbws: 1.0.0

This is a bug on the following tickets in sbws 1.0.0:

  • Number of results comparison and number of results away from each other are incorrect (#28041)
  • Stop removing results that are not away from some other X secs (#28103)
  • Use secs-away when provided instead of data_period (#28105)
  • Disable measuring RTTs (#28159)

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

Replying to teor:

Are these fixes in longclaw's version of sbws?

This was the mistake: i temporally launched from an older branch to check something and forgot to launch it back from 1.0.0 branch.
What is more, there screen where it was running died. This mistake will be more difficult to happen with the Debian package.

Right now sbws is reporting around 6000 relays while torflow is reporting around 7000 (which is inside the 80% margin), so this should be reviewed again, but i think we can move it to 1.1.0 or later.

comment:10 Changed 9 months ago by teor

Milestone: sbws 1.0 (MVP must)sbws 1.1

comment:11 Changed 9 months ago by teor

Milestone: sbws 1.1sbws 1.2

Milestone renamed

comment:12 Changed 9 months ago by teor

Milestone: sbws 1.2sbws: 1.2.x

Milestone renamed

comment:13 Changed 9 months ago by teor

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

Milestone renamed

comment:14 Changed 9 months ago by teor

Milestone: sbws: 1.2.x-finalsbws: unspecified

Milestone renamed

comment:15 Changed 7 months ago by juga

Milestone: sbws: unspecifiedsbws: 1.0.x-final
Version: sbws: 1.0.0sbws: 1.0.2

Based on what we talked in https://trac.torproject.org/projects/tor/wiki/org/meetings/2019BrusselsNetworkTeam/Notes/SBWSRoadmap#Questions, there're these other ideas:

  • Run the scanner for at least 24h, then publish (all?) the results
  • If the measurement don't pass the filter, add a zero result?

comment:16 Changed 7 months ago by juga

Points: 0.3

comment:17 Changed 6 months ago by juga

Milestone: sbws: 1.0.x-finalsbws: 1.1.x-final
Points: 0.31

Minimum points is 1. We moved this to milestone 1.1.

comment:18 Changed 6 months ago by teor

Resolution: fixed
Status: newclosed

Some of this ticket is dealt with by the lower relay count ticket #29710.
I don't know if there is anything else left to do on this ticket, because we opened bugs for specific issues, and fixed them.

Let's new other tickets for any further work.

Note: See TracTickets for help on using tickets.