Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Improve Engagement Dashboard's "Channels" tab performance #32493

Merged
merged 33 commits into from
Jul 19, 2024

Conversation

matheusbsilva137
Copy link
Member

@matheusbsilva137 matheusbsilva137 commented May 24, 2024

Proposed changes (including videos or screenshots)

  • Replace duplicated aggregation execution (which was used only to count documents) by a single request for counting all rooms;
  • Apply limit as soon as possible in the aggregation so as to carry less data to the next steps;
  • Only return channels, groups and DM rooms from this endpoint. Livechat rooms have been removed since we don't track their messages data.
  • Added a hideRoomsWithNoActivity param to the engagement-dashboard/channels/list endpoint, which enables a faster search by not returning rooms that had no message sent in the given period. This param is being used as true by default in our client, which will be the default behavior starting on 7.0 (when this param will be removed).

Issue(s)

Steps to test or reproduce

The Engagement Dashboard's "Channels" tab should keep working just like before, but faster when there's a huge amount of rooms in the workspace. Channels should always be listed sorted by their amount of messages when no sort param is provided.
What should be validated:

  • Rooms returned by the engagement-dashboard/channels/list endpoint should be sorted by their amount of new messages in the analyzed timespan (decreasingly);
  • The engagement-dashboard/channels/list endpoint should accept a new boolean hideRoomsWithNoActivity param, which enables a faster search by removing rooms that had no messages sent in the analyzed timespan. This is specially relevant for workspaces that have a huge amount of rooms (this option is being used in the UI);
  • A deprecation warning log should be displayed whenever the engagement-dashboard/channels/list endpoint is called (so that users are aware that the new param will be deprecated);
  • Livechat rooms should not be returned by the engagement-dashboard/channels/list endpoint anymore.

Further comments

SUP-513
SUP-558

Aggregation explain (before): explain-aggregation-before.json

Aggregation explain (after): explain-aggregation-after.json

Aggregation explain (after and using hideRoomsWithNoActivity param): aggregation-explain-after-hide-rooms-with-no-activity.json

Copy link
Contributor

dionisio-bot bot commented May 24, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented May 24, 2024

🦋 Changeset detected

Latest commit: f3ca6f0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 32 packages
Name Type
@rocket.chat/meteor Minor
@rocket.chat/model-typings Minor
@rocket.chat/apps Patch
@rocket.chat/models Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/omnichannel-services Patch
rocketchat-services Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/instance-status Patch
@rocket.chat/presence Patch
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/ui-contexts Major
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/ddp-client Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-client Major
@rocket.chat/ui-video-conf Major
@rocket.chat/web-ui-registration Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@matheusbsilva137 matheusbsilva137 added this to the 6.10 milestone May 24, 2024
Copy link

codecov bot commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.52%. Comparing base (e6ce4ee) to head (f3ca6f0).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32493      +/-   ##
===========================================
- Coverage    55.58%   55.52%   -0.06%     
===========================================
  Files         2634     2634              
  Lines        57281    57255      -26     
  Branches     11861    11858       -3     
===========================================
- Hits         31838    31790      -48     
- Misses       22750    22770      +20     
- Partials      2693     2695       +2     
Flag Coverage Δ
e2e 54.26% <ø> (-0.08%) ⬇️
unit 72.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@matheusbsilva137 matheusbsilva137 marked this pull request as ready for review May 24, 2024 18:12
@matheusbsilva137 matheusbsilva137 requested a review from a team as a code owner May 24, 2024 18:12
apps/meteor/server/models/raw/Rooms.ts Outdated Show resolved Hide resolved
apps/meteor/ee/server/lib/engagementDashboard/channels.ts Outdated Show resolved Hide resolved
packages/model-typings/src/models/IRoomsModel.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@KevLehman KevLehman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

༼ つ ◕_◕ ༽つ API tests

@matheusbsilva137 matheusbsilva137 requested a review from a team as a code owner May 27, 2024 18:50
@geekgonecrazy
Copy link
Contributor

@matheusbsilva137 since adjusting the aggregation as part of improving can you please attach the mongo explain of the query before the optimization and after?

ideally with some real amount of data being processed so we can get an idea if the query is going to utilize appropriate indexes and be any better.

Copy link
Member Author

@matheusbsilva137 matheusbsilva137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just attached the old and new aggregations explains to the PR description.
The new aggregation uses the t index for rooms, while the old one doesn't use indexes at all (based on what Compass told me 🤓 ). For now I'm using my local env, which doesn't have that much data, but we're planning to run this on a bigger DB for testing before this is merged.

cc @MarcosSpessatto @geekgonecrazy

@geekgonecrazy
Copy link
Contributor

Thanks for adding 🙏 this sort of data is important. Might be worth just creating a bunch of records. We should make it easier to fake a lot of data for optimizations like this. Having to wait on larger environments seems like adding in delays that we could avoid

@matheusbsilva137 matheusbsilva137 requested a review from a team as a code owner June 6, 2024 20:59
…nto feat/improve-engagement-dashboard-chanels
KevLehman
KevLehman previously approved these changes Jul 18, 2024
@scuciatto scuciatto modified the milestones: 6.11, 6.12 Jul 19, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jul 19, 2024
sampaiodiego
sampaiodiego previously approved these changes Jul 19, 2024
@ggazzo ggazzo modified the milestones: 6.12, 6.11 Jul 19, 2024
@sampaiodiego sampaiodiego dismissed stale reviews from KevLehman and themself via f3ca6f0 July 19, 2024 21:50
@sampaiodiego sampaiodiego merged commit 439faa8 into develop Jul 19, 2024
49 checks passed
@sampaiodiego sampaiodiego deleted the feat/improve-engagement-dashboard-chanels branch July 19, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad: core stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants