Record search queries smartly #6088

dojutsu-user · 2019-08-21T14:17:33Z

This PR refactor the recording mechanism for better support of "search as you type" feature.
Also, It makes all the queries lowercase so as to have no difference between words like Build and build

dojutsu-user · 2019-08-21T14:41:30Z

Currently, it reduces the total partial queries -- not all but better than storing every partial query.

stsewd · 2019-08-21T15:06:39Z

readthedocs/search/tasks.py

        project=project,
        version=version,
        query=query,
    )
+    obj.created = time


Shouldn't this be an auto field?

Yes... but the actual time is when the query was made.
Celery task are not executed immediately so the automated time will be wrong.

I'd say that's probably OK to use the auto field for this.

I used this data for query also, so I think if we use automated data, results will be wrong.

https://github.com/readthedocs/readthedocs.org/pull/6088/files#diff-2a0ffe6d23a7ca20d7e60b8e1617836bR161

https://github.com/readthedocs/readthedocs.org/pull/6088/files#diff-2a0ffe6d23a7ca20d7e60b8e1617836bR168

It should be querying the modifed field, and using that -- it will automatically update whenever it is saved: https://github.com/django-extensions/django-extensions/blob/master/django_extensions/db/models.py#L19 -- we should likely use that, instead of trying to change the created date.

stsewd · 2019-08-21T15:09:12Z

Also, what about just saving the query if it is greater than 3 characters?

dojutsu-user · 2019-08-21T15:19:50Z

@stsewd

Also, what about just saving the query if it is greater than 3 characters?

This will fail in many cases, like: readt, readthe, readthedo (for readthedocs)

stsewd · 2019-08-21T15:29:35Z

Why would that fail?

We are start executing the same logic from readt. I'm not saying to remove the logic from this PR

dojutsu-user · 2019-08-21T15:31:05Z

@stsewd
Ohhh... Okay
I get it.
Yeahh... that's a nice improvement.
I will update the PR.

dojutsu-user · 2019-08-21T16:35:13Z

This is not ready for a review. It is too far from being OK.
Results are not good currently.

readthedocs/search/tasks.py

davidfischer · 2019-08-21T21:49:55Z

readthedocs/search/tasks.py

+        version__slug=version_slug,
+        query__startswith=query,
+        created__gte=before_10_sec,
+    ).order_by('-created')


I wonder about the performance characteristics of this. I don't believe created is indexed although project and version are. This might be worth testing on the live system before merging. This might be ok but I'm not 100% confident without just timing a similar query in prod.

readthedocs/search/tasks.py

davidfischer · 2019-08-21T21:55:59Z

