Opened 3 weeks ago

Last modified 5 hours ago

#33617 needs_review enhancement

Add a BandwidthStatistics option and consensus parameter

Reported by: teor Owned by: MrSquanchee
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop313, ipv6, outreachy-ipv6, network-team-roadmap-2020Q1
Cc: metrics-team Actual Points:
Parent ID: #33052 Points: 1
Reviewer: teor Sponsor: Sponsor55-can

Description

Stage 1:

We propose adding a new BandwidthStatistics torrc option, which activates reporting of all the bandwidth statistics. At the moment, all statistics are controlled by ExtraInfoStatistics, but we propose using the new BandwidthStatistics option specifically for the bandwidth statistics.

The default value of this option should be 1. (The existing bandwidth statistics are reported by default.)

The new option should have a manual page entry, and test_parseconf.sh tests in src/test/conf_examples.

Stage 2:

Add a BandwidthStatistics consensus paramter.

Change the default value of the BandwidthStatistics option to "auto". If the value is "auto", use the value of the consensus parameter. If the consensus parameter is not set, use "1".

Update the manual page to describe the new consensus parameter.

See the proposal:
https://gitweb.torproject.org/torspec.git/tree/proposals/313-relay-ipv6-stats.txt#n298

Child Tickets

TicketTypeStatusOwnerSummary
#33812taskneeds_reviewMrSquancheeAdd unit tests for bandwidth statistics functions

Change History (28)

comment:1 Changed 3 weeks ago by teor

Keywords: outreachy-ipv6 added

comment:2 Changed 3 weeks ago by MrSquanchee

Owner: set to MrSquanchee
Status: newassigned

Hii teor,
Thanks for the ticket on tasks for summer-of-code.
Assigning myself.
Hope I would be valuable here :)

comment:3 Changed 2 weeks ago by MrSquanchee

Hii teor,
I completed stage 1 partially.
You can check my branch at https://github.com/MrSquanchee/tor/commits/ticket%2333617 .
Just added the BandwidthStatistics option and made it activate reporting of bandwidth statistics.
Will add the tests in the next commit.
And I don't have any idea on how to add a consensus parameter, any hint will do./
And please do tell me if I went wrong in some way.

comment:4 Changed 2 weeks ago by teor

Status: assignedneeds_revision

Hi, it's a bit hard to read and review this branch.

So I made a pull request here:
https://github.com/torproject/tor/pull/1808

Please submit code for review as a pull request, even if it isn't ready yet.

I made some specific comments on the pull request. Here are some general comments:

comment:5 Changed 2 weeks ago by teor

Please make any changes by adding new commits to the MrSquanchee:ticket#33617 branch. (The pull request will update automatically, and run CI.)

comment:6 in reply to:  4 Changed 2 weeks ago by MrSquanchee

Status: needs_revisionneeds_review

Replying to teor:

So I made a pull request here:
https://github.com/torproject/tor/pull/1808

Thanks :)

  • this code looks good, but how do you know it works?

How can I test it ? Do I have to run a relay ?

Modified the files pointed. Correct me if there is any problem with the man entry Only used by servers.

Resolved your comments on github.
You can check it on https://github.com/torproject/tor/pull/1808

comment:7 in reply to:  description ; Changed 2 weeks ago by MrSquanchee

Stage 2:

Add a BandwidthStatistics consensus paramter.

How to add a consensus parameter. Please cite me to some useful resources.
Edited :- I have looked in some PRs about consensus parameter. and these are added in the directory servers and checked periodically in relays and bridges.
I think I have to make a function to initialize BandwisdthStatistics whenever new consensus arrives.
Ok got that !!!
I want to know where should I create that function ??
Help Required !!!

Last edited 2 weeks ago by MrSquanchee (previous) (diff)

comment:8 Changed 2 weeks ago by teor

Here's a good example of a function that checks an option and a consensus parameter:
https://github.com/torproject/tor/blob/44f92e8e4278403b9e633668f8be70f197b6e8db/src/core/or/dos.c#L154

You shouldn't need to worry about when the consensus arrives. networkstatus_get_param() handles that for you.

The tricky part will be turning statistics on and off based on the parameter. I suggest you collect the statistics if:

  • the BandwidthStatistics option is on (1), or
  • the BandwidthStatistics option is auto (-1).

And then you only put the statistics in the extra-info if:

  • the BandwidthStatistics option is on (1), or
  • the BandwidthStatistics option is auto (-1), and the consensus parameter is on (1).

So it looks like you'll need two new functions, one for collecting the statistics, and another for submitting them.

comment:9 Changed 2 weeks ago by MrSquanchee

Status: needs_reviewassigned

Hii teor,
Thanks for your suggestion,
I was really stuck with this consensus parameter :)

comment:10 Changed 2 weeks ago by teor

I found these examples by searching the tor github for "consensus parameter".

If you tell us what you've already looked at (with links), then we won't suggest the same things. And we have a better idea of what you're missing.

comment:11 in reply to:  7 ; Changed 2 weeks ago by MrSquanchee

Replying to MrSquanchee:

I want to know where should I create that function ??
Help Required !!!

I wrote the function.
But where should I place it ??

comment:12 in reply to:  11 Changed 2 weeks ago by MrSquanchee

Replying to MrSquanchee:

Replying to MrSquanchee:

I want to know where should I create that function ??
Help Required !!!

I wrote the function.
But where should I place it ??

Never mind I figured it out :)

Updating the PR soon !!

comment:13 Changed 13 days ago by MrSquanchee

Status: assignedneeds_review

I added the changes,
Hope you like them,
Suggestions are most welcomed,
[PR]https://github.com/torproject/tor/pull/1808 ,

Thanks,
MrSquanchee

comment:14 Changed 13 days ago by teor

Thanks, I left comments on the pull request.

Please also write test_parseconf.sh tests for BandwidthStatistics 1 and auto.

I'm still not sure how we will test this change. Maybe see if there are existing tests for any of the bandwidth functions?
(I remember writing some. Maybe they never got finished and merged.)

comment:15 Changed 13 days ago by teor

Status: needs_reviewneeds_revision

comment:16 Changed 12 days ago by MrSquanchee

I think, I can use this file to test the added changes.
What do you say ??

Also I added the changes requested.
And I think when BandwidthStatistics is explicitly set to 1, it should not check the consensus as cited here (indirectly).

comment:17 Changed 12 days ago by MrSquanchee

Status: needs_revisionneeds_review

Finding ways to test :)

comment:18 in reply to:  16 ; Changed 12 days ago by teor

Replying to MrSquanchee:

I think, I can use this file to test the added changes.
What do you say ??

Sounds like a great idea!

Also I added the changes requested.
And I think when BandwidthStatistics is explicitly set to 1, it should not check the consensus as cited here (indirectly).

Thanks! We need to answer those questions in the comments and manual page.

Writing comments and manuals can be hard.
Can you try one more time?
Try to use similar options as a template.

If you're still having trouble, I can help rewrite them. But I might not have time this week.

comment:19 Changed 12 days ago by dgoulet

Reviewer: teor
Status: needs_reviewneeds_revision

comment:20 Changed 11 days ago by gaba

Keywords: network-team-roadmap-2020Q1 added

Add all the tickets from sponsor 55 that are done and being worked on to the keyword #network-team-roadmap-2020Q1 so I can look at them in the wiki page...

comment:21 in reply to:  18 ; Changed 4 days ago by teor

Replying to teor:

Replying to MrSquanchee:

I think, I can use this file to test the added changes.
What do you say ??

Sounds like a great idea!

Also I added the changes requested.
And I think when BandwidthStatistics is explicitly set to 1, it should not check the consensus as cited here (indirectly).

Thanks! We need to answer those questions in the comments and manual page.

Writing comments and manuals can be hard.
Can you try one more time?
Try to use similar options as a template.

If you're still having trouble, I can help rewrite them. But I might not have time this week.

Hi MrSquanchee, just wondering how you are going with these tests and documentation?

comment:22 in reply to:  21 Changed 3 days ago by MrSquanchee

Replying to teor:

Hi MrSquanchee, just wondering how you are going with these tests and documentation?

I have added the changes you suggested, yesterday only. You can see my commit
here https://github.com/torproject/tor/pull/1808/commits/32778dc6a09eaeca3d03e4261a2dbb9b669eb6fc.

tbh, I was a little distracted, I am adding the tests today.

Thanks,

Suraj Upadhyay (MrSquanchee).

comment:23 Changed 3 days ago by teor

Thanks, please set this ticket to needs_review when you have made all the changes.

comment:24 Changed 3 days ago by MrSquanchee

Status: needs_revisionneeds_review

Made reviewer changes.
I will add the tests soon.

comment:25 Changed 3 days ago by MrSquanchee

Hii reviewer,
I added tests for get_bandwidth_stats_param() (one of the functions,
I added).
And the PRs all green now (for some reason the CI succeeded but it shows
still pending).
https://github.com/torproject/tor/pull/1808
Now, I am adding tests for rep_hist_bw_stats_write() (the second function
I added).

Suggest me if I should continue making test for the second function or not ??
Because it's a lot similar to other functions for other statistics.

comment:26 Changed 3 days ago by teor

Every function should have unit tests,

We write tests to make sure that every function works now, and to make sure we don't break it in future. It doesn't matter how similar they are. (But sometimes similar functions can be tested using similar tests.)

comment:27 Changed 7 hours ago by MrSquanchee

Hii teor,
I just started writing test for rep_hist_bw_stats_write(),
and realized that before testing rep_hist_bw_stats_write()
we have to test rep_get_bandwidth_lines() and rep_hist_fill_bandwidth_history().

I should say that we need a new ticket ( #33812 ) to write tests for bandwidth code,
and only than this ticket can be closed :)

comment:28 Changed 5 hours ago by teor

Great! Thanks.

Note: See TracTickets for help on using tickets.