Opened 9 months ago

Closed 5 months ago

Last modified 4 months ago

#28565 closed defect (implemented)

Report excluded results in a relay's bandwidth line

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: juga Actual Points:
Parent ID: #28547 Points: 1
Reviewer: teor Sponsor:

Description

When sbws excludes some results for a relay, we should:

  • count how many results were excluded and how many were used
  • report the excluded and used result counts in the relay's bandwidth line

If the number of excluded results is high, we should split it into subcategories.

Child Tickets

Change History (26)

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

Milestone: sbws 1.0.4sbws 1.1

Milestone renamed

comment:3 Changed 9 months ago by teor

Milestone: sbws 1.1sbws: 1.1.x

Milestone renamed

comment:4 Changed 9 months ago by teor

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

Milestone renamed

comment:5 Changed 7 months ago by juga

Owner: set to juga
Points: 1
Status: newassigned

comment:6 Changed 7 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 created 4 additional header KeyValues to report the reasons why the relays were exclued, that would make the KeyValue recent_measurement_exclusion_count unneeded, since it can be deduced from the previous.
  • relay_recent_measurement_exclusion_count is 0 all the relays reported, so it'd mean that any relay that gets measurements excluded doesn't make it to be reported.

By the initial headers obtained in the Tor network is already explained (or mostly) that sbws is reporting ~1000 relays less than torflow.

Edit:

  • fix typo
  • comment about relay_recent_measurement_exclusion_count

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

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

Last edited 7 months ago by juga (previous) (diff)

comment:7 Changed 7 months ago by juga

Cc: pastly removed

Avoid non-desired noise

comment:8 Changed 5 months ago by juga

Status: assignedneeds_review

https://github.com/torproject/sbws/pull/348
Based on #28567 (which should be reviewed before)
To review only from 70531ba included.

comment:9 Changed 5 months ago by asn

Reviewer: teor

comment:10 Changed 5 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:11 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

I will do a much better review after this change has documentation:
https://trac.torproject.org/projects/tor/ticket/28567#comment:13

comment:12 in reply to:  11 ; Changed 5 months ago by juga

Replying to teor:

I will do a much better review after this change has documentation:
https://trac.torproject.org/projects/tor/ticket/28567#comment:13

do you mean this PR or the PR in #28567?.

comment:13 Changed 5 months ago by juga

Rebased to master again, no new documentation in this PR.

comment:14 in reply to:  8 ; Changed 5 months ago by teor

Replying to juga:

https://github.com/torproject/sbws/pull/348
Based on #28567 (which should be reviewed before)
To review only from 70531ba included.

I'm guessing that you merged #28567 and rebased on to it, so I should review all the commits.

comment:15 in reply to:  12 ; Changed 5 months ago by teor

Replying to juga:

Replying to teor:

I will do a much better review after this change has documentation:
https://trac.torproject.org/projects/tor/ticket/28567#comment:13

do you mean this PR or the PR in #28567?.

I wanted documentation for this pull request, similar to what nick wanted:

I think this is okay, but PLEASE document all new functions, 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.

Here are some specific things to fix:

  • You made the bandwidth file header count relay exclusions, not result exclusions. That is a good choice: anyone who wants to know result totals can just add all the relay-level results. But the key names, key documentation, and comments need to say what you are counting.
  • It's not clear to me what each key is meant to be counting. Please update the spec in #29775, or write comments that define each kind of failure and each key (or both).

comment:16 in reply to:  15 ; Changed 5 months ago by juga

Replying to teor:

Here are some specific things to fix:

  • You made the bandwidth file header count relay exclusions, not result exclusions. That is a good choice: anyone who wants to know result totals can just add all the relay-level results. But the key names, key documentation, and comments need to say what you are counting.
  • It's not clear to me what each key is meant to be counting. Please update the spec in #29775, or write comments that define each kind of failure and each key (or both).

I added fixups extending documentation.
TBH, i was minimizing documentation because i think we should work on #28684, which would change part of the documentation (and code).

comment:17 in reply to:  14 Changed 5 months ago by juga

Replying to teor:

I'm guessing that you merged #28567 and rebased on to it, so I should review all the commits.

Yes.

comment:18 Changed 5 months ago by juga

Status: needs_revisionneeds_review

comment:19 Changed 5 months ago by teor

I don't think it is a good idea to start #28684 when you only have 10 paid days left. It will be harder to find and fix bugs when you don't have as much time.

comment:20 in reply to:  16 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

Replying to juga:

Replying to teor:

Here are some specific things to fix:

  • You made the bandwidth file header count relay exclusions, not result exclusions. That is a good choice: anyone who wants to know result totals can just add all the relay-level results. But the key names, key documentation, and comments need to say what you are counting.
  • It's not clear to me what each key is meant to be counting. Please update the spec in #29775, or write comments that define each kind of failure and each key (or both).

I added fixups extending documentation.
TBH, i was minimizing documentation because i think we should work on #28684, which would change part of the documentation (and code).

Ok, I have done a review on the code.

comment:21 Changed 5 months ago by teor

I suggested some new names for the keys, that are easier to understand.
You'll need to update the key names everywhere in the code.

comment:22 Changed 5 months ago by juga

Status: needs_revisionneeds_review

ok, i added comments, changed some names and store the relay's key-values only when a measurement is excluded.

comment:23 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

I think you missed some of the changes you were going to do.
There are also some comments that are hard to understand, or duplicates.

Once you are happy with the name change and comments, please put this ticket into merge_ready.

comment:24 in reply to:  23 Changed 5 months ago by juga

Status: needs_revisionmerge_ready

Replying to teor:

I think you missed some of the changes you were going to do.

yes, added them.

There are also some comments that are hard to understand, or duplicates.

Changed and removed.

Once you are happy with the name change and comments, please put this ticket into merge_ready.

Done.

comment:25 Changed 5 months ago by juga

Resolution: implemented
Status: merge_readyclosed

merged

comment:26 Changed 4 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.