readthedocs/search/api.py


        # record the search query with a celery task
        tasks.record_search_query.delay(
            project_slug,
            version_slug,
            query,
            total_results,
+            time,


Depending on how tasks are serialized, this may not work exactly as planned. I know celery supports Pickle and JSON and I'm not sure which one we are using. Datetimes are not JSON serializable by default although Django does have a way to do it. However, I'm not sure they come back as timezone aware datetimes after they do. Did you verify this?

During local development, celery uses json serializer.

readthedocs.org/readthedocs/settings/dev.py

Line 39 in 3c04a96

CELERY_RESULT_SERIALIZER = 'json'

(Docs: https://docs.celeryproject.org/en/latest/userguide/configuration.html?highlight=CELERY_RESULT_SERIALIZER#new-lowercase-settings)

they come back as timezone aware datetimes after they do

I am not sure how to verify this.
I did print the timezone.now() and the SQL query -- that looks pretty similar.

timezone.now(): 2019-08-23 08:45:42.841161+00:00
SQL Query:

SELECT "search_searchquery"."id", "search_searchquery"."created", "search_searchquery"."modified", "search_searchquery"."project_id", "search_searchquery"."version_id", "search_searchquery"."query" FROM "search_searchquery" INNER JOIN "projects_project" ON ("search_searchquery"."project_id" = "projects_project"."id") INNER JOIN "builds_version" ON ("search_searchquery"."version_id" = "builds_version"."id") WHERE ("projects_project"."slug" = rtd AND "builds_version"."slug" = latest AND "search_searchquery"."query" LIKE read% ESCAPE '\' AND "search_searchquery"."created" >= 2019-08-23 08:45:32.841161) ORDER BY "search_searchquery"."created" DESC

davidfischer · 2019-08-21T21:57:23Z

readthedocs/search/tasks.py

+        return
+
+    # don't record query with zero results.
+    if not total_results:


Is there a reason this block isn't at the top of the function? I guess you could update the time on another query even if this one returns no results. However, that probably warrants a comment or something.

Is there a reason this block isn't at the top of the function?

Yes. I wanted to update the timing and query even if no results are there. For example: readthed will return no results but I wanted to update the timing and query if there is a previous query of readth.

I guess you could update the time on another query even if this one returns no results. However, that probably warrants a comment or something.

I am not sure if I understood.

ericholscher · 2019-08-27T22:29:33Z

@dojutsu-user what is the status here? Is it working locally, or still needs more work?

dojutsu-user · 2019-08-28T11:47:03Z

@ericholscher
I would really like to have some help here.
The logic and everything seems correct but I it is not working as expected.

ericholscher

Think this looks good, but we should definitely be updating the modified time, not the created time :)

ericholscher · 2019-08-28T18:49:28Z

readthedocs/search/tasks.py

        project=project,
        version=version,
        query=query,
    )
+    obj.created = time


It should be querying the modifed field, and using that -- it will automatically update whenever it is saved: https://github.com/django-extensions/django-extensions/blob/master/django_extensions/db/models.py#L19 -- we should likely use that, instead of trying to change the created date.

ericholscher · 2019-09-05T22:05:44Z

Gonna merge this so it goes out on the next deploy, but would like to do some small updates to this over time.

humitos · 2019-09-09T12:16:22Z

Think this looks good, but we should definitely be updating the modified time, not the created time :)

@dojutsu-user can you create an issue to track this?

dojutsu-user · 2019-09-09T14:03:53Z

@humitos
Sure 👍

record queries smartly

0603ed4

dojutsu-user changed the title ~~Record queries smartly~~ Record search queries smartly Aug 21, 2019

dojutsu-user requested review from ericholscher and davidfischer August 21, 2019 14:18

fix lint

81b1f5d

stsewd reviewed Aug 21, 2019

View reviewed changes

dojutsu-user added the PR: work in progress Pull request is not ready for full review label Aug 21, 2019

add .strip()

703f510

davidfischer reviewed Aug 21, 2019

View reviewed changes

queries improvements

d1090f0

Merge branch 'master' into recording-query-smartly

2001be4

dojutsu-user added 3 commits August 28, 2019 23:46

fix logic

9200121

add comment

58ecb39

add test

294f6bd

ericholscher approved these changes Sep 5, 2019

View reviewed changes

ericholscher merged commit a1f3b05 into readthedocs:master Sep 5, 2019

dojutsu-user deleted the recording-query-smartly branch September 6, 2019 04:31

dojutsu-user mentioned this pull request Sep 9, 2019

Use modified time instead of created time for recording search queries #6155

Closed

Uh oh!

Record search queries smartly #6088

Record search queries smartly #6088

Uh oh!

Conversation

dojutsu-user commented Aug 21, 2019

Uh oh!

dojutsu-user commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

stsewd commented Aug 21, 2019

Uh oh!

dojutsu-user commented Aug 21, 2019

Uh oh!

stsewd commented Aug 21, 2019

Uh oh!

dojutsu-user commented Aug 21, 2019

Uh oh!

dojutsu-user commented Aug 21, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dojutsu-user Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dojutsu-user Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericholscher commented Aug 27, 2019

Uh oh!

dojutsu-user commented Aug 28, 2019

Uh oh!

ericholscher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericholscher commented Sep 5, 2019

Uh oh!

humitos commented Sep 9, 2019

Uh oh!

dojutsu-user commented Sep 9, 2019

Uh oh!

Uh oh!

dojutsu-user commented Aug 21, 2019 •

edited

Loading

dojutsu-user Aug 23, 2019 •

edited

Loading

dojutsu-user Aug 23, 2019 •

edited

Loading