Opened 10 months ago

Closed 9 months ago

#27050 closed defect (fixed)

Reverse DNS lookups are still slow

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

Description

It would appear that only new relays are looked up. Any relay that already exists with a NodeStatus is not looked up. I believe this is due to incorrectly recording the last lookup time even where no lookup has actually occurred.

The lack of a reverse DNS record and the failure of a lookup (including running out of time before even getting to it) all count as success and a lookup time will be recorded. This prevents the lookup being attempted again for another 12 hours.

I hadn't spotted this when testing locally as I'd always set RDNS_LOOKUP_MAX_AGE_MILLIS to 0L to ensure that every relay was looked up every time.

This has happened this way as I had tried to maintain backwards-bug-compatibility with the old field that returned an IP address in cases where the lookup did not find an error but also did not find a domain. The implementation wasn't strict enough in its bug compatibility because it returned an IP address also in error conditions.

I wonder if it is worth spending time to fix this here, or instead removing the host_name field (which is optional in the spec, and we did deprecate on July 16th) and so not requiring the returning of IP addresses at all.

Child Tickets

Change History (8)

comment:1 Changed 10 months ago by irl

Cc: metrics-team added
Owner: changed from metrics-team to irl
Status: newaccepted

I am looking at this.

comment:2 Changed 9 months ago by irl

Please review the 2 commits in my task/27050 branch.

Checks and tests pass, and I have tested on a real status directory.

This change removes the "host_name" field from details documents and so should be deployed along with Onionoo protocol version 7.0.

If these changes are ok, set back to needs_revision anyway for me to write a changelog entry.

Last edited 9 months ago by irl (previous) (diff)

comment:3 Changed 9 months ago by irl

Status: acceptedneeds_review

comment:4 Changed 9 months ago by karsten

Reviewer: karsten

Will take a look tomorrow.

comment:5 Changed 9 months ago by karsten

Status: needs_reviewneeds_revision

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?

comment:6 in reply to:  5 ; Changed 9 months ago by irl

Replying to karsten:

  • 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?

No user-visible impact, but it changes the format of an internal document type. I also have a Python implementation of the NodeStatus document serialization for testing which required updating to interpret the new fields. This would not go in the changelog.

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

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

The last lookup time is not served in the documents. Maybe it should be? It's not a user-visible change unless they look closely. A missing reverse name is treated as a failure and tried again on the next update, a success is only tried again after 12 hours.

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

I'll do this shortly.

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

I wasn't sure if we should wait until 7.0 or not. This change doesn't violate protocol 6.2 as the host_name field is optional and I guess as it's mostly broken anyway it doesn't make much of a difference doing the update sooner.

comment:7 Changed 9 months ago by irl

Just pushed a commit with the changelog entry.

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

Milestone: Onionoo 1.17.0
Resolution: fixed
Status: needs_revisionclosed

Merged!

Replying to irl:

I wasn't sure if we should wait until 7.0 or not. This change doesn't violate protocol 6.2 as the host_name field is optional and I guess as it's mostly broken anyway it doesn't make much of a difference doing the update sooner.

No need to wait until 7.0. We deprecated the field on July 16, which is 1 month ago. And, as you say, it was optional. We can just remove it now.

I'll start preparing a release now. That will be in another ticket, though.

Closing. Thanks!

Note: See TracTickets for help on using tickets.