Opened 10 months ago

Closed 6 months ago

#24494 closed defect (fixed)

Specification says nickname is optional in documents but it's always there

Reported by: irl Owned by: karsten
Priority: Medium Milestone: Onionoo 1.11.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: iwakeh Sponsor:

Description

Example:

https://onionoo.torproject.org/details?search=Unnamed&limit=4&fields=nickname

Specification says:

Relay nickname consisting of 1–19 alphanumerical characters. Omitted if the relay nickname is "Unnamed".

I think I would prefer to fix the specification than to fix the Onionoo behaviour, as it's useful to have only one code path in Relay Search instead of more code paths depending on whether or not the field is present.

Child Tickets

Change History (13)

comment:1 Changed 10 months ago by karsten

Agreed, we should take out that optimization. It seemed like a good idea to save some bandwidth at the beginning, but we added so many more fields that this one doesn't really matter anymore. Reducing complexity seems more important. Let's do it.

comment:2 Changed 7 months ago by karsten

Owner: changed from metrics-team to karsten
Status: newaccepted

I'll look into this.

comment:3 Changed 7 months ago by irl

Can I safely assume that nickname will always be present for Relay Search? This means I get to strip out a conditional and make the results table a little faster.

comment:4 Changed 7 months ago by karsten

Don't assume it just yet. But I'll take that question as a request and will try to make it so.

comment:5 Changed 7 months ago by karsten

Status: acceptedneeds_review

Please review commit 81c72b0 in my Onionoo task-24494 branch and commit d2178c3 in my metrics-web task-24494 branch. Yet untested, except for unit tests.

irl, if these reviews go through, you can safely assume that nickname will always be present for Relay Search. In fact, you can already do that for details documents, just not for summary documents. But feel free to wait for this change and new protocol version 5.1 to be deployed.

comment:6 Changed 7 months ago by irl

The patch for metrics-web looks to be what I was expecting.

Relay Search does not actually use the summary documents anymore, and instead only makes one fetch for the details document (instead of the summary document followed by n fetches of details documents).

comment:7 Changed 7 months ago by karsten

Reviewer: iwakeh

Okay, that leaves the Onionoo branch for review, which is probably iwakeh's part. Setting them as reviewer.

comment:8 Changed 7 months ago by irl

Reviewer: iwakeh

Relay Search is now assuming that nicknames are always present.

comment:9 Changed 7 months ago by irl

Reviewer: iwakeh

comment:10 Changed 7 months ago by iwakeh

Status: needs_reviewmerge_ready

Onionoo changes look fine and pass all tests and checks. The protocol description makes sense.
Always good to simplify things when possible.

comment:11 Changed 7 months ago by karsten

Great! Pushed to master together with another commit that bumps the protocol version to 5.1. Not closing yet, because we'll still need to merge the metrics-web patch when we put out the next Onionoo release. Leaving as merge_ready.

comment:12 Changed 6 months ago by karsten

Milestone: Onionoo 1.11.0

This will be part of the next milestone.

comment:13 Changed 6 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Moving the specification part to #25476 and closing this ticket.

Note: See TracTickets for help on using tickets.