Opened 12 months ago

Closed 3 months ago

#30726 closed defect (fixed)

Missing relay keys in bandwidth file spec

Reported by: teor Owned by:
Priority: High Milestone: sbws: 1.1.x-final
Component: Core Tor/Tor Version:
Severity: Major Keywords: sbws-spec, sbws-roadmap
Cc: juga, gk Actual Points:
Parent ID: #33121 Points: 6
Reviewer: ahf, gk Sponsor:

Description

Not in spec or examples:

  • relay_in_recent_consensus_count
  • consensus_bandwidth
  • consensus_bandwidth_is_unmeasured
  • desc_bw_avg
  • desc_bw_bur
  • desc_bw_obs_last
  • desc_bw_obs_mean

Not in spec:

  • relay_recent_measurement_attempt_count
  • relay_recent_priority_list_count

We need a specification for these keys so we know what they mean.

Child Tickets

Change History (19)

comment:1 Changed 10 months ago by gaba

Keywords: sbws-roadmap-september added
Points: 6

comment:2 Changed 4 months ago by gaba

Keywords: sbws-roadmap added; sbws-roadmap-september removed

Changing keyword of roadmapped open sbws tickets to a general sbws-roadmap one.

comment:3 Changed 4 months ago by gaba

Parent ID: #33121

The goal is to deploy sbws in all bw authorities. We need to fix critical bugs to do this.

comment:4 Changed 3 months ago by juga

Reviewer: ahf, gk
Status: newneeds_review

putting this ticket to needs review because i've some questions.

i realized that there are keys in the spec like desc_bw_average, that was never named like that in sbws, it was named desc_bw_avg, that's why the last one is missing in the spec, it was a mistake when writing the spec.

Should we leave them as they are for version 1.2 and add the correct names in version 1.5? ,and remove desc_bw_average from the sbws examples in the Annexes?

and the keys that were missing, should we also say that we add them in version 1.5 even if they were in sbws time ago?

comment:5 Changed 3 months ago by juga

Component: Core Tor/sbwsCore Tor/Tor

This is about the spec, so component is Tor

comment:6 Changed 3 months ago by gk

Cc: gk added

comment:7 in reply to:  4 ; Changed 3 months ago by gk

Replying to juga:

putting this ticket to needs review because i've some questions.

i realized that there are keys in the spec like desc_bw_average, that was never named like that in sbws, it was named desc_bw_avg, that's why the last one is missing in the spec, it was a mistake when writing the spec.

Should we leave them as they are for version 1.2 and add the correct names in version 1.5? ,and remove desc_bw_average from the sbws examples in the Annexes?

and the keys that were missing, should we also say that we add them in version 1.5 even if they were in sbws time ago?

So, all of those examples look to me like spec bugs: the right thing happened in the code, but the spec does not reflect that and thus needs to get updated. If that's the case we should just fix the spec without the need for bumping the version. That goes both for missing as well as wrongly spelled keys. The missing ones, too, should get added to the spec with the first version in which they appeared (as all the other ones that we did not forget).

