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

refactor(omnichannel): spare few Room.find on requestRoom method #32363

Merged
merged 14 commits into from
Jul 11, 2024

Conversation

ggazzo
Copy link
Member

@ggazzo ggazzo commented May 2, 2024

OPI-22

Proposed changes (including videos or screenshots)

Changing the order in which things happen can help avoid testing the same things multiple times.

Previously, we would test if the system had an agent online, then find who was online, and then assign once again. These actions end up being repetitive, making the system difficult to understand and consuming CPU.

We also avoid calling the same collection multiple times to update different fields on the same document, thus optimizing messages sent to the database.

Database operations( find/update) GET http://localhost:3000/api/v1/livechat/room?token=${visitor.token}
88 versus 76

With that, the order of events has changed a bit, but I believe it's not something problematic.

old:

 run livechat.applyRoomRestrictions 
 run livechat.onCheckRoomApiParams 
 run livechat.checkDefaultAgentOnNewRoom 
 run livechat.beforeRoom 
 run livechat.newRoom 
 run afterSaveMessage 
 run beforeGetMentions 
 run beforeGetMentions 
 run beforeSendMessageNotifications 
 run livechat.beforeInquiry 
 run livechat.beforeDelegateAgent 
 run livechat.chatQueued 
 run livechat.afterInquiryQueued 
 run renderNotification 
 run livechat.beforeRouteChat

new


 run livechat.applyRoomRestrictions
 run livechat.onCheckRoomApiParams
 run livechat.checkDefaultAgentOnNewRoom
 run livechat.beforeDelegateAgent
 run livechat.beforeRoom
 run livechat.newRoom
 run afterSaveMessage
 run beforeGetMentions
 run beforeGetMentions
 run beforeSendMessageNotifications
 run livechat.beforeInquiry
 run livechat.afterInquiryQueued
 run livechat.chatQueued
 run renderNotification

