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

@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

@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

4 participants