Re: the examples. Yes, if we change desc_bw_average to desc_bw_avg (and do not say the bug is actually desc_bw_avg in the code which should be desc_bw_average as it is written in the spec) then we should fix up the examples as well in the Annexes (I don't know where those are but even without looking at them in detail it makes sense to fix the examples :)).

comment:8 Changed 3 months ago by juga

Status: needs_reviewneeds_revision

Let's change this to revision until i create a PR

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

Replying to gk:

So, all of those examples look to me like spec bugs: the right thing happened in the code, but the spec does not reflect that and thus needs to get updated.

Yes.
And i think the source of true for the version and when the KeyValues appeared should be the bandwidth files published by the bwauths, because with sbws happens that:

  • we change KeyValues
  • we change the specification version in sbws
  • we make a new sbws release (and usually bwauths have run only releases, except longclaw a couple of times)
  • the new version spec and the KeyValues start to appear in the bandwidth files
  • we change some more KeyValues (if there's no bwauth running a non-released version, they're still publishing the last specification version
  • new specification change, new sbws release, bwauths update sbws version (and therefore KeyValues and specification version)...

So i should check the collector to be sure about the which keys were in which versions

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

comment:10 in reply to:  9 ; Changed 3 months ago by gk

Replying to juga:

Replying to gk:

So, all of those examples look to me like spec bugs: the right thing happened in the code, but the spec does not reflect that and thus needs to get updated.

Yes.
And i think the source of true for the version and when the KeyValues appeared should be the bandwidth files published by the bwauths, because with sbws happens that:

  • we change KeyValues
  • we change the specification version in sbws
  • we make a new sbws release (and usually bwauths have run only releases, except longclaw a couple of times)
  • the new version spec and the KeyValues start to appear in the bandwidth files
  • we change some more KeyValues (if there's no bwauth running a non-released version, they're still publishing the last specification version
  • new specification change, new sbws release, bwauths update sbws version (and therefore KeyValues and specification version)...

So i should check the collector to be sure about the which keys were in which versions

Sounds good.

comment:11 in reply to:  10 ; Changed 3 months ago by juga

Status: needs_revisionneeds_review

Replying to gk:

So i should check the collector to be sure about the which keys were in which versions

Sounds good.

I checked and all the collected bandwidth files started with the specification version 1.4.0, so in versions earlier than that it does not really matter the KeyValues are not correct.

Still, i fixed them in their respective versions, including the version 1.3.0 that was never generated by sbws (it should have been other ticket, but the change is small).

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

What i should still check is whether we need to open tickets to metrics and stem because of the change of version in the spec. I don't think so because there were no bandwidth files published earlier than 1.4.0.

comment:12 in reply to:  11 ; Changed 3 months ago by gk

Status: needs_reviewneeds_revision

Replying to juga:

Replying to gk:

So i should check the collector to be sure about the which keys were in which versions

Sounds good.

I checked and all the collected bandwidth files started with the specification version 1.4.0, so in versions earlier than that it does not really matter the KeyValues are not correct.

Still, i fixed them in their respective versions, including the version 1.3.0 that was never generated by sbws (it should have been other ticket, but the change is small).

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

Looks mostly good to me. I have some comments/nits mainly for 0b6e97ca37b60fbfbd628306ec4b0551b34142cc:

4ec93f2d8b2add1da598d927785ed7d8e60f0ec3: looks good

0b6e97ca37b60fbfbd628306ec4b0551b34142cc:

  • desc_bw_bur, consensus_bandwidth, and consensus_bandwidth_is_unmeasured are all in v1.0.3 released which is using SPEC_VERSION 1.2.0. Thus, those KeyValues are already in that spec version available
  • in the commit message s/They appeared in in/They appeared in/
  • s/the consensus bandwdith/the consensus bandwidth/
  • In the new "in the last data_period days" parts the default value for "data_period days" is not mentioned anymore as it is in previous KeyValue explanations. Maybe add a hint to the default one, here too
  • In relay_recent_priority_list_count there is no "in the last data_period days" but there should be, shouldn't it (at least that's what v3bwfile.py is indicating)?
  • "Add more KeyValues": I think it might make sense to use "Adds more KeyValues" in the sense that a particular spec version *adds* those things. I am a bit confused though, as the explanations to previous spec versions alternate between "Add" and "Adds" without some pattern. You could fix that part up in a follow-up commit if you think we should do that.

fb36a2bae2bc0aa1f0dfbfa68fbb903a5ee6d193: looks good (modulo the previously mentioned "Adds"/"Add" inconsistency; could be part of a follow-up fix as well)

What i should still check is whether we need to open tickets to metrics and stem because of the change of version in the spec. I don't think so because there were no bandwidth files published earlier than 1.4.0.

I agree that it should not be needed from my understanding but, yes, could be worth double-checking, though.

comment:13 Changed 3 months ago by juga

Status: needs_revisionneeds_review

Thanks, think i addressed what you comment, except the commit message cause i pushed fixups.
I should remember to change that when squashing.

comment:14 in reply to:  12 Changed 3 months ago by juga

Replying to gk:

What i should still check is whether we need to open tickets to metrics and stem because of the change of version in the spec. I don't think so because there were no bandwidth files published earlier than 1.4.0.

I agree that it should not be needed from my understanding but, yes, could be worth double-checking, though.

stem parsers only the headers and the not matching version is only a comment (https://gitweb.torproject.org/stem.git/tree/stem/descriptor/bandwidth_file.py#n127).

comment:15 in reply to:  13 ; Changed 3 months ago by juga

Replying to juga:

Thanks, think i addressed what you comment, except the commit message cause i pushed fixups.
I should remember to change that when squashing.

I squashed commits in https://github.com/torproject/torspec/pull/111. I had to rewrite one commit message since it wasn't matching anymore.

comment:16 in reply to:  15 ; Changed 3 months ago by gk

Status: needs_reviewneeds_revision

Replying to juga:

Replying to juga:

Thanks, think i addressed what you comment, except the commit message cause i pushed fixups.
I should remember to change that when squashing.

I squashed commits in https://github.com/torproject/torspec/pull/111. I had to rewrite one commit message since it wasn't matching anymore.

Thanks. Just one final nit: s/days.(unless/days. (Unless/

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

Status: needs_revisionneeds_review

Replying to gk:

I squashed commits in https://github.com/torproject/torspec/pull/111. I had to rewrite one commit message since it wasn't matching anymore.

Thanks. Just one final nit: s/days.(unless/days. (Unless/

Done, squashed now is PR 113

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

comment:18 in reply to:  17 Changed 3 months ago by gk

Status: needs_reviewmerge_ready

Replying to juga:

Replying to gk:

I squashed commits in https://github.com/torproject/torspec/pull/111. I had to rewrite one commit message since it wasn't matching anymore.

Thanks. Just one final nit: s/days.(unless/days. (Unless/

Done, squashed now is PR 113

Looks good now, thanks!

comment:19 Changed 3 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.