Opened 9 months ago

Closed 8 months ago

#28085 closed defect (implemented)

Update key/values in the bandwidth file spec

Reported by: juga Owned by: juga
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec
Cc: pastly, juga, teor Actual Points:
Parent ID: #27107 Points:
Reviewer: dgoulet, teor Sponsor:

Description

To scale as Torflow, it was added relay_observed_bandwidth and relay_average_bandwidth.

I think we should also come up with some policy to add/remove key/values.
Even if all the ones we have so far seem useful, we are not using it.
For instance, do we have tickets of cases where these key/values were needed/requested?.

Child Tickets

TicketStatusOwnerSummaryComponent
#28197closedjugaRename some bandwidth file keys in the codeCore Tor/sbws
#28365closedjugaInclude statistics headers all bandwidth filesCore Tor/sbws
#28366closedjugaChange specification version in bandwidth file headersCore Tor/sbws
#28379closedjugaUse 5 "=" characters as terminator character in the bandwidth filesCore Tor/sbws

Change History (21)

comment:1 in reply to:  description Changed 9 months ago by teor

Replying to juga:

To scale as Torflow, it was added relay_observed_bandwidth and relay_average_bandwidth.

We document the keys and values in the spec, so we know what they mean.
So we need to add these keys to the spec.

I think we should also come up with some policy to add/remove key/values.

I answered in a new ticket #28099, because a policy is a different issue to a spec update.

comment:2 Changed 9 months ago by juga

These are the keys i introduced because they were useful to create graphs and/or scale using different methods. I think we still need to try different method and we should keep them:

bw_median, bw_mean, desc_avg_bw, desc_obs_bw_last, desc_obs_bw_mean

What maybe we want to change is the name.

These are the keys introduced in #28062 :
num_relays_to_include, num_target_relays, num_net_relays, perc_relays_to_include, perc_relays_to_include_target

Are we ok with this names?

Edit: fix syntax

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

comment:3 Changed 9 months ago by juga

See #28197 regarding other names.

comment:4 Changed 9 months ago by juga

Should those keys be optional or mandatory?. If they are mandatory, should we increment the verion of the spec to 1.2.0?

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

I answered your name question in https://trac.torproject.org/projects/tor/ticket/28197#comment:3

Replying to juga:

Should those keys be optional or mandatory?.

None of these keys are required for the network to operate. So they should be optional.

If they are mandatory, should we increment the verion of the spec to 1.2.0?

New fields are a new feature, so we should increment the spec version to 1.2.0. It doesn't matter if they are optional or mandatory.

comment:6 Changed 9 months ago by juga

Status: assignedneeds_review

comment:7 Changed 9 months ago by juga

Component: Core Tor/sbwsCore Tor/Tor

specs sub-component is Tor

comment:8 Changed 9 months ago by dgoulet

Reviewer: teor

comment:9 Changed 9 months ago by dgoulet

Reviewer: teorcatalyst

comment:10 Changed 9 months ago by dgoulet

Keywords: tor-spec added
Reviewer: catalystdgoulet

comment:11 Changed 9 months ago by dgoulet

Status: needs_reviewneeds_revision

teor posted a review on the PR.

comment:12 in reply to:  11 Changed 9 months ago by teor

Replying to dgoulet:

teor posted a review on the PR.

I didn't do a full review: I was leaving that for dgoulet.
I just noticed a few typos.

comment:13 Changed 9 months ago by juga

Status: needs_revisionneeds_review

Replied to comments in the PR. Change to needs review since i think it's better to continue with the review before doing those changes

comment:14 Changed 8 months ago by dgoulet

Status: needs_reviewmerge_ready

I see nothing wrong with this spec file. I unfortunately can't comment on the content much since I have close to no knowledge of whole problem space. But the file seems fine.

comment:15 Changed 8 months ago by teor

Status: merge_readyneeds_revision

I did a review of the content.

I think it will be easier to understand if we revise some of the definitions.

comment:16 Changed 8 months ago by juga

Status: needs_revisionneeds_review

New commits and comments

comment:17 Changed 8 months ago by teor

Reviewer: dgouletdgoulet, teor
Status: needs_reviewneeds_revision

I added some more comments on the spec.

It seems like the bandwidth file examples are out of sync with the spec.
Let's commit examples that match the spec. Otherwise, metrics will get confused.

comment:18 Changed 8 months ago by juga

Status: needs_revisionneeds_review

comment:19 Changed 8 months ago by teor

Status: needs_reviewmerge_ready

Looks good to me, let's squash and merge

comment:20 Changed 8 months ago by teor

Milestone: sbws 1.0 (MVP must)Tor: 0.4.0.x-final

Putting this in a Tor milestone so it gets merged to torspec.

comment:21 Changed 8 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.