Opened 4 months ago

Closed 3 months ago

#29299 closed defect (fixed)

Include scanner country and Web server country in the bandwidth file header

Reported by: juga Owned by: juga
Priority: Medium Milestone: sbws: 1.1.x-final
Component: Core Tor/sbws Version: sbws: 1.0.2
Severity: Normal Keywords:
Cc: juga Actual Points: 1
Parent ID: Points: 1
Reviewer: catalyst Sponsor:

Description

This will allow us to diversify the scanners/Web servers and get a better idea on how the location affects the measurements.

The bandwidth file specification will need to get updated.

Child Tickets

Change History (17)

comment:1 Changed 4 months ago by juga

The scanner country can be added to the headers.
Currently, we are not reporting which destinations are used, not in the headers nor in the bandwidth lines, so we should:

  • report which destinations are used. In the headers or for each bandwidth line?
  • if we report the destinations in the headers, we can also report their countries. if we report the destination in each bandwidth line, the country of each destination can still be reported in the headers.

Also, when the destination is in a CDN, we could know from which country is replying by solving the IP address from the exit, but that would add more complexity.
I'm adding instead "00" as country code for a CDN, meaning that we don't know from where it's reached.

comment:2 Changed 4 months ago by juga

Owner: set to juga
Points: 0.11
Status: newassigned

Changing points to 1 since that's the minimum.
Assigning to myself.

comment:3 Changed 4 months ago by juga

Milestone: sbws: 1.0.x-finalsbws: 1.1.x-final

Moving to 1.1 milestone, since it'd change the bandwidth file.

comment:4 in reply to:  1 ; Changed 4 months ago by juga

Replying to juga:

The scanner country can be added to the headers.
Currently, we are not reporting which destinations are used, not in the headers nor in the bandwidth lines, so we should:

  • report which destinations are used. In the headers or for each bandwidth line?
  • if we report the destinations in the headers, we can also report their countries. if we report the destination in each bandwidth line, the country of each destination can still be reported in the headers.

Also, when the destination is in a CDN, we could know from which country is replying by solving the IP address from the exit, but that would add more complexity.
I'm adding instead "00" as country code for a CDN, meaning that we don't know from where it's reached.

I'd solve this as:

  • if there's only 1 destination, include the destination (and its country) in the headers
  • if all the destinations are in a CDN, include the destinations (and their countries) in the headers
  • if there're several destinations and some of them are not in a CDN, still doubt whether to include the destination in each bandwidth line, or to make it simpler, add it only to the headers.

comment:5 Changed 4 months ago by juga

Branch https://github.com/juga0/sbws/tree/ticket29299 add scanner and destination country in the configuration and scanner country in the bandwidth file. It's still missing adding the destination(s) country in the bandwidth file.

nickm thought is the more information, the better.

comment:6 in reply to:  4 Changed 4 months ago by juga

Replying to juga:

  • if there're several destinations and some of them are not in a CDN, still doubt whether to include the destination in each bandwidth line, or to make it simpler, add it only to the headers.

If a relay was measured different times with different destinations, it'd be needed to include all the destinations and associate every destination with every measurement to understand how the location affects, but that would increase code complexity.

So for now, i'd add the destinations only in the headers.

comment:7 Changed 3 months ago by juga

Status: assignedneeds_review

comment:8 Changed 3 months ago by dgoulet

Reviewer: catalyst

comment:9 Changed 3 months ago by juga

I added two fixups just to replace destinations_country to destinations_countries, since i think the later makes it clearer that there can be several.
I also replaced in the test 00, DE (with space) to 00,DE (without space), since they list of countries is generated without space.

comment:10 Changed 3 months ago by juga

Merged in a branch with current master and current needs_review tickets and tested a whole loop with the public Tor network

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

Status: needs_reviewneeds_revision

Replying to juga:

Merged in a branch with current master and current needs_review tickets and tested a whole loop with the public Tor network

The pull request claims the branch still has conflicts with master; could you please take a look?

comment:12 Changed 3 months ago by juga

Status: needs_revisionneeds_review

comment:13 in reply to:  12 Changed 3 months ago by catalyst

Status: needs_reviewneeds_revision

Replying to juga:

Squashed and rebased in https://github.com/torproject/sbws/pull/338

Thanks! It mostly looks good. I don't have the ability to easily test this, so my review is based on reading the changes. I made some minor comments on the pull request.

comment:14 Changed 3 months ago by juga

Status: needs_revisionneeds_review

Thanks, addressed your comments.

comment:15 in reply to:  14 Changed 3 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to juga:

Thanks, addressed your comments.

Thanks! It looks good now. I left one more comment on github about an extraneous minor format change, but please feel free to merge without further review after fixing that.

comment:16 Changed 3 months ago by juga

Actual Points: 1

I merged, then realized about that child.
Going to work on child.

comment:17 Changed 3 months ago by juga

Resolution: fixed
Status: merge_readyclosed

Child now in needs_review and un-parented.

Note: See TracTickets for help on using tickets.