feat(cc-sdk): added-health-check-and-keep-alives #2

adhmenon · 2024-11-11T07:14:05Z

COMPLETES #< SPARK-574669 >

This pull request addresses

MIgration of WebSocket handling from agentx to cc-sdk.
Addition of a worker thread for keep alive and health check.
Code changes in order to make this work.
Unit tests

by making the following changes

Added a WebSocketManager which handles the web socket connection.
Used code from agentx for reconnect logic and keep-alive logic
Modified the code in cc.ts to incorporate these changes.

GIF

(With Logout/Login)

Change Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Tooling change
Internal code refactor

The following scenarios were tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

I certified that

I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

…bex-js-sdk into dev/adhmenon-SPARK-574669

adhmenon · 2024-11-11T08:43:25Z

packages/@webex/plugin-cc/__mocks__/workerMock.js

@@ -0,0 +1,15 @@
+class Worker {


Created this so that we can mock the worker thread in tests. Kept it ouside so it can be reused.

adhmenon · 2024-11-11T08:44:19Z

packages/@webex/plugin-cc/src/cc.ts

  private httpRequest: HttpRequest;
+  private webSocketManager: WebSocketManager;


Decided to add an instance of the WebSocketManager here as now we will directly call the initWebSocket method to establish a connection with the web socket and use that Promise to setup the config (shown below).

adhmenon · 2024-11-11T08:44:41Z

packages/@webex/plugin-cc/src/cc.ts

-      this.services = Services.getInstance();
+      this.webSocketManager = new WebSocketManager({webex: this.$webex});
+
+      this.services = Services.getInstance(this.webSocketManager);


Here we will inject the manager for reasons that will be explained in the Service class implementation.

adhmenon · 2024-11-11T08:45:22Z

packages/@webex/plugin-cc/src/services/core/HttpRequest.ts

-      this.webSocket.connectWebSocket({
-        webSocketUrl: subscribeResponse.body.webSocketUrl,
-      });
-    });
  }

  public async request(options: {


We only need to retain this method, the rest of them are now supported via the WebSocketManager.
Discussed with Ravi and we felt this was the best course of action.

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

adhmenon · 2024-11-11T08:48:41Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+import workerScript from './keepalive.worker';
+
+export class WebSocketManager {
+  readonly onMessage: Signal.WithData<string>;


RIght now, we will go ahead with a signal implementation, as discussed with Ravi. We will have another ticket to refactor it to using event handlers - wanted to avoid increasing the scope of this ticket as of now.

Can we add a TODO and a SPARK for this?

adhmenon · 2024-11-11T08:50:20Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+  async initWebSocket(options: {body: SubscribeRequest}): Promise<WelcomeResponse> {
+    const connectionConfig = options.body;
+    await this.register(connectionConfig);
+    return new Promise((resolve, reject) => {


Again as mentioned, this promise is for the registration flow, if the connect fails, we simply reject the promise, it works for both cases.

adhmenon · 2024-11-11T08:51:28Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+    });
+  }
+
+  async reconnect() {


This method is exposed so that we can reconnect the flow if the network is turned off and gets turned on again. It uses the keep-alive to monitor this and based on the message, it initiates a reconnect logic (found in connection-service.ts, which is taken from agent desktop).

adhmenon · 2024-11-11T08:51:57Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+
+        this.keepaliveWorker.postMessage({
+          type: 'start',
+          intervalDuration: 4000, // Keepalive interval


These are durations taken from agent desktop, open to changing them.

adhmenon · 2024-11-11T08:52:27Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+          }
+        }
+
+        if (eventData.type === 'AGENT_MULTI_LOGIN') {


Again, all have been taken from agent desktop, so if we do not require this, we can remove them.
But overall the logic is pretty much the same.

It is okay to keep this as this whole copy-paste effort is keeping the WxCC Desktop use cases in mind

adhmenon · 2024-11-11T08:54:04Z

packages/@webex/plugin-cc/src/services/core/WebSocket/keepalive.worker.js

@@ -0,0 +1,89 @@
+/* eslint-disable */
+// TODO: Try to find alternative to using Blob and script here
+const workerScript = `


As mentioned, this is a workaround, currently we export the template string and make it a Blob and use it as a worker script.
Work is being done to explore a better method for this, however to unblock we can go ahead with this workaround.
Open to any suggestions.

adhmenon · 2024-11-11T08:55:05Z

packages/@webex/plugin-cc/src/services/core/aqm-reqs.ts

@@ -2,6 +2,7 @@ import {Signal} from './Signal'; // TODO: remove and use Event Emitters after Ad
 import {Msg} from './GlobalTypes';
 import * as Err from './Err';
 import {HTTP_METHODS, WebexRequestPayload} from '../../types';
+import {WebSocketManager} from './WebSocket/WebSocketManager';


The changes in this file are simply replacing the web socket manager in the same manner in which we have aqm-notfifs inside of the the agent desktop.
It uses signals as well (logic is basically the same as agent desktop, with minor refactoring).

adhmenon · 2024-11-11T08:55:17Z

packages/@webex/plugin-cc/src/services/core/aqm-reqs.ts

    if (event.type === 'Welcome') {
      LoggerProxy.logger.info(`Welcome message from Notifs Websocket${event}`);

      return;
    }

-    if (event.keepalive) {
-      LoggerProxy.logger.info(`Keepalive from notifs${event}`);
+    if (event.keepalive === 'true') {


To record the keepalive message.

adhmenon · 2024-11-11T08:56:06Z

packages/@webex/plugin-cc/src/services/index.ts


 export default class Services {
  public readonly agent: ReturnType<typeof routingAgent>;
  private static instance: Services;

-  constructor() {
-    const aqmReq = new AqmReqs();
+  constructor(webSocketManager: WebSocketManager) {


We use DI to inject the websocketmanager from the cc.ts into the sqm-reqs. Preferred this approach over using a singleton system.

Non-blocking Q: What is the consensus behind preferring this approach over singleton?

Would we have another entity in the plugin that would reuse this services with a different webSocketManager?

adhmenon · 2024-11-11T08:56:39Z

packages/@webex/plugin-cc/test/unit/spec/cc.ts

@@ -1,12 +1,18 @@
 import 'jsdom-global/register';
 import {LoginOption, StationLogoutResponse, WebexSDK} from '../../../src/types';
+import WebSocketManager from '../../../src/services/core/WebSocket/WebSocketManager';


Tests here are mostly the same, just include some of the mocks and the blob mocking. All seem to run properly and are passing.

adhmenon · 2024-11-11T08:59:21Z

packages/@webex/plugin-cc/test/unit/spec/services/core/HttpRequest.ts

  });

-  describe('subscribeNotifications', () => {


Removed tests that were no longer required.

adhmenon · 2024-11-11T08:59:49Z

packages/@webex/plugin-cc/test/unit/spec/services/core/WebSocket/WebSocketManager.ts

+  }
+}
+
+describe('WebSocketManager', () => {


Here, I have mocked the webSocket creation in a similar manner to the way it was being done in agentx.

adhmenon · 2024-11-11T09:00:13Z

packages/@webex/plugin-cc/test/unit/spec/services/core/WebSocket/WebSocketManager.ts

+
+    await webSocketManager.initWebSocket({ body: subscribeRequest });
+
+    setTimeout(() => {


We need to set this timeout so that we cna call the onopen callback and the code flows as per usual.

adhmenon · 2024-11-11T09:00:33Z

packages/@webex/plugin-cc/test/unit/spec/services/core/WebSocket/connection-service.ts

+
+jest.mock('../../../../../../src/services/core/WebSocket/WebSocketManager');
+
+describe('ConnectionService', () => {


Mostly taken tests from the agent desktop in this case. All test cases are passing.

adhmenon · 2024-11-11T09:01:03Z

packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts

@@ -121,13 +120,13 @@ describe('AqmReqs', () => {
        req({}),
        new Promise<void>((resolve) => {
          setTimeout(() => {
-            aqm['onMessage']({
+            webSocketManagerInstance.onMessageSend(JSON.stringify({


Had to modify it in order to accommodate the latest changes.

adhmenon · 2024-11-11T09:01:19Z

packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts

            resolve();
          }, 1000);
        }),
      ]);
      expect(p).toBeDefined();
    } catch (e) {}
  });
-
-  it('should handle onMessage with Welcome event', () => {


We do not need these tests anymore, hence removed them.

packages/@webex/plugin-cc/src/services/core/WebSocket/connection-service.ts

rsarika · 2024-11-11T10:36:39Z

packages/@webex/plugin-cc/src/services/core/WebSocket/connection-service.ts

+  lostConnectionRecoveryTimeout: number;
+};
+
+const LOST_CONNECTION_RECOVERY_TIMEOUT = 20000; // 20 seconds


can be moved to config?

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

rsarika · 2024-11-11T10:58:33Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+      });
+      this.url = subscribeResponse.body.webSocketUrl;
+    } catch (e) {
+      console.error("Register API Failed", "Request to RoutingNotifs websocket registration API failed", e);


yes, please replace all the console with LoggerProxy

adhmenon · 2024-11-11T11:42:21Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+import {SUBSCRIBE_API, WCC_API_GATEWAY} from '../../constants';
+import {SubscribeResponse} from '../../config/types';
+import LoggerProxy from '../../../logger-proxy';
+import workerScript from './keepalive.worker';


Note -
This is a workaround, currently I was not able to find any way to import the script the same way we were doing in agent desktop (as that is a node server).
However this workaround can be improved (later) wherein we read from the .js script file rather than exporting the string.

adhmenon · 2024-11-11T11:43:15Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+  private forceCloseWebSocketOnTimeout: boolean;
+  private isConnectionLost: boolean;
+  private webex: WebexSDK;
+  private welcomePromiseResolve:


This was a needed change so that when we call the initWebSocket from the cc.ts, we are able to return a promise to resolve the initial registration. This differs from agentx which was calling it from the aqm-reqs and hence it could be handled via signals, however since we call it from cc.ts, I had to add this here for that.
It works fine (as shown in the GIF).

mkesavan13

First round of review

mkesavan13 · 2024-11-11T09:45:40Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

@@ -0,0 +1,166 @@
+/* eslint-disable */


You could disable the console.error -specific check alone

Suggested change

/* eslint-disable */

/* eslint no-console: "error" */

mkesavan13 · 2024-11-11T13:03:38Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+import workerScript from './keepalive.worker';
+
+export class WebSocketManager {
+  readonly onMessage: Signal.WithData<string>;


Can we add a TODO and a SPARK for this?

packages/@webex/plugin-cc/src/services/core/WebSocket/keepalive.worker.js

packages/@webex/plugin-cc/src/services/core/WebSocket/connection-service.ts

packages/@webex/plugin-cc/src/services/index.ts

adhmenon · 2024-11-12T06:51:36Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+      };
+
+      this.websocket.onmessage = (e: MessageEvent) => {
+        this.dispatchEvent(new CustomEvent('message', {detail: e.data}));


As per discussion with Ravi, I have switched over to using events here as opposed to singals. I felt this was a more straight-forward solution. Now, I am not using custom event handlers here as it was working with events, however I would like someone to point out any edge cases or issues that might arise (most things are working including reconnect logic).

adhmenon · 2024-11-12T06:52:03Z

packages/@webex/plugin-cc/src/services/core/WebSocket/connection-service.ts

+    this.isSocketReconnected = false;
+    this.isKeepAlive = false;
+
+    this.webSocketManager.addEventListener('message', this.onPing);


As mentioned, using event listeners here over signals, however this was the only change here.

adhmenon · 2024-11-12T06:53:19Z

packages/@webex/plugin-cc/src/services/core/WebSocket/connection-service.ts

+      this.dispatchConnectionEvent(true);
+    }
+
+    this.reconnectingTimer = setTimeout(this.handleConnectionLost, this.wsDisconnectAllowed);


We have a timer of 8 seconds, which closes the web socket if keep alive is false for 8 seconds (health check gets browser online status).
In that case, when keep alive is true once again, it restores the timer by simply re-registering the web socket connection (mentioned in handleSocketClose).
Logic is the same as agent desktop here.

adhmenon · 2024-11-12T06:54:28Z

NOTE
I have switched over to using custom events over signals. Please let me know if there are any problems with this.
I did a round of testing and all operations seem to be working properly (including reconnect).

adhmenon · 2024-11-12T11:42:46Z

packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts

@@ -1,7 +1,9 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import AqmReqs from '../../../../../src/services/core/aqm-reqs';
 import HttpRequest from '../../../../../src/services/core/HttpRequest';
+import {WebSocketManager} from '../../../../../src/services/core/WebSocket/WebSocketManager';


I had to add couple of more tests so coverage went above 85%. Discussed this with Ravi and I got it working... it has made the PR a little huge, however.

adhmenon · 2024-11-12T11:42:59Z

packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts

+jest.mock('../../../../../src/services/core/WebSocket/WebSocketManager');
+
+// Mock CustomEvent class
+class MockCustomEvent<T> extends Event {


Simply mocking the event class as we are now using events.

mkesavan13

Changes mostly look good to me. Just a few non-blocking questions. Approved

mkesavan13 · 2024-11-12T10:37:04Z

packages/@webex/plugin-cc/src/cc.ts

-      this.services = Services.getInstance();
+      this.webSocketManager = new WebSocketManager({webex: this.$webex});
+
+      this.connectionService = new ConnectionService(


Q: This is just an instance creation. We do not call any methods of this conneionService directly. Is my understanding correct?

Yes, but since we are setting up the event listeners on init, it will call the internal handlers.

mkesavan13 · 2024-11-12T12:06:43Z

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts

+          }
+        }
+
+        if (eventData.type === 'AGENT_MULTI_LOGIN') {


It is okay to keep this as this whole copy-paste effort is keeping the WxCC Desktop use cases in mind

mkesavan13 · 2024-11-12T12:10:21Z

packages/@webex/plugin-cc/src/services/index.ts


 export default class Services {
  public readonly agent: ReturnType<typeof routingAgent>;
  private static instance: Services;

-  constructor() {
-    const aqmReq = new AqmReqs();
+  constructor(webSocketManager: WebSocketManager) {


Non-blocking Q: What is the consensus behind preferring this approach over singleton?

Would we have another entity in the plugin that would reuse this services with a different webSocketManager?

adhmenon added 6 commits November 8, 2024 23:51

feat(cc-sdk): ported-code-for-health-check

0f5c1c8

feat(cc-sdk): fixed-broken-tests

99fc367

feat(cc-sdk): fixed-blob-issue

5665ee1

feat(byods-sdk): added-additional-methods

3cd6307

Merge branch 'Agent-Req-From-Agentx' of https://github.com/rsarika/we…

8821963

…bex-js-sdk into dev/adhmenon-SPARK-574669

feat(cc-sdk): merged-with-ravi-pr

994fe63

adhmenon commented Nov 11, 2024

View reviewed changes

packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts Outdated Show resolved Hide resolved

adhmenon commented Nov 11, 2024

View reviewed changes

feat(cc-sdk): re-added-cc-code

ff494fc

adhmenon commented Nov 11, 2024

View reviewed changes

adhmenon requested a review from rsarika November 11, 2024 09:01

adhmenon added the validated label Nov 11, 2024

rsarika reviewed Nov 11, 2024

View reviewed changes

feat(cc-sdk): added-logger-proxy

859cfd6

adhmenon requested a review from rsarika November 11, 2024 11:40

adhmenon commented Nov 11, 2024

View reviewed changes

feat(cc-sdk): added-config-values

f8923d4

mkesavan13 suggested changes Nov 12, 2024

View reviewed changes

adhmenon added 4 commits November 12, 2024 10:36

feat(cc-sdk): added-connection-instance

83652ba

feat(cc-sdk): added-event-listeners-instead-of-signals

51fbfe1

feat(cc-sdk): merged-with-main-pr

4c303f2

feat(cc-sdk): fixed-broken-tests

8d7bad7

adhmenon commented Nov 12, 2024

View reviewed changes

adhmenon requested a review from mkesavan13 November 12, 2024 06:59

feat(cc-sdk): fixed-test-coverage

22fdd15

adhmenon commented Nov 12, 2024

View reviewed changes

mkesavan13 approved these changes Nov 12, 2024

View reviewed changes

rsarika approved these changes Nov 12, 2024

View reviewed changes

rsarika merged commit 2926276 into rsarika:Agent-Req-From-Agentx Nov 12, 2024
9 of 10 checks passed

adhmenon deleted the dev/adhmenon-SPARK-574669 branch November 12, 2024 13:06

		private httpRequest: HttpRequest;
		private webSocketManager: WebSocketManager;


		await webSocketManager.initWebSocket({ body: subscribeRequest });

		setTimeout(() => {


		jest.mock('../../../../../../src/services/core/WebSocket/WebSocketManager');

		describe('ConnectionService', () => {

feat(cc-sdk): added-health-check-and-keep-alives #2

feat(cc-sdk): added-health-check-and-keep-alives #2

Conversation

adhmenon commented Nov 11, 2024 • edited Loading

COMPLETES #< SPARK-574669 >

This pull request addresses

by making the following changes

GIF

Change Type

The following scenarios were tested

I certified that

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adhmenon Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkesavan13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adhmenon commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkesavan13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adhmenon commented Nov 11, 2024 •

edited

Loading

adhmenon Nov 11, 2024 •

edited

Loading