Fix "I'm feeling lucky" search to include all crates #1251

syphar · 2021-01-17T18:32:51Z

This fixes the I'm feeling lucky search ( #789 ) to do an efficient search over all creates, not only the first 280 ones.

I was searching and testing through many different ways to get a random record, and found this to be the best for our dataset. (see also this stackoverflow answer for some overview).

I tested this locally with a dummy-dataset with 600 crates >100 stars, and out of 10k of these queries there were only <50 which needed a second try. In the statement execution time was <10ms all the time.

Other approaches felt too complex in code, were too slow, or were too slow randomly.

I'm happy to implement any needed changes.

In #789 @Kixiron was mentioning metrics to be added, while I'm not sure which metrics are needed.

src/web/releases.rs

jyn514 · 2021-01-17T20:38:35Z

src/web/releases.rs

+                    -- this query too often. 
+                    -- it depends on the percentage of crates with > 100 stars
+                    SELECT DISTINCT 1 + trunc(random() * params.max_id)::INTEGER AS id
+                    FROM params, generate_series(1, 500)


Hard-coding 500 makes me a little nervous. Could we generate this live based on production info? Or does that slow down the query too much?

generating that value live slows down the query too much, I valued speed higher.

the number depends on the ratio of crates >100 stars

while thinking about this, I have another idea (but not likely it's faster)

Let me know if it works out :)

didn't help 😢 was too slow :)

src/web/releases.rs

syphar · 2021-01-18T07:10:36Z

@jyn514 thanks for the review! I'll check the comments in detail in the evening today

Kixiron · 2021-01-18T14:08:28Z

The metrics were mostly because I was curious, I wanted to put a counter on the I'm feeling lucky button that reported how many times it was actually used

Kixiron · 2021-01-18T14:11:26Z

src/web/releases.rs

+    // since there is a chance of this query returning an empty result,
+    // we retry a certain amount of times, and log a warning.
+    for _ in 0..20 {


It seems strange to use a fallible query, is there not a way to make it always work?

@Kixiron I tried (I think) 5-10 different approaches, the ones that always work are either too slow or involve things like a recursive CTE (which is in in the end similar to retrying here, while this is more readable IMHO).

I just ran some numbers: current query runs between 2 and 8 ms (~)
and delivers a random crate directly in most cases (50 misses for 10k queries),

the repetitions are more or less a safeguard.

Also I could not test on a production dataset so I don't know the amount of gaps in the id-span on the crate-table (and the production github stars without backfilling everything locally)

src/web/releases.rs

syphar · 2021-01-18T18:18:32Z

The metrics were mostly because I was curious, I wanted to put a counter on the I'm feeling lucky button that reported how many times it was actually used

@Kixiron wouldn't we get that from the access logs too? or don't we have these?

I'll check other examples and see if it's easy to add

syphar · 2021-01-18T18:47:42Z

@Kixiron @jyn514 I (hopefully) answered your questions and added two commits with the prosed log-statement changes and metrics.
I admit I was definitely focusing on query speed, while sacrificing beauty or (probably a little) readability.

When wanted I could likely produce something better readable, but slower ( ~100 or even 200 ms ) ,

or we could also just add the metrics to see if the feature is actually used or could be removed :)

jyn514

wouldn't we get that from the access logs too? or don't we have these?

The access logs are not integrated with the grafana logging, and also don't work when running locally. Plus I prefer to have as little depending on nginx as possible because it's not tested at all and doesn't go through review.

or we could also just add the metrics to see if the feature is actually used or could be removed :)

I would be interested in that.

When wanted I could likely produce something better readable, but slower ( ~100 or even 200 ms ) ,

I would prefer to have the faster query I think, the searches take long enough already. Thanks for offering though :)

Also I could not test on a production dataset so I don't know the amount of gaps in the id-span on the crate-table (and the production github stars without backfilling everything locally)

