Skip to content

Conversation

@henchaves
Copy link
Member

Description

The idea of this PR is to implement a WebSocket to provide a channel through which the backend can send the ML Worker status without any request from the front.

This improvement will be divided into two phases:
1- Implement WebSocket on back and frontend, send the worker status periodically from Spring app, and update Vue components to use websocket instead of store. (CURRENT PR)
2- Make the backend only send a message to the channel when the worker status change, instead of sending it every N seconds. Also, send the last worker status whenever a new subscriber appears.

Related Issue

GSK-1199 (available on Linear)

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

@linear
Copy link

linear bot commented Jun 16, 2023

GSK-1199 Create websocket to get worker status

This issue is a improvement of GSK-1099

@henchaves henchaves requested review from Googleton, andreybavt and kevinmessiaen and removed request for andreybavt June 16, 2023 08:04
@henchaves henchaves marked this pull request as ready for review June 19, 2023 08:21
…ker-status

# Conflicts:
#	frontend/src/views/main/project/TestSuite.vue
#	frontend/src/views/main/project/modals/RunTestSuiteModal.vue
@henchaves henchaves requested a review from andreybavt June 20, 2023 11:27
@andreybavt
Copy link
Contributor

@henchaves on my end I don't get notifications when running Giskard in production mode (in docker).

Also when testing in dev mode I had to use the UI for a bit before I started receiving ML Worker connected/disconnected messages:

  • I installed a blank instance
  • connected the worker - no notification ❌
  • disconnected the worker - no notification ❌
  • created project, connected/disconnected worker - no notification ❌
  • ran the onboarding code and connected/disconnected worker - no notification ❌
  • opened the test suite and connected/disconnected worker - got notification ✅

@henchaves
Copy link
Member Author

@henchaves on my end I don't get notifications when running Giskard in production mode (in docker).

Also when testing in dev mode I had to use the UI for a bit before I started receiving ML Worker connected/disconnected messages:

  • I installed a blank instance
  • connected the worker - no notification ❌
  • disconnected the worker - no notification ❌
  • created project, connected/disconnected worker - no notification ❌
  • ran the onboarding code and connected/disconnected worker - no notification ❌
  • opened the test suite and connected/disconnected worker - got notification ✅

@andreybavt, you will only get notifications if you are interacting with a page (component) that imports the ML Worker state

@andreybavt
Copy link
Contributor

Well, it should be enabled on a global level (except for the login page). It's important to know if a worker gets disconnected even if you're not immediately using it

@henchaves
Copy link
Member Author

@andreybavt, I added worker notifications for all components inside Main.vue. Can you try again the use cases you mentioned? I tried it on my side, and it's working now.

@andreybavt
Copy link
Contributor

@henchaves , sure I'll retest now. Did you try in Docker too?

@henchaves
Copy link
Member Author

@andreybavt not yet. Let me try that

@andreybavt
Copy link
Contributor

I also spotted an unhandled corner case: when you logout (available with a paid license) you don't unsubscribe from the websocket events and thus still continue receiving the notifications

image

@henchaves
Copy link
Member Author

@andreybavt, everything should be working now (including using Docker)

@andreybavt
Copy link
Contributor

@henchaves ok, I'm checking again and then it's good to merge

@andreybavt
Copy link
Contributor

@henchaves , one other thing I noticed is that the security question isn't solved in this PR. You can connect to this WS endpoint and send/receive STOMP messages from any client without being authenticated

@andreybavt
Copy link
Contributor

Once the build passes it should be OK to merge

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

24.1% 24.1% Coverage
0.0% 0.0% Duplication

@andreybavt andreybavt merged commit affdde3 into main Jun 26, 2023
@Hartorn Hartorn deleted the feature/gsk-1199-create-websocket-to-get-worker-status branch September 13, 2023 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants