Opened 6 months ago

Closed 6 months ago

#30217 closed enhancement (fixed)

Extend CollecTor filesystem protocol for bandwidth files

Reported by: irl Owned by: irl
Priority: Medium Milestone:
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: tor-bwauth, tor-dirauth, metrics-roadmap-2019-q2
Cc: tom, teor, metrics-team, starlight@…, juga Actual Points:
Parent ID: #21378 Points:
Reviewer: karsten Sponsor:

Description

We should be careful which timestamps we are using, and how we are identifying the bandwidth scanners.

Child Tickets

Change History (12)

comment:1 Changed 6 months ago by teor

Cc: juga added

Replying to #21378 comment 32:

#30217 is an open question. We need to work out how to store these in a way that identifies which scanner collected the data, and the timestamp, and have these things be unique too. There are multiple timestamps in the file format,

In format version 1.0.0 (Torflow), the unnamed latest bandwidth Timestamp is not unique: multiple files can be created with the same "newest result".

In format versions 1.1.0 and later, the file_created timestamp should be reasonably unique: files should be created every 30-60 minutes, and each file should have a different timestamp.

If you need a unique identifier, you should use the bandwidth file digest. If the authority is running 0.4.0.4-rc or later, there will be a bandwidth-file-digest in the vote:
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n2154

and not currently any identifier of the bandwidth scanner.

We thought about adding a scanner name. And now I can't remember why we didn't. Maybe juga knows?

comment:2 in reply to:  1 Changed 6 months ago by juga

Replying to teor:

We thought about adding a scanner name. And now I can't remember why we didn't. Maybe juga knows?

I think i didn't included just because i don't remember talking about that.
I realized that some identifier would be useful in #29355.
So, there are 2 options to identify the scanner:

  • 1. Include the nickname. The scanner does not run when some options have not been edited, but it's not the case of the nickname. I could change the configuration parser to force editing a nickname. It's human readable. Since we would only have ~10 scanners (unless we start having more than 1 scanner per dirauth), should be unique.
  • 2. Include the UUID (#29355). It gets created automatically the first time and it's unique but not human readable.

Which one would we prefer?, both?.

comment:3 Changed 6 months ago by karsten

I didn't give this as many thoughts as you all did, but as I see it, we would want to include the following information in the bandwidth file name as archived by CollecTor:

  1. Timestamp: The timestamp (first line in the document) is not guaranteed to be unique between bandwidth generators and apparently not even for the same bandwidth generator. Still, including the timestamp is useful and also required for things like monthly tarballs.
  2. Identifier: Having an identifier of the bandwidth generator would certainly be useful. But it sounds like we don't have one for existing bandwidth files, which we want to archive anyway. I think we might have to find a bandwidth file name that doesn't include a bandwidth generator identifier, and whoever wants to only look at bandwidth files produced by certain bandwidth generators will have to use grep.
  3. Digest: If we include a digest, we'll come up with a unique file name, which is what we want. Ideally, we'd use the same digest that is also included in votes, though we might have to encode it as hex rather than base64 to avoid all kinds of trouble with / and + in file names and URLs and upper- and lower-case in case-insensitive file systems.

Putting everything together, how about this: 2019-04-21-09-00-00-bandwidth-file-42D07217935283D42CC559A61E7F788E3A92F0E2CB54BE58B5B83E25563C55A2 ?

comment:4 Changed 6 months ago by teor

Works for me.

If you wanted to use a name, you could use these names in order:

  1. the scanner nickname, or
  2. the authority name, or
  3. "unnamed"

comment:5 Changed 6 months ago by irl

Reviewer: irl
Status: newneeds_review

comment:6 Changed 6 months ago by irl

Owner: changed from metrics-team to irl
Reviewer: irlkarsten
Status: needs_reviewaccepted

I'm turning comment:3 into a PROTOCOL diff.

comment:7 Changed 6 months ago by irl

Status: acceptedneeds_review

Please review this commit:

https://gitweb.torproject.org/user/irl/collector.git/commit/?h=task/30217&id=19ae66a1314f9b1b4fa5d8019908971169a06599

The only change here is preferring to use the "file_created" timestamp instead of the "timestamp" when it is available.

comment:8 Changed 6 months ago by irl

Ah, and I've used "bandwidth" in place of "bandwidth-file" which is just less typing. I don't think this is ambiguous, but we can use the full "bandwidth-file" if you feel strongly.

comment:9 in reply to:  7 Changed 6 months ago by karsten

Replying to irl:

Please review this commit:

https://gitweb.torproject.org/user/irl/collector.git/commit/?h=task/30217&id=19ae66a1314f9b1b4fa5d8019908971169a06599

Will take a look after the lunch break. Quick question before that:

The only change here is preferring to use the "file_created" timestamp instead of the "timestamp" when it is available.

I can see how this is easier to comprehend for bandwidth files created by sbws. But when using a mix of files created by torflow and sbws, this might be confusing to users. I don't feel as strongly about this, but I want to point out that this is a possible source of confusion. What do you think?

Using "bandwidth" instead of "bandwidth-file" is fine.

I'll review the diff after my break.

comment:10 Changed 6 months ago by irl

If a bandwidth scanner's measurement threads crash but it's still writing out a bandwidth file, we'd be getting a load of files all with the same timestamp for a while. If we use the file_created timestamp then this won't happen. We still fall back to the timestamp field if there is no file_created field so we can still archive the older files.

I don't want to be in a situation where a bandwidth scanner has been broken for a few days and we're putting those files into last month's tarball if we can avoid it.

comment:11 Changed 6 months ago by karsten

Okay, makes sense. Patch looks good, merged. Is there anything else remaining?

comment:12 Changed 6 months ago by irl

Resolution: fixed
Status: needs_reviewclosed

I think the implementation is now all in #30218.

Note: See TracTickets for help on using tickets.