4230 search query suggested changes

[grossir]

Some details and questions:

1. I didn't want to commit directly on Sergei's branch, so I intended for this to be PR over that branch, but I see Github has interpreted it as a PR on main, even when I branched from it

2. Setting on_delete=models.CASCADE to the User FK follows this comment from the original issue:

When accounts are deleted, the history must be nuked, not just anonymized.

3. Should we really use AbstractDateTimeModel as base class for SearchQuery? Are we ever going to modify the SearchQuery once it happens? In case we don't, there is no use for the date_modified field and it's index; and expecting this new table to have so many rows, we would save some space / speed

4. About computing the "query_time_ms" using time.perf_counter_ns, it seems to be the most precise. Check the section in Python docs https://docs.python.org/3/library/time.html#time.perf_counter.

5. Also, I noted that with these changes, we will only be capturing search queries from the main homepage/search for Case Law; it won't log queries for RECAP archive, citation search, parenthetical search, etc. Is it of interes to log that?

6. I took some freedom to refactor the search.views.show_results function, it could use early returns to reduce indentation levels, which were confusing since the if/else clauses are so long.

7. I don't know how to compute the hit_cache field, we should probably ask Alberto how to know if ES or Solr were using the cache. I see that the do_es_search functions have a cache argument, but it's not used on the show_results function. For now, I have set it to False always

[sentry-io]

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: cl/search/views.py

Function	Unhandled Issue
show_results	OpinionCluster.MultipleObjectsReturned: get() returned more than one OpinionCluster -- it returned 2! ... Event Count: 1

---

_{Did you find this useful? React with a 👍 or 👎}

[mlissner]

Should we really use AbstractDateTimeModel as base class for SearchQuery?

Yeah, you're right. Let's just do a date_created field.

time.perf_counter_ns

I'm not sure, but I think that might not do what we want. Do you know if it keeps counting while we want for network requests? I think the Python speed isn't very interesting and the important part is the Elastic speed.

Also: Does counting nanoseconds take more CPU time? If so, it's not really worth doing. I think they'll be harder to reason about too.

it won't log queries for RECAP archive, citation search, parenthetical search, etc. Is it of interes to log that?

Yes. We should log all this stuff and the API too. We might want to add a boolean for API requests though, so they're separate from the rest. Some API users do really weird (and slow) stuff compared to actual users.

hit_cache

We'll need to get that from the downstream function. Maybe it can return whether the cache was used?

Thank you, Gianfranco!