✂️ Remove search from Connect #13235

atsmith813 · 2021-04-02T18:22:38Z

What type of PR is this? (check all applicable)

Refactor

Description

This PR is part of the migration process from Elasticsearch to PostgreSQL FTS. TLDR - this PR removes search from Connect.

We started by exploring replacing Elasticsearch with PostgreSQL FTS for Connect. In doing so, we learned a few things and decided it would be better to scrap search within Connect all together, for now (the idea of removing search from Connect has product sign-off already).

The model/object relationships are a bit complicated - they're spread out across ChatChannelMembership, ChatChannel, Message, and User.
Connect itself is rarely used and searching within Connect is used even less.
The current search in Connect UX is buggy and it's out of the scope of this PR and migration project to fix it right now.
The current search isn't the most useful as it searches only the title of the chat and names of the users, not the message contents itself. In other words, the primary use case is for users who have many chats and want a way to quickly jump to a specific one. As mentioned above, Connect isn't really used.
Connect is scheduled for a major overhaul - it would be better to revisit search at that time once we know more about what we want the page to look like, what we want to search for, etc.
This project aims to get Elasticsearch out as quickly and smoothly as possible, which this PR helps address. It's "easier" to remove search than it is to make PostgreSQL FTS performant for something that isn't used and is going to change.

I'm leaving all the Elasticsearch code for Connect in place for now. We'll remove it at the end in the PR where we take out everything Elasticsearch related.

Before

After

Related Tickets & Documents

RFC 0153: https://github.com/forem/rfcs/pull/153
https://github.com/orgs/forem/projects/29#card-57639173

QA Instructions, Screenshots, Recordings

Admittedly, testing Connect in development isn't the smoothest 😅 . Here's what I did:

Fire up the app locally (bin/startup).
Sign up/sign in to your own account.
Follow the local admin account (admin_mcadmin).
Log out of your account and log into the local admin account (info for this is in the seed file).
Follow your other account back.
Send your account some messages.
Make sure everything looks fine, click around.
Log out of the local admin account and back into your account.
Send the local admin account some messages back.
Make sure everything looks fine, click around.

Added tests?

Yes

[Forem core team only] How will this change be communicated?

I'm not sure how best to communicate this change and need help - Do we need to include this change in a changelog post or announce this to the community in any way?
I will share this change internally with the appropriate teams

app/controllers/search_controller.rb

atsmith813 · 2021-04-02T18:44:06Z

app/controllers/search_controller.rb

