[metrics-bugs] #27050 [Metrics/Onionoo]: Reverse DNS lookups are still slow

[Tor Bug Tracker & Wiki]

#27050: Reverse DNS lookups are still slow
-----------------------------+--------------------------------
 Reporter:  irl              |          Owner:  irl
     Type:  defect           |         Status:  needs_revision
 Priority:  Medium           |      Milestone:
Component:  Metrics/Onionoo  |        Version:
 Severity:  Normal           |     Resolution:
 Keywords:                   |  Actual Points:
Parent ID:                   |         Points:
 Reviewer:  karsten          |        Sponsor:
-----------------------------+--------------------------------
Changes (by karsten):

 * status:  needs_review => needs_revision


Comment:

 The code looks correct. I have a few questions to the list of changes
 though:

 > * SortedSets are used in place of Lists to ensure deterministic ordering
 of looked up names

 The second part, that looked up names are now ordered determistically,
 sounds like something we should mention in the change log. Whether that is
 done with SortedSet vs. List is too much detail for the change log, but
 the fact that the ordering is now deterministic is worth mentioning.

 > * The NodeStatus serialization is extended to include verified and
 unverified host names

 What exactly is the user-visible effect of this change? Should it go into
 the change log, too?

 > * The existing host name field in NodeStatus serializations is removed
 and a placeholder inserted

 No need to include this in the change log.

 > * The last reverse DNS lookup time is now only updated on successful
 lookups

 This sounds potentially user-visible, too. Change log entry?

 > * The host name field is removed from summary and details documents

 The "summary document" part here is potentially confusing, because we
 didn't include the host name in summary documents we're giving out. The
 only reason for having the field in `SummaryDocument` is that we're using
 it for the node index. I think it's fine to keep this comment in the
 commit message, but it should probably not go into the change log.

 The "details document" part should go into the change log, though.

 > * Tests are updated to use SortedSets in place of Lists

 No need to include this in the change log.

 Do you mind adding these change log entries in another commit?

 I'll start a local test run now. Assuming that it succeeds, should we put
 out 6.2-1.17.0 tomorrow?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27050#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online