Opened 6 years ago

Closed 3 days ago

#6946 closed enhancement (fixed)

Allow filtering relays by OS

Reported by: rransom Owned by: irl
Priority: Medium Milestone: Onionoo 1.15.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: nusenu@…, metrics-team Actual Points:
Parent ID: Points:
Reviewer: karsten Sponsor:

Description

It would be much easier to handle tickets like #4076 if there were an easy way to search for relays running on a particular operating system.

Child Tickets

Change History (20)

comment:1 Changed 3 years ago by tyseom

Cc: nusenu@… added

comment:2 Changed 10 months ago by karsten

Severity: Normal
Summary: Compass should allow filtering relays by OSAllow filtering relays by OS

Keep summary as short and precise as possible.

comment:3 Changed 10 months ago by karsten

Owner: set to metrics-team
Status: newassigned

comment:4 Changed 8 months ago by irl

Component: Metrics/CompassMetrics/Atlas

In #23517 it is planned to merge Compass functionality with Relay Search (formerly known as Atlas). These tickets may be relevant to that work and so these are being reassigned to the Metrics/Atlas component.

comment:5 Changed 8 months ago by irl

Component: Metrics/AtlasMetrics/Onionoo

Relay Search will magically support this for both single relay lookup and aggregations if it's supported by Onionoo.

comment:6 Changed 10 days ago by irl

Owner: changed from metrics-team to irl
Status: assignedaccepted

Will look at this this week.

comment:7 Changed 10 days ago by irl

Currently there is no operating system field in the documents. This ticket will only consider adding an index for filtering by operating system. Clients can probably do the same parsing that we will do in Onionoo if they want to get the same string out, I don't think it's necessary to duplicate it.

comment:8 Changed 10 days ago by irl

It looks like summary documents are comprised solely of fields that are present in the node statuses. These seem to be populated using the consensus and GeoIP/DNS information, but not descriptors. This means it's not possible to easily add the operating system information to the node status such that it can be used for indexing/filtering.

karsten - Do you think it is reasonable to have another step in the updater process to include descriptor information in the node statuses just to implement this filter? I wonder if perhaps we should take tickets like this one and put them on a wishlist for a future OnionooDB where they will be considerably easier to implement.

comment:9 Changed 10 days ago by irl

Status: acceptedneeds_information

comment:10 Changed 9 days ago by irl

Cc: metrics-team added

comment:11 Changed 9 days ago by karsten

Wait, we already include descriptor contents for contact information. This doesn't mean that it will be trivial to make the extension suggested above, but at least it would be doable. I'd say it's worth investigating now.

comment:12 Changed 9 days ago by irl

Status: needs_informationaccepted

You're entirely correct. I was looking in the wrong place. It happens in NodeDetailsStatusUpdater.updateNodeDetailsStatuses().

Thanks for the pointer. (:

comment:14 Changed 9 days ago by karsten

Reviewer: karsten

Reviewing...

comment:15 Changed 9 days ago by karsten

Status: needs_reviewneeds_revision

Some things I noticed:

  • You're reusing ResourceServlet#parseVersionParameter for parsing the os parameter. Wouldn't it make more sense to reuse #parseContactParameter here? After all, we'll want to support searches for "Linux" as well as for "Windows 95" here, right? Also, maybe rename the method to #parseContactOrOsParameter to make it clear that the method is used for both.
  • Did you try out what happens when you search for strings that contain spaces? And did you also try out using the new "os" parameter in a qualified search term? IIRC, we did not fix the space issue in qualified search terms, and in this case it may bite us again. I don't have an easy fix for this.
  • Do you mind extending ResourceServletTest to actually test different requests using the new parameter? Maybe the #testContactXY methods serve as a template.

I did not run this code yet. I can do that later today, unless there's revised code that I can test instead.

Thanks!

comment:16 in reply to:  15 Changed 9 days ago by irl

Replying to karsten:

Some things I noticed:

  • You're reusing ResourceServlet#parseVersionParameter for parsing the os parameter. Wouldn't it make more sense to reuse #parseContactParameter here? After all, we'll want to support searches for "Linux" as well as for "Windows 95" here, right? Also, maybe rename the method to #parseContactOrOsParameter to make it clear that the method is used for both.

I've added a new #parseOperatingSystemParameter in a fixup commit. When we're looking at UTF-8 for proposal 285 it would be really good to refactor all the #parseXParameter functions into what they are actually doing, e.g. only printable UTF-8 instead of what the parameter name is. I could not use #parseContactParameter as this was splitting on spaces.

  • Did you try out what happens when you search for strings that contain spaces? And did you also try out using the new "os" parameter in a qualified search term? IIRC, we did not fix the space issue in qualified search terms, and in this case it may bite us again. I don't have an easy fix for this.

I've checked now that spaces do work. In the search string though, we can only search as far as the first space. We can ensure we don't hit any errors in Relay Search by never trying to build search strings that have spaces in the OS. It may not be the best solution but for now at least we do increase functionality, even if it's not to where it could be.

  • Do you mind extending ResourceServletTest to actually test different requests using the new parameter? Maybe the #testContactXY methods serve as a template.

Ok. Will try to do this this afternoon or tomorrow morning. That looks easy enough.

comment:17 Changed 8 days ago by irl

Status: needs_revisionneeds_review

New tests in a fixup commit, along with a fix that the tests caught, proving the value of tests.

I've also fixed all checkstyle complaints.

comment:18 Changed 7 days ago by karsten

Status: needs_reviewmerge_ready

Great! Looks good. Merged to master. Leaving open for the protocol update part. Thanks!

comment:19 Changed 4 days ago by karsten

Milestone: Onionoo 1.15.0

comment:20 Changed 3 days ago by karsten

Resolution: fixed
Status: merge_readyclosed

Closing, because Onionoo 6.1-1.15.0 is now released.

Note: See TracTickets for help on using tickets.