Issue(s)

          /\      |‾‾| /‾‾/   /‾‾/
     /\  /  \     |  |/  /   /  /
    /  \/    \    |     (   /   ‾‾\
   /          \   |  |\  \ |  (‾)  |
  / __________ \  |__| \__\ \_____/ .io

     execution: local
        script: create-room-omnichannel.js
        output: Prometheus remote write (http://localhost:9090/api/v1/write)

     scenarios: (100.00%) 1 scenario, 1 max VUs, 1m0s max duration (incl. graceful stop):
              * default: 1 looping VUs for 30s (gracefulStop: 30s)


     █ setup

     data_received..................: 2.1 MB 70 kB/s
     data_sent......................: 243 kB 8.1 kB/s
     http_req_blocked...............: avg=7.13µs  min=1µs    med=2µs     max=5.51ms   p(90)=3µs     p(95)=3µs
     http_req_connecting............: avg=2.68µs  min=0s     med=0s      max=3.22ms   p(90)=0s      p(95)=0s
     http_req_duration..............: avg=24.9ms  min=4.76ms med=31.63ms max=100.54ms p(90)=46.93ms p(95)=49.92ms
       { expected_response:true }...: avg=24.9ms  min=4.76ms med=31.63ms max=100.54ms p(90)=46.93ms p(95)=49.92ms
     http_req_failed................: 0.00%  ✓ 0         ✗ 1200
     http_req_receiving.............: avg=52.31µs min=27µs   med=47µs    max=736µs    p(90)=68µs    p(95)=76µs
     http_req_sending...............: avg=12.8µs  min=7µs    med=12µs    max=86µs     p(90)=16µs    p(95)=19µs
     http_req_tls_handshaking.......: avg=0s      min=0s     med=0s      max=0s       p(90)=0s      p(95)=0s
     http_req_waiting...............: avg=24.83ms min=4.72ms med=31.56ms max=100.48ms p(90)=46.83ms p(95)=49.87ms
     http_reqs......................: 1200   39.979253/s
     iteration_duration.............: avg=49.92ms min=875ns  med=48.9ms  max=107.63ms p(90)=57.58ms p(95)=63.55ms
     iterations.....................: 600    19.989627/s
     vus............................: 1      min=1       max=1
     vus_max........................: 1      min=1       max=1


running (0m30.0s), 0/1 VUs, 600 complete and 0 interrupted iterations
default ✓ [======================================] 1 VUs  30s

after:

          /\      |‾‾| /‾‾/   /‾‾/
     /\  /  \     |  |/  /   /  /
    /  \/    \    |     (   /   ‾‾\
   /          \   |  |\  \ |  (‾)  |
  / __________ \  |__| \__\ \_____/ .io

     execution: local
        script: create-room-omnichannel.js
        output: Prometheus remote write (http://localhost:9090/api/v1/write)

     scenarios: (100.00%) 1 scenario, 1 max VUs, 1m0s max duration (incl. graceful stop):
              * default: 1 looping VUs for 30s (gracefulStop: 30s)


     █ setup

     data_received..................: 3.2 MB 106 kB/s
     data_sent......................: 365 kB 12 kB/s
     http_req_blocked...............: avg=5µs     min=1µs    med=2µs     max=5.56ms   p(90)=2µs     p(95)=3µs
     http_req_connecting............: avg=2.55µs  min=0s     med=0s      max=4.6ms    p(90)=0s      p(95)=0s
     http_req_duration..............: avg=16.55ms min=4.1ms  med=19.01ms max=109.15ms p(90)=29.89ms p(95)=32.56ms
       { expected_response:true }...: avg=16.55ms min=4.1ms  med=19.01ms max=109.15ms p(90)=29.89ms p(95)=32.56ms
     http_req_failed................: 0.00%  ✓ 0         ✗ 1804
     http_req_receiving.............: avg=46.38µs min=23µs   med=44µs    max=156µs    p(90)=60µs    p(95)=66.84µs
     http_req_sending...............: avg=11.78µs min=5µs    med=11µs    max=302µs    p(90)=15µs    p(95)=16.84µs
     http_req_tls_handshaking.......: avg=0s      min=0s     med=0s      max=0s       p(90)=0s      p(95)=0s
     http_req_waiting...............: avg=16.49ms min=4.05ms med=18.94ms max=109.08ms p(90)=29.84ms p(95)=32.49ms
     http_reqs......................: 1804   60.071478/s
     iteration_duration.............: avg=33.24ms min=584ns  med=31.89ms max=123.58ms p(90)=39.34ms p(95)=43.2ms
     iterations.....................: 902    30.035739/s
     vus............................: 1      min=1       max=1
     vus_max........................: 1      min=1       max=1


running (0m30.0s), 0/1 VUs, 902 complete and 0 interrupted iterations
default ✓ [======================================] 1 VUs  30s

Steps to test or reproduce

Further comments

https://rocketchat.atlassian.net/browse/CORE-440

Copy link
Contributor

dionisio-bot bot commented May 2, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented May 2, 2024

⚠️ No Changeset found

Latest commit: 641ac7d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.65%. Comparing base (4aa6d58) to head (641ac7d).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32363      +/-   ##
===========================================
- Coverage    56.66%   56.65%   -0.01%     
===========================================
  Files         2504     2501       -3     
  Lines        55524    55484      -40     
  Branches     11442    11433       -9     
===========================================
- Hits         31462    31434      -28     
+ Misses       21376    21369       -7     
+ Partials      2686     2681       -5     
Flag Coverage Δ
e2e 56.46% <ø> (-0.01%) ⬇️
unit 72.21% <ø> (-0.04%) ⬇️

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

@ggazzo ggazzo force-pushed the refactor/requestroom branch from c3e7598 to eac2d8b Compare May 2, 2024 15:32
@ggazzo ggazzo force-pushed the refactor/requestroom branch from a62c7eb to 4050861 Compare July 9, 2024 20:06
@ggazzo ggazzo changed the title chore: prevent multiple room finds during requestRoom method chore(omnichannel): spare few Room.find on requestRoom method Jul 9, 2024
@ggazzo ggazzo changed the title chore(omnichannel): spare few Room.find on requestRoom method refactor(omnichannel): spare few Room.find on requestRoom method Jul 10, 2024
@ggazzo ggazzo marked this pull request as ready for review July 10, 2024 13:28
@ggazzo ggazzo requested review from a team as code owners July 10, 2024 13:28
apps/meteor/lib/callbacks.ts Outdated Show resolved Hide resolved
@ggazzo ggazzo force-pushed the refactor/requestroom branch from 4050861 to 705cbca Compare July 10, 2024 22:38
@ggazzo ggazzo force-pushed the refactor/requestroom branch from 2c6f7aa to 2dce3f5 Compare July 11, 2024 20:36
@ggazzo ggazzo added this to the 6.11 milestone Jul 11, 2024
@ggazzo ggazzo merged commit 7745bc3 into develop Jul 11, 2024
53 checks passed
@ggazzo ggazzo deleted the refactor/requestroom branch July 11, 2024 21:24
gabriellsh added a commit that referenced this pull request Jul 12, 2024
…/removeUsers

* 'develop' of github.com:RocketChat/Rocket.Chat:
  refactor(omnichannel): replace create and find by findAndUpdate (#32773)
  chore: Bump apps engine 1.44-alpha (#32774)
  refactor(client): Remove usage of `React.FC` type (#32760)
  refactor(Livechat): `Livechat/Message/markdown.js` -> TS (#32295)
  refactor(omnichannel): spare few Room.find on requestRoom method (#32363)
  refactor:  `dispatchInquiryPosition` being called multiple times (#32767)
  fix: SAML "Overwrite user fullname" setting is not being honored (#32628)
  fix(Omnichannel): Use Correct components on ChatInfo (#32592)
@jessicaschelly jessicaschelly added stat: QA assured Means it has been tested and approved by a company insider and removed stat: needs testing labels Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants