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

Monitoring: add service URL manager #3134

Merged
merged 43 commits into from
Aug 11, 2024

Conversation

0oM4R
Copy link
Contributor

@0oM4R 0oM4R commented Jul 18, 2024

Updated

Description

  • Add ServiceURLManager : a class managing service URLs with liveness checking pick the first reachable endpoint for each service
  • Enhancements and Refactoring in service monitoring

Changes

  • Refactor:

    • replace ws connection on RMB and TFChain ping with http using health endpoints,
    • replace axios with fetch.
    • Renamed sendGetRequest to sendRequest
    • fix GraphQl isAlive logic
    • replace serviceUrl and serviceName with getters
    • add update method that uses URL setter
    • Make all IServiceBase classes take an optional options on their contractor, add error handling for side effect
    • Grouped all RMB properties to RMB to RMBProps
    • ping url without change the service url, by passing optional url to isAlive
  • Feat:

    • add error event handler on RMB direct client
    • add stats monitor class that monitors stats service
    • add activation monitor class that monitor activation service
    • Add serviceURLManager
      • Support silent mode settings
      • Implement constructor to handle URLManagerOptions
      • Add private handleErrorsOnSilentMode method for error handling based on silent mode
      • Implement getAvailableStack method to find reachable service URL
      • Implement getAvailableServicesStack method to fetch and store available service URLs for all services
  • Examples: update service monitor example and add serviceURlManager Example, also update example command to take the example file path

  • Fix: main and type paths in package.json

Related Issues

Documentation PR

For UI changes, Please provide the Documetation PR on info_grid

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)

0oM4R added 11 commits July 18, 2024 00:27
  - sendGetRequest to sendRequest and use fetch instead of axios.
  - replace serviceUrl and serviceName with getters
  - add update method that uses URL setter
  - Make all IServiceBase classes take an optional options on their contractor, add error handling for side effect
  -  Grouped all RMB properties to RMB to RMBProps
- Feat:
 - add stats monitor class that monitors stats service
 - add activation monitor class  that monitor activation service
it is class managing service URLs with liveness checking <pick the first reachable endpoint for each service>

- Define retries and silent mode settings
- Implement constructor to handle StackManagerOptions
- Add private handleErrorsOnSilentMode method for error handling based on silent mode
- Implement getAvailableStack method to find reachable service URL
- Implement getAvailableServicesStack method to fetch and store available service URLs for all services
…urn undefined or emptystring

remove debug lines
- change naming
- remove error on get url and return empty string in case the url is not set yet
- add errror handling in isAlive, will throw in case the service url is not set
@0oM4R 0oM4R marked this pull request as ready for review August 1, 2024 09:35
Comment on lines 18 to 22
* Updates the service with the provided parameters.
* @param {P} param - The parameter object with specific keys, should be specified on class.
*/
disconnect: () => Promise<void>;
update(param: P): void;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need this update method?

Copy link
Contributor Author

@0oM4R 0oM4R Aug 7, 2024

Choose a reason for hiding this comment

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

I guess I should replace it with normal setter, and use it here, with the reachable url

Copy link
Contributor

@Mahmoud-Emad Mahmoud-Emad left a comment

Choose a reason for hiding this comment

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

Good job, Omar

packages/monitoring/example/serviceMonitor.ts Show resolved Hide resolved
Comment on lines 4 to 15
export class GridProxyMonitor implements ILivenessChecker {
private readonly name = "GridProxy";
private url: string;
constructor(gridProxyUrl: string) {
this.url = gridProxyUrl;
private readonly _name = "GridProxy";
private _url: string;
constructor(gridProxyUrl?: string) {
if (gridProxyUrl) this.url = gridProxyUrl;
}
serviceName() {
return this.name;
public get name() {
return this._name;
}
serviceUrl() {
return this.url;
public get url() {
return this._url ?? "";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that you have multiple classes that share a significant amount of similar functionality. To improve code maintainability and reduce duplication, I suggest creating a Base class that encapsulates the common logic. Then, each of the existing classes can inherit from this Base class, allowing them to share the common behavior while still implementing any specific functionality they need.

class Base {
  get url(): string{}
  set url(name: string){}
  async isAlive(url: string): Promise<ServiceStatus> {}
  ....
}

class X extends Base {
  ping(url: string){
    ...
  }
}

class Y extends Base {
  pong(){
    ...
  }
}

packages/monitoring/src/serviceMonitor/rmb.ts Show resolved Hide resolved
Comment on lines 39 to 51
try {
const statusPromise = service.isAlive(url);

const timeoutPromise = new Promise((resolve, reject) => {
setTimeout(() => {
reject(new Error("Timeout"));
}, _timeout * 1000);
});

const result = await Promise.race([statusPromise, timeoutPromise]);
if (result instanceof Error && result.message === "Timeout") {
throw result;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try {
const statusPromise = service.isAlive(url);
const timeoutPromise = new Promise((resolve, reject) => {
setTimeout(() => {
reject(new Error("Timeout"));
}, _timeout * 1000);
});
const result = await Promise.race([statusPromise, timeoutPromise]);
if (result instanceof Error && result.message === "Timeout") {
throw result;
}
const TIME_OUT = "Timeout";
try {
const statusPromise = service.isAlive(url);
const timeoutPromise = new Promise((resolve, reject) => {
setTimeout(() => {
reject(new Error(TIME_OUT));
}, _timeout * 1000);
});
const result = await Promise.race([statusPromise, timeoutPromise]);
if (result instanceof Error && result.message === TIME_OUT) {
throw result;
}

@0oM4R 0oM4R requested a review from Mahmoud-Emad August 8, 2024 12:39
@AhmedHanafy725 AhmedHanafy725 merged commit 457da79 into development Aug 11, 2024
9 checks passed
@AhmedHanafy725 AhmedHanafy725 deleted the development_monitoring__stacks branch August 11, 2024 11:02
@0oM4R 0oM4R restored the development_monitoring__stacks branch August 13, 2024 11:15
@xmonader xmonader added this to the 2.6.0 milestone Sep 24, 2024
@0oM4R 0oM4R deleted the development_monitoring__stacks branch October 3, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants