Opened 5 years ago

Closed 3 years ago

#15395 closed defect (fixed)

add support for more than 40 results

Reported by: cypherpunks Owned by: irl
Priority: Medium Milestone:
Component: Metrics/Relay Search Version:
Severity: Normal Keywords: complete-before-abandoning-globe
Cc: nusenu@…, irl@…, phw Actual Points:
Parent ID: #18893 Points:
Reviewer: Sponsor:

Description

If your search results in more than 40 results you will get:

Too many matches!

The current version of Atlas does not support a result set greater than 40. This is due to some performance issues in doing multiple parallel connections in javascript. Future versions will hopefully manage to overcome this issue.

sample (big relay groups):

https://atlas.torproject.org/#search/contact:39B9820C

https://onionoo.torproject.org/summary?contact=39B9820C

Child Tickets

Change History (17)

comment:1 Changed 5 years ago by tyseom

Cc: nusenu@… added

comment:2 Changed 3 years ago by karsten

Keywords: complete-before-abandoning-globe added
Severity: Normal

Suggestion: Display the first 40 results and indicate in a warning that there are really more than 40 results that cannot all be displayed. In theory and without looking at the code, this should be a simple fix.

If possible, let's fix this before dumping Globe.

comment:3 Changed 3 years ago by karsten

Status: newneeds_review

Please review my branch task-15395. I tested it locally, but it's certainly possibly that I overlooked an edge case or two.

comment:4 Changed 3 years ago by irl

<% if (relays.length >= 40) {%>

Should this actually ever happen? I mean, == 40 is fine, but at most 40 relays will be returned so if more than 40 are returned something has gone horribly wrong.

<% if (i > 40) return true; %>

Again here, if this code path is ever followed, then there is an error occuring in your changes in results.js and at best we're disguising the error.

(These comments are not actually bugs, more just a preference for not having more code than is necessary to achieve the goal and attempting to minimise complexity - my local testing has not thrown up any issues with this patch).

Last edited 3 years ago by irl (previous) (diff)

comment:5 Changed 3 years ago by irl

Looking at #17938, we can use this as a temporary fix bringing Atlas to the same level of functionality as Globe but we really should implement pagination later.

comment:6 Changed 3 years ago by irl

Cc: irl@… added

comment:7 Changed 3 years ago by hellais

I checked out that branch. The only issue I have with it is that it was considering too many results also the situation where you had exactly 40 relays, since it is truncating the list of relays and assuming that if there are 40 then it's due to truncation and not that this is the value returned by onionoo.

To overcome this I wrote a patch to forward the exact error code for this case into the view. You can find it at this branch: https://github.com/hellais/atlas/commit/e2b19792c8d67077af0db1613fee0cdd5067e042 that is based on top of yours.

comment:8 Changed 3 years ago by hellais

So looking back at actually properly fixing this and not just making a workaround, I realised that the true issue lied in the fact that a full page-re-render was being done every time a relay was being looked up.

This obviously lead to terrible performances.

By delaying the rendering of the page of results until all the relays have been looked up there is a noticeable performance benefit: https://github.com/hellais/atlas/commit/4da8ad27333efb72eddcc69bad5fb42c15b740bc.

I think there is probably a better way of doing this by leveraging by doing some two-way data binding in backbone (in angular this is quite trivial to do, but in backbone there a slight bit more work to do it properly: see: http://stackoverflow.com/questions/12604834/two-way-data-binding-in-backbone-js)

comment:9 Changed 3 years ago by irl

Owner: changed from phw to irl
Status: needs_reviewaccepted

The performance is still not ideal above 500 results on my machine.

A search for country:gb is OK but country:de locks up my browser for a while.

I'll merge together the two solutions (fixing the over-rendering and passing along an error if the limit is exceeded) and test, then come back with a final patch for review.

comment:10 Changed 3 years ago by irl

Status: acceptedneeds_review
 Allow returning up to 500 search results and error on empty search

 * Added the ability for errors to be passed along when rendering result
   sets, to display an error or warning message while still allowing
   a result set to be rendered.
 * Results are only rendered once all the requests have succeeded (or
   failed), not every time there is a response. This greatly improves
   the performance and now up to 500 results can be rendered reasonably.
 * Limits the result set to a maximum of 500 results, introducing a new
   error message when this number is exceeded. (Fixes: #15395)
 * Introduces a new error message when an empty string is passed and
   fixed the rendering of that error message by displaying it. (Fixes:
   #19372)
 * Thanks to karsten and hellais for their help on this patch. (:

https://github.com/irl/atlas/commit/b2ac2eeed6afbf984124b52d79582b98b82f70d7

I think this solves everything.

Example in action: http://irl.github.io/atlas/#search/country:de

Tested locally quite extensively, and accidentally also fixed #19372 as a side-effect.

comment:11 Changed 3 years ago by karsten

The patch looks very reasonable, though I'd like to make one request: can you please change the number 500 back to 40 for now? Here's why: For n results, Atlas makes n+1 requests to the Onionoo server which returns n+1 responses each containing 1 result. That's just crazy. In theory, it should only make 1 request containing n results. We'll have to change the queries that Atlas makes and very likely the way how Atlas processes responses. I'm happy to discuss those changes, but we could easily do that after applying this patch. Except for those change from 40 to 500 this patch looks ready to go. Cool stuff, thanks a lot!

comment:12 Changed 3 years ago by irl

karsten: Would you be happy with 50? This would be the same limit Globe has been using.

I agree that we should be doing just 1 request for the search, and I'd be happy for that bug to be filed after this patch is applied. (:

Patch with 50 as the limit: https://github.com/irl/atlas/commit/feecc4b16ce7427cf9de5b92d986335196576aba

Last edited 3 years ago by irl (previous) (diff)

comment:13 Changed 3 years ago by karsten

Sure, 50 is as good as 40. Thanks!

The new ticket would have a summary like "Make single request for Onionoo details document" and would say in the description "Rather than making 1 request for an Onionoo summary document and then n requests for full Onionoo details documents, let's make a single request for 1 Onionoo details document. Exact parameters are subject to further discussion but would include limit=51 (50 results max and 1 to see if there are even more), fields=... (all fields we'd like to display in the results table), search=... (whatever we pass to the summary request right now), and maybe more." Let's discuss more details once this patch is merged.

comment:14 Changed 3 years ago by karsten

Cc: phw added

Ah, I didn't say this explicitly above, but the new patch (feecc4b) looks good, to me. I'd say merge. phw, what do you think? Happy to merge and deploy if you're okay with that.

comment:15 Changed 3 years ago by irl

Parent ID: #18893

comment:16 Changed 3 years ago by phw

Thanks karsten, hellais, and irl! feecc4b is now merged and deployed.

Karsten, I took the liberty of creating the ticket you requested. It's #19452.

Last edited 3 years ago by phw (previous) (diff)

comment:17 Changed 3 years ago by phw

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.