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

🎨 Enhances setup to remediate accumulation of messages in socketio exchange #5341

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Feb 16, 2024

What do these changes do?

The problem reported in incident 48 (see related issues) indicate that the socketio exchange is overloaded with messages that are not attended. Those seem to be produced by those queued from the web-server's emit call. Most of those calls do not actually need to be queued because the client is directly connected to that server!

  • 🎨 Disabled queue when message was directly delivered to the client to avoid filling socketio exchange with messages going to and back to the server
  • 🎨stopped gathering the emit coroutines since the latter is not designed to run concurrently. Signature now only excepts one message at a time.
  • ♻️ Unified common code under _safe_emit
  • ♻️ Simplified test mocks

Related issue/s

How to test

  • Manual exploratory testing shows no more accumulation in my local deploy

Dev Checklist

DevOps Checklist

@pcrespov pcrespov added the a:webserver issue related to the webserver service label Feb 16, 2024
@pcrespov pcrespov self-assigned this Feb 16, 2024
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4206404) 87.3% compared to head (06d2c48) 80.3%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5341      +/-   ##
=========================================
- Coverage    87.3%   80.3%    -7.0%     
=========================================
  Files        1321     542     -779     
  Lines       54147   27179   -26968     
  Branches     1172     202     -970     
=========================================
- Hits        47290   21851   -25439     
+ Misses       6607    5278    -1329     
+ Partials      250      50     -200     
Flag Coverage Δ
integrationtests 54.2% <77.4%> (-10.7%) ⬇️
unittests 86.5% <74.1%> (+1.3%) ⬆️

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

Files Coverage Δ
...rc/simcore_service_webserver/payments/_socketio.py 100.0% <100.0%> (ø)
...simcore_service_webserver/projects/projects_api.py 84.3% <100.0%> (ø)
...rc/simcore_service_webserver/socketio/_handlers.py 85.8% <100.0%> (-0.2%) ⬇️
...r/src/simcore_service_webserver/socketio/server.py 100.0% <100.0%> (ø)
...tifications/_rabbitmq_exclusive_queue_consumers.py 89.7% <88.8%> (+0.9%) ⬆️
...src/simcore_service_webserver/socketio/messages.py 93.5% <81.8%> (-6.5%) ⬇️

... and 920 files with indirect coverage changes

@pcrespov pcrespov added this to the Schoggilebe milestone Feb 16, 2024
@pcrespov pcrespov marked this pull request as ready for review February 16, 2024 13:37
@pcrespov pcrespov changed the title WIP: 🎨 Enhances setup to remediate accumulation of messages in socketio exchange 🎨 Enhances setup to remediate accumulation of messages in socketio exchange Feb 16, 2024
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

very nice thanks!

Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks! let observe how this behaves in master 👍

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

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

Thanks a lot

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

👍 looking forward to this. drop me a ping when it's merged and deployed to master and I will verify how ti works

@pcrespov pcrespov enabled auto-merge (squash) February 16, 2024 14:29
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@pcrespov pcrespov merged commit 734e40e into ITISFoundation:master Feb 16, 2024
55 checks passed
jsaq007 pushed a commit to jsaq007/osparc-simcore that referenced this pull request Feb 20, 2024
jsaq007 pushed a commit to jsaq007/osparc-simcore that referenced this pull request Feb 22, 2024
@pcrespov pcrespov deleted the fix/sio-slow-consuming-follow-up branch March 5, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants