Opened 7 months ago

Last modified 5 months ago

#33617 needs_revision 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: extra-review, prop313, ipv6, outreachy-ipv6, network-team-roadmap-2020Q1
Cc: metrics-team, nickm, ahf, dgoulet Actual Points:
Parent ID: #33052 Points: 1
Reviewer: asn 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 (39)

comment:1 Changed 7 months ago by teor

Keywords: outreachy-ipv6 added

comment:2 Changed 7 months 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 7 months 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 7 months 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 7 months 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 7 months 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 7 months 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 7 months ago by MrSquanchee (previous) (diff)

comment:8 Changed 7 months 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 7 months ago by MrSquanchee

Status: needs_reviewassigned

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

comment:10 Changed 7 months 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 7 months 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 7 months 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 7 months 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 7 months 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 7 months ago by teor

Status: needs_reviewneeds_revision

comment:16 Changed 7 months 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 7 months ago by MrSquanchee

Status: needs_revisionneeds_review

Finding ways to test :)

comment:18 in reply to:  16 ; Changed 7 months 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 7 months ago by dgoulet

Reviewer: teor
Status: needs_reviewneeds_revision

comment:20 Changed 7 months 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 7 months 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 7 months 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 7 months ago by teor

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

comment:24 Changed 7 months ago by MrSquanchee

Status: needs_revisionneeds_review

Made reviewer changes.
I will add the tests soon.

comment:25 Changed 7 months 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 7 months 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 months 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 7 months ago by teor

Great! Thanks.

comment:29 Changed 7 months ago by teor

Status: needs_reviewneeds_revision

comment:30 Changed 6 months ago by MrSquanchee

Status: needs_revisionneeds_review

Hii teor,

I think you should review this PR https://github.com/torproject/tor/pull/1808 now
for the documentation, comments and other changes so far.

Also I looked at the similar functions as rep_hist_bw_stats_write() and they seem
to have no tests, i.e. the functions which write stats to disk and rightly so
because for that we have to mock a lot of lower level functions.

Suggest me again if I should write a test for this function and if so whether
in test_bwmgt.c or test_stats.c ??

Thanks :-)

comment:31 Changed 6 months ago by MrSquanchee

Hii teor,

It has been two weeks and, I think now is the prefect time to review it before
I forgot how did I do this :)

comment:32 in reply to:  31 ; Changed 6 months ago by teor

Cc: nickm ahf dgoulet added
Reviewer: teor

Replying to MrSquanchee:

Hii teor,

It has been two weeks and, I think now is the prefect time to review it before
I forgot how did I do this :)

We've been a bit busy for the past few weeks. I was also waiting for you to write more tests.

I did a review. We need a few tweaks to the documentation, then this part is done.

Please open another pull request, once you've made these changes. And the next reviewer can work with you on more tests.

(I might not have time to review this ticket in future. So I'm leaving it for another reviewer to finish.)

comment:33 in reply to:  32 ; Changed 6 months ago by MrSquanchee

Replying to teor:

We've been a bit busy for the past few weeks. I was also waiting for you to write more tests.

I thought we were done with the tests. Can you please tell me what functions do you want to have
tests for ??

I did a review. We need a few tweaks to the documentation, then this part is done.
Please open another pull request, once you've made these changes. And the next reviewer can work with you on more tests.

I am committing the changes you have requested and will open a new PR with a cleaner git history.
Thanks :)

(I might not have time to review this ticket in future. So I'm leaving it for another reviewer to finish.)

No problem. We all have some unfinished rescheduled tasks nowadays.

comment:34 Changed 6 months ago by MrSquanchee

PR https://github.com/torproject/tor/pull/1872
Rebased with a cleaner git history.

comment:35 in reply to:  33 Changed 6 months ago by teor

Keywords: extra-review added

Replying to MrSquanchee:

Replying to teor:

We've been a bit busy for the past few weeks. I was also waiting for you to write more tests.

I thought we were done with the tests. Can you please tell me what functions do you want to have
tests for ??

We should test all the functions that have been modified. Sometimes we decide to do unit tests later, and open another ticket. Other times we ask for them as part of the PR, so we are sure that the new code works.

We don't have good integration tests for this code, so unit tests are important.

I'll leave it to the next reviewer to decide if we need more tests.

comment:36 Changed 5 months ago by asn

Reviewer: asn

comment:37 Changed 5 months ago by asn

Status: needs_reviewneeds_revision

Sorry for the late review here.

The code looks good, but I'm wondering that there is no unittest for this functionality. In particularly, I'm refering to rep_hist_bw_stats_write() and its position in wep_hist_bw_stats_write(). I think we need a unittest that tests the latter function and makes sure that rep_hist_bw_stats_write() will be called (or not be called) based on the consensus/torrc parameter, and that when it's called it does the right thing.

Please let me know if you need help designing this unittest.

comment:38 Changed 5 months ago by MrSquanchee

Hii, asn.

  1. rep_hist_bw_stats_write() gets called from write_stats_file_callback in mainloop.c. This function handles disk write for all the stats produced. So, do you want exhaustive unit-tests for write_stats_file_callback, which would test for all the stats ?? or should I write unit tests for write_stats_file_callback pertaining only to the bandwidth statistics ??
  2. Also, rep_hist_bw_stats_write() writes the stats to the disk, so does many other stat functions, but I haven't yet seen a unit test that tests for a directory and contents of a file maybe for some reasons I don't know or maybe I am wrong and you can point me to an appropriate test. Would you like to explain how I can write such a test ??

comment:39 in reply to:  38 Changed 5 months ago by asn

Replying to MrSquanchee:

Hii, asn.

Hello MrSquanchee,

thanks for showing interest to write these tests. Let me try to help out.

  1. rep_hist_bw_stats_write() gets called from write_stats_file_callback in mainloop.c. This function handles disk write for all the stats produced. So, do you want exhaustive unit-tests for write_stats_file_callback, which would test for all the stats ?? or should I write unit tests for write_stats_file_callback pertaining only to the bandwidth statistics ??

I would like unittests for write_stats_file_callback pertaining only to the bandwidth statistics.

In particular, you could call that function with a special options argument that only activates the bandwidth statistics. As part of this, I would like to see that the separation between BandwidthStatistics and ExtraInfoStatistics is clear. That is, how does each of those two options influence the other? Given that this is one of the original purposes of this ticket (i.e. not having ExtraInfoStatistics control everything) I would like the test to be able to test that things work as expected.

  1. Also, rep_hist_bw_stats_write() writes the stats to the disk, so does many other stat functions, but I haven't yet seen a unit test that tests for a directory and contents of a file maybe for some reasons I don't know or maybe I am wrong and you can point me to an appropriate test. Would you like to explain how I can write such a test ??

There are a bunch of ways to achieve this. One way would be to actually do the file writes; see how test_config_write_to_data_subdir() and test_config_check_or_create_data_subdir() do this. IMO this would be the best and most robust way to approach this (also probably simpler).

The other way, would be to mock the file-writing functions, so that their behavior does not happen when they are getting tested. As an example, of a test that does mocking check test_config_resolve_my_address().

Best of luck in this adventure!

Note: See TracTickets for help on using tickets.