Let me know how to test this, happy to runs some queries. I did run the 'redirect_to_random_query' once with EXPLAIN ANALYZE and it took less than a millisecond, great job :)

jyn514 · 2021-01-19T04:03:43Z

src/web/releases.rs

+                    -- this query too often. 
+                    -- it depends on the percentage of crates with > 100 stars
+                    SELECT DISTINCT 1 + trunc(random() * params.max_id)::INTEGER AS id
+                    FROM params, generate_series(1, 500)


Let me know if it works out :)

syphar · 2021-01-19T19:31:56Z

or we could also just add the metrics to see if the feature is actually used or could be removed :)

I would be interested in that.

To validate what I understand: I would create a separate PR only adding the metrics. Then we watch how often this is actually used and we decide to drop or fix? @jyn514

Also I could not test on a production dataset so I don't know the amount of gaps in the id-span on the crate-table (and the production github stars without backfilling everything locally)

Let me know how to test this, happy to runs some queries. I did run the 'redirect_to_random_query' once with EXPLAIN ANALYZE and it took less than a millisecond, great job :)

I just wrote a small (ugly) tool: https://gist.github.com/syphar/949cdb2ee297ca5695bad179cfd99a6e

With that you should be able to play around with the numbers :)

jyn514 · 2021-01-19T23:09:25Z

To validate what I understand: I would create a separate PR only adding the metrics. Then we watch how often this is actually used and we decide to drop or fix? @jyn514

I think this is a good change as-is, I would be happy merging now and we can add the metrics PR later.

With that you should be able to play around with the numbers :)

Thanks! I tested this in prod and it never came up missing:

records: 10000
with result: 10000
without result: 0

So I don't think we have to worry.

Is this ready to merge? It looks good from my end :)

syphar · 2021-01-20T08:37:04Z

To validate what I understand: I would create a separate PR only adding the metrics. Then we watch how often this is actually used and we decide to drop or fix? @jyn514

I think this is a good change as-is, I would be happy merging now and we can add the metrics PR later.

After the comment by @Kixiron I already added metrics, as I understood how it would have to be done (see e9bfc47 ) .

Is this ready to merge? It looks good from my end :)

From my side It would be ready, except we want more tests, different metrics, or any other change

syphar · 2021-01-20T13:08:07Z

@jyn514 just discovered by accident that my testing-gist used an even smaller number or random IDs (100) and still always delivered results.

But 500 is fast enough, so I guess we're fine with the current number for some time.

Fix I'm feeling lucky search to include all crates

243a869

jyn514 reviewed Jan 17, 2021

View reviewed changes

Kixiron reviewed Jan 18, 2021

View reviewed changes

syphar added 2 commits January 18, 2021 19:20

add/fix logging statements

bf88deb

add metrics for "I'm feeling lucky" searches

e9bfc47

jyn514 reviewed Jan 19, 2021

View reviewed changes

jyn514 merged commit e9f300a into rust-lang:master Jan 21, 2021

Nemo157 mentioned this pull request Jan 21, 2021

Switch to chart.js for the release activity page #1255

Merged

syphar deleted the fix-im-feeling-lucky branch January 21, 2021 19:25

syphar mentioned this pull request Mar 21, 2021

Fix "I'm feeling lucky" query and add metrics #789

Closed

Fix "I'm feeling lucky" search to include all crates #1251

Fix "I'm feeling lucky" search to include all crates #1251

Uh oh!

Conversation

syphar commented Jan 17, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

syphar commented Jan 18, 2021

Uh oh!

Kixiron commented Jan 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

syphar commented Jan 18, 2021

Uh oh!

syphar commented Jan 18, 2021

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syphar commented Jan 19, 2021

Uh oh!

jyn514 commented Jan 19, 2021

Uh oh!

syphar commented Jan 20, 2021

Uh oh!

syphar commented Jan 20, 2021

Uh oh!

Uh oh!