Report excluded results in a relay's bandwidth line
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.
Activity
changed milestone to %sbws: 1.1.x-final
added changes-version-minor component::core tor/sbws milestone::sbws: 1.1.x-final owner::juga parent::28547 points::1 priority::medium resolution::implemented reviewer::teor sbws-1.0-must-moved-20181128 sbws-110-proposed sbws-11x-final-removed-20190312 severity::normal status::closed tor-bwauth type::defect labels
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_countunneeded, since it can be deduced from the previous. -
relay_recent_measurement_exclusion_countis 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_countBranch: 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.
-
https://github.com/torproject/sbws/pull/348 Based on #28567 (moved) (which should be reviewed before) To review only from 70531ba included.
Trac:
Status: assigned to needs_review I will do a much better review after this change has documentation: https://trac.torproject.org/projects/tor/ticket/28567#comment:13
Trac:
Status: needs_review to needs_revision-
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 (moved)?.
Replying to juga:
https://github.com/torproject/sbws/pull/348 Based on #28567 (moved) (which should be reviewed before) To review only from 70531ba included.
I'm guessing that you merged #28567 (moved) and rebased on to it, so I should review all the commits.
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 (moved)?.
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 (moved), or write comments that define each kind of failure and each key (or both).
-
-
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 (moved), 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 (moved), which would change part of the documentation (and code).
-
-
Replying to teor:
I'm guessing that you merged #28567 (moved) and rebased on to it, so I should review all the commits. Yes.
I don't think it is a good idea to start #28684 (moved) 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.
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 (moved), 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 (moved), which would change part of the documentation (and code).
Ok, I have done a review on the code.
Trac:
Status: needs_review to needs_revision-
-
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.
Trac:
Status: needs_revision to merge_ready mentioned in issue #29568 (moved)
mentioned in issue tpo/network-health/sbws#29568 (closed)