+        [current_user.id]
+      end
+
+    result = Search::Postgres::ChatChannelMembership.search_documents(


You'll notice I left everything in the Search namespace (the route, the controller, and the service naming). I originally started removing chat_channels from search all together since this behaves more like a filter than a search.

However, I discovered there are some JS components that are using this endpoint as an "index". I started refactoring everything, but it quickly got out of hand and out of scope for what this PR is intended to address. I also realized it may actually be nice to leave everything in the search namespace so that when we re-do the search for Connect in the future, it's a smoother experience.

I started refactoring everything, but it quickly got out of hand and out of scope for what this PR is intended to address.

👍 good call taking the simpler approach in this PR

With that said, since we have the ChatChannelsController and the ChatChannelMembershipsController each with index actions, I think the right move would be to move those JS components to them so we don't have multiple controllers performing the same actions. I also am always hesitant to leave code around with the idea we might use it. What do you think about making an Issue for it once this is merged so someone else can tackle the refactor at some point?

#13277 🚀

atsmith813 · 2021-04-02T18:44:25Z

app/javascript/chat/chat.jsx

-          <input
-            placeholder="Search Channels"
-            onKeyUp={this.debouncedChannelFilter}
-            id="chatchannelsearchbar"
-            className="crayons-textfield"
-            aria-label="Search Channels"
-          />


Removing the search box from the UX 🎉

atsmith813 · 2021-04-02T18:44:38Z

app/javascript/chat/util.js

@@ -118,7 +118,6 @@ export const createDataHash = (additionalFilters, searchParams) => {
  }
  dataHash.per_page = 30;
  dataHash.page = searchParams.paginationNumber;
-  dataHash.channel_text = searchParams.query;


We don't search by any terms anymore.

atsmith813 · 2021-04-02T19:10:53Z

app/serializers/search/chat_channel_membership_serializer.rb

    attributes :id, :status, :viewable_by, :chat_channel_id, :last_opened_at,
               :channel_text, :channel_last_message_at, :channel_status,
-               :channel_status, :channel_type, :channel_username, :channel_name,
-               :channel_image, :channel_modified_slug, :channel_discoverable, :channel_messages_count
+               :channel_type, :channel_username, :channel_name, :channel_image,
+               :channel_modified_slug, :channel_discoverable,
+               :channel_messages_count


:channel_status was originally duplicated and I cleaned up the spacing/formatting a little bit.

mstruve

Pulled down the branch, clicked around and everything looks good!

mstruve · 2021-04-02T20:22:19Z

app/controllers/search_controller.rb

+        [current_user.id]
+      end
+
+    result = Search::Postgres::ChatChannelMembership.search_documents(


I started refactoring everything, but it quickly got out of hand and out of scope for what this PR is intended to address.

👍 good call taking the simpler approach in this PR

With that said, since we have the ChatChannelsController and the ChatChannelMembershipsController each with index actions, I think the right move would be to move those JS components to them so we don't have multiple controllers performing the same actions. I also am always hesitant to leave code around with the idea we might use it. What do you think about making an Issue for it once this is merged so someone else can tackle the refactor at some point?

jgaskins

I had a whole in-depth performance analysis written here and then GitHub was like "there was a problem submitting your review" and then threw it all away 😡

This was the TL;DR (too long; damn sure ain't rewriting):

We're getting what looks like N+1 queries
- I think it's due to ActiveRecord's back-and-forth traversal of the associations
- I tried adding a bunch of inverse_of clauses to the associations to see if I could get it to go away and it wasn't working
- Honestly, posting the queries is probably what broke submitting the review 😢
Stats on a prod DEV snapshot:
- p50 = 8ms
- p95 = 136ms
- p99 = 395ms
CCM stats:
- Only 15% of users have any CCMs at all
- 9.2% of users have > 5
- 0.4% of users have >= 10

If we can fix the N+1 queries trivially, that would be fantastic, but if not I'm not super concerned.

app/services/search/postgres/chat_channel_membership.rb

vaidehijoshi · 2021-04-03T13:59:58Z

Do we need to include this change in a changelog post or announce this to the community in any way?

My personal feeling here is that we shouldn't announce this to the community to avoid getting more eyes on a part of the product that is going to likely be removed or at least is a bit in flux right now. Folks already don't really use Connect all that much (based on statistics that we've pulled in the past at least), so telling them about changes there will likely just drive people to look at it (and since this is removing functionality, getting user's eyes on it kind of feels like a moot point).

That's just my personal opinion though, maybe Product folks will disagree with me 🤷🏽‍♀️

citizen428

This looks good @atsmith813. If you can fix the problems @jgaskins mentions that'd be great but given the somewhat unclear future of Connect and its low usage I'd probably timebox the effort and just live with it if we can't find a solution.

app/controllers/search_controller.rb

rhymes

Agree with the idea! Thanks for following through with the idea!

One thing I'm not sure I understand: why are we adding code for Search::Postgres::ChatChannel if we are removing the search from the UI?

Isn't it redudant? I understand leaving the ES code in, but why are we adding the PostgreSQL version as well?

Thank you!

atsmith813 · 2021-04-05T16:15:04Z

@rhymes

I left everything in the Search namespace (the route, the controller, and the service naming). I originally started removing chat_channels from search all together since this behaves more like a filter than a search.

However, I discovered there are some JS components that are using this endpoint as an "index". I started refactoring everything, but it quickly got out of hand and out of scope for what this PR is intended to address. I also realized it may actually be nice to leave everything in the search namespace so that when we re-do the search for Connect in the future, it's a smoother experience.

atsmith813 · 2021-04-05T16:30:24Z

@vaidehijoshi - great call WRT communicating this change!

rhymes

Ah got it, sorry, I always get confused as we overload the term "search" with "search and filter" :D So Search::Postgres::ChatChannelMembership is only a "filter" :-)

app/services/search/postgres/chat_channel_membership.rb

atsmith813 · 2021-04-06T00:53:14Z

@jgaskins

and then GitHub was like "there was a problem submitting your review" and then threw it all away

😭 that's brutal! How did you investigate this? My first thought would be to hop in a prod console and run the same exact Active Record query, but I don't have access.

I played around with it locally, no luck yet. Maybe the n+1 is coming from something else on that page?

jgaskins · 2021-04-06T01:06:23Z

@atsmith813 I pulled a prod snapshot to benchmark changes against locally.

At some point I'd like to set up something that lets all Forem devs pull sanitized prod snapshots (all PII overwritten with data from faker, passwords all changed to password, anything we can't get via Blazer would be scrubbed) so that we can all check things against a realistic volume of data, but that'll probably be a while since it requires some infrastructure and a better knowledge of the DB than I currently have. 😄

jgaskins · 2021-04-06T01:08:48Z

Mostly when I'm checking that stuff I'm just trying to make sure it's not going to put an unreasonable amount of pressure on the DB — hot endpoints should be pretty quick but infrequently used endpoints can be a little slow, as a treat.

Remove search from connect

61e3a7d

pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Apr 2, 2021

atsmith813 changed the title ~~Remove search from connect~~ ✂️ Remove search from Connect Apr 2, 2021

atsmith813 commented Apr 2, 2021

View reviewed changes

app/controllers/search_controller.rb Outdated Show resolved Hide resolved

atsmith813 commented Apr 2, 2021

View reviewed changes

Remove alternate approach

2d6ff38

atsmith813 marked this pull request as ready for review April 2, 2021 19:04

atsmith813 requested a review from a team April 2, 2021 19:04

atsmith813 requested a review from a team as a code owner April 2, 2021 19:04

pr-triage bot removed the PR: draft bot applied label for PR's that are a work in progress label Apr 2, 2021

atsmith813 requested review from aitchiss and msarit and removed request for a team April 2, 2021 19:04

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Apr 2, 2021

atsmith813 requested review from rhymes, citizen428, mstruve and jgaskins April 2, 2021 19:04

atsmith813 commented Apr 2, 2021

View reviewed changes

mstruve approved these changes Apr 2, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Apr 2, 2021

jgaskins approved these changes Apr 2, 2021

View reviewed changes

app/services/search/postgres/chat_channel_membership.rb Outdated Show resolved Hide resolved

citizen428 approved these changes Apr 5, 2021

View reviewed changes

app/controllers/search_controller.rb Outdated Show resolved Hide resolved

rhymes reviewed Apr 5, 2021

View reviewed changes

Alphabetize CHAT_CHANNEL_PARAMS

99f19c6

Remove default value for user_ids

2248a2f

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Apr 5, 2021

rhymes approved these changes Apr 5, 2021

View reviewed changes

app/services/search/postgres/chat_channel_membership.rb Outdated Show resolved Hide resolved

app/services/search/postgres/chat_channel_membership.rb Show resolved Hide resolved

Add spec for result ordering

78eb052

atsmith813 merged commit 3891c7d into master Apr 6, 2021

atsmith813 deleted the atsmith813/remove-search-from-connect branch April 6, 2021 17:03

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Apr 6, 2021

atsmith813 mentioned this pull request Apr 6, 2021

Refactor /search/chat_channels endpoint #13277

Closed

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: merged bot applied label for PR's that are merged labels Apr 6, 2021

rhymes added this to DONE 🎉 in Current Cycle Work Apr 27, 2021

✂️ Remove search from Connect #13235

✂️ Remove search from Connect #13235

atsmith813 commented Apr 2, 2021 •

edited

atsmith813 Apr 2, 2021

mstruve Apr 2, 2021

atsmith813 Apr 6, 2021

atsmith813 Apr 2, 2021

atsmith813 Apr 2, 2021

atsmith813 Apr 2, 2021

mstruve left a comment

mstruve Apr 2, 2021

jgaskins left a comment

vaidehijoshi commented Apr 3, 2021

citizen428 left a comment

rhymes left a comment

atsmith813 commented Apr 5, 2021

atsmith813 commented Apr 5, 2021

rhymes left a comment

atsmith813 commented Apr 6, 2021

jgaskins commented Apr 6, 2021

jgaskins commented Apr 6, 2021

✂️ Remove search from Connect #13235

✂️ Remove search from Connect #13235

Conversation

atsmith813 commented Apr 2, 2021 • edited

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Added tests?

[Forem core team only] How will this change be communicated?

atsmith813 Apr 2, 2021

Choose a reason for hiding this comment

mstruve Apr 2, 2021

Choose a reason for hiding this comment

atsmith813 Apr 6, 2021

Choose a reason for hiding this comment

atsmith813 Apr 2, 2021

Choose a reason for hiding this comment

atsmith813 Apr 2, 2021

Choose a reason for hiding this comment

atsmith813 Apr 2, 2021

Choose a reason for hiding this comment

mstruve left a comment

Choose a reason for hiding this comment

mstruve Apr 2, 2021

Choose a reason for hiding this comment

jgaskins left a comment

Choose a reason for hiding this comment

vaidehijoshi commented Apr 3, 2021

citizen428 left a comment

Choose a reason for hiding this comment

rhymes left a comment

Choose a reason for hiding this comment

atsmith813 commented Apr 5, 2021

atsmith813 commented Apr 5, 2021

rhymes left a comment

Choose a reason for hiding this comment

atsmith813 commented Apr 6, 2021

jgaskins commented Apr 6, 2021

jgaskins commented Apr 6, 2021

atsmith813 commented Apr 2, 2021 •

edited