Opened 7 months ago

Closed 7 months ago

#29754 closed defect (fixed)

Include new monitoring KeyValues in the bandwidth-file-spec

Reported by: juga Owned by: teor
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec, bandwidth-file-spec, tor-bwauth
Cc: Actual Points: 2
Parent ID: #28547 Points: 1
Reviewer: juga Sponsor:

Description

After #28547 is implemented, there will be new KeyValues that need to be documented in the bandwidth file spec.

Child Tickets

Change History (21)

comment:1 Changed 7 months ago by juga

Parent ID: #28547

Assigning parent #28547 so we don't forget about it.

comment:2 Changed 7 months ago by juga

We might want to do #29755 first.

comment:3 Changed 7 months ago by juga

Owner: set to juga
Status: newassigned

will probably work on this this week

comment:4 Changed 7 months ago by juga

Status: assignedneeds_review

I think i might need to rephrase almost all, because i'm not sure it can be understood, but in case review gives some tips.

https://github.com/torproject/torspec/pull/67

comment:5 Changed 7 months ago by teor

Reviewer: teor

I'm sorry, but this branch conflicts with #29813.

I'm going to merge #29813, because it's merge ready.
Then I'll push some changes to this branch, rebased on the new master.

After that, I'll have some questions for you about missing KeyValues.

comment:6 in reply to:  5 ; Changed 7 months ago by teor

Reviewer: teorteor, juga

Replying to teor:

I'm sorry, but this branch conflicts with #29813.

I'm going to merge #29813, because it's merge ready.
Then I'll push some changes to this branch, rebased on the new master.

It was easier to rebase #29813 on my changes for this ticket.
See https://github.com/torproject/torspec/pull/68

Please review my changes in that branch.

After that, I'll have some questions for you about missing KeyValues.

I opened #29854 for missing KeyValues in the relay line, and #29853 for missing relays in the bandwidth file when we're below MIN_REPORT. Let's talk about those issues in those tickets.

comment:7 in reply to:  6 ; Changed 7 months ago by juga

Replying to teor:
[...]

I opened #29854 for missing KeyValues in the relay line,

Commented there.

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

Replying to juga:

Replying to teor:
[...]

I opened #29854 for missing KeyValues in the relay line,

Commented there.

To don't forget, these're the KeyValues to add to the lines example: "relay_recent_measurement_attempt_count", "relay_recent_priority_list_count",
"relay_in_recent_consensus_count"

comment:9 Changed 7 months ago by juga

Status: needs_reviewneeds_revision

Thanks for all fixes and new keys!.
I did some initial review. Let me know if you're going to work on this in the next hours, otherwise i'd continue with it.
Let me also know if you're adding those 3 missing keys, otherwise i'll do it.

comment:10 Changed 7 months ago by juga

I created https://github.com/teor2345/torspec/pull/1 against your PR 68, fixing some of the things i commented in the review, but not all.
Not closing https://github.com/torproject/torspec/pull/68 because i'm not sure what you think about the other comments i made.

comment:11 Changed 7 months ago by juga

Status: needs_revisionneeds_review

Changing to review since we might have got lost on who's turn to do a review/revision is

comment:13 Changed 7 months ago by teor

Status: needs_reviewneeds_revision

I need to modify the spec for under_min_report based on:
https://trac.torproject.org/projects/tor/ticket/29853#comment:7

comment:14 Changed 7 months ago by teor

Owner: changed from juga to teor
Status: needs_revisionassigned

comment:15 Changed 7 months ago by teor

Status: assignedneeds_revision

comment:16 in reply to:  10 ; Changed 7 months ago by teor

Replying to juga:

I created https://github.com/teor2345/torspec/pull/1 against your PR 68, fixing some of the things i commented in the review, but not all.

I reviewed your pull request.
Most of the commits are good, but we can't wrap example bandwidth file lines.

Not closing https://github.com/torproject/torspec/pull/68 because i'm not sure what you think about the other comments i made.

I responded to your comments. I can fix the remaining issues.

comment:17 in reply to:  16 ; Changed 7 months ago by juga

Replying to teor:

Replying to juga:

I created https://github.com/teor2345/torspec/pull/1 against your PR 68, fixing some of the things i commented in the review, but not all.

I reviewed your pull request.
Most of the commits are good, but we can't wrap example bandwidth file lines.

ok, i did it cause i was getting lost myself in such long lines, but i've removed the new lines now.
If a parser uses the Values exactly as they're in the examples, the headers don't match with the lines. Maybe i could just put them in different subsections so that it is a bit more clear that they don't match?.
If i change the number in the header to match the lines, then it would not be that realistic.

Not closing https://github.com/torproject/torspec/pull/68 because i'm not sure what you think about the other comments i made.

I responded to your comments. I can fix the remaining issues.

ack

comment:18 in reply to:  17 Changed 7 months ago by teor

Replying to juga:

Replying to teor:

Replying to juga:

I created https://github.com/teor2345/torspec/pull/1 against your PR 68, fixing some of the things i commented in the review, but not all.

I reviewed your pull request.
Most of the commits are good, but we can't wrap example bandwidth file lines.

ok, i did it cause i was getting lost myself in such long lines, but i've removed the new lines now.
If a parser uses the Values exactly as they're in the examples, the headers don't match with the lines. Maybe i could just put them in different subsections so that it is a bit more clear that they don't match?.

We can't split the header and the relay lines into different subsections, because parsers need to be able to parse the whole file.

If i change the number in the header to match the lines, then it would not be that realistic.

Parsers should still work if the numbers in the header and relay lines do not match.

comment:19 Changed 7 months ago by teor

Actual Points: 2
Reviewer: teor, jugajuga
Status: needs_revisionneeds_review

Hi Juga,

I merged your changes into the pull request and added fixups on:
https://github.com/torproject/torspec/pull/68

The squashed version for merging is:
https://github.com/torproject/torspec/pull/71

comment:20 in reply to:  19 Changed 7 months ago by juga

Status: needs_reviewneeds_revision

Replying to teor:

Hi Juga,

I merged your changes into the pull request and added fixups on:
https://github.com/torproject/torspec/pull/68

The squashed version for merging is:
https://github.com/torproject/torspec/pull/71

LGTM, just a couple of suggestions, feel free change them or not and put this merge ready.

comment:21 Changed 7 months ago by teor

Resolution: fixed
Status: needs_revisionclosed

Ok, fixed and squashed in:
https://github.com/torproject/torspec/pull/74

Merged to master as fd9ccd5.

Note: See TracTickets for help on using tickets.