api down management #12

geniusit · 2020-04-29T14:01:56Z

I don't use the same linter than you. Sorry. It would be nice to find a way to lint in the same way than you.

geniusit · 2020-04-29T14:09:40Z

I have some questions about the application.
First, why the onResponse is always undefined ? I removed it ...
Second, how the handleError is called? I don't find any link in the code... I want to understand it.
Third, I have some health checks needs and I don't want if it's beter to do PR to your official lib or is it beter to make the lib to life of my fork ...

My backend API uses https://quarkus.io/guides/microprofile-health

May be we can have a chat about that ?

Thank you

geniusit · 2020-04-30T11:50:26Z

So I ve understand the whole mecanism. I noticed that you pass undefined as the fourth parameter of the begin function.

katiawheeler · 2020-04-30T20:23:37Z

@geniusit sorry for the delayed response!

Yes, the initial implementation was missing a handling for when the original API went back up - it should have an onResponse handler passed as the fourth parameter of begin. Checking over your code now!

katiawheeler

Please let me know if you have any questions/concerns on the edits or if I can explain anything further!

src/pages/StatusPage/index.tsx

katiawheeler · 2020-04-30T20:29:35Z

src/pages/StatusPage/index.tsx

+ public handleResponse = (api: Api, response: Response) => {
+   console.log('handleResponse' + response);
+   const apis = [...this.state.apis];
+   const index = apis.findIndex(stateApi => stateApi.api === api);


Object equality in JS won't work here - has to check a property or it will always be false.

I my implementation I did it like this :

const index = apis.findIndex(stateApi => stateApi.api.endpoint === api.endpoint);

Should I change here also ?

const index = apis.findIndex(stateApi => stateApi.api === api);

is the way that you do...

Yeah, it was probably an oversight on my part - it should probably be updated everywhere. I shall note where to update to make sure we get them all 😄

katiawheeler · 2020-04-30T20:34:57Z

src/pages/StatusPage/index.tsx

+
+   this.setState({
+     apis,
+     hasError: hasError,


You can just pass hasError. JS/TS will automatically map it.

Suggested change

hasError: hasError,

hasError,

Ok, didn't know it ...

It's a neat ES6 trick! https://alligator.io/js/object-property-shorthand-es6/

katiawheeler · 2020-04-30T20:38:57Z

src/pages/StatusPage/index.tsx

+     hasError: false,
+   });
+
+   await begin(this.props.apis, this.handleResponse, this.props.interval);


So, we still need the handleError and the handleResponse separately.

handleError will get called specifically if there is an error.
handleResponse will get called if there's not an error (the API is not down or if it came back up).

The logic needs to be:

handleError - set error state
handleResponse - check if previous error state - if there is, iterate through and see if that API is resolved. If no previous error state, do nothing (it's a good response, nothing to update).

Check out the last line on the table: https://github.com/katiawheeler/healthy#functions

So I did some change.
According to your impl Im agree that we still need a way to differentiate Response and Error.
I rename Response with Success.
I propose you to have the handler like this :

handleSuccess -> onSuccess

handleError -> onError

handleDown -> onDown

Can we rename onDown to onApiDown just so it's clear what is down?

src/pages/StatusPage/index.tsx

katiawheeler · 2020-04-30T20:41:52Z

src/services/index.ts

+ 
 /** Begins the chain of API calls */
-export const begin = async (
-  apis: Api[],
-  onError: Handler,
-  interval?: number,
-  onResponse?: Handler
-) => {
-  const currentInterval = interval ? interval : 30000;
-
-  apis.forEach(async a => {
-    await makeCall(a, onError, onResponse || undefined);
-  });
-
-  /** Set the timeout and do it again */
-  setTimeout(
-    async () => await begin(apis, onError, currentInterval, onResponse || undefined),
-    currentInterval
-  );
+export const begin = async (apis: Api[], onResponse: Handler, interval?: number) => {
+ const currentInterval = interval ? interval : 30000;
+
+ apis.forEach(async a => {
+   await makeCall(a, onResponse);
+ });
+
+ /** Set the timeout and do it again */
+ setTimeout(async () => await begin(apis, onResponse, currentInterval), currentInterval);
 };
-
+ 
 /** Make the actual fetch and return a Response object */
-const makeCall = async (api: Api, onError: Handler, onResponse?: Handler) => {
-  await fetch(api.endpoint)
-    .then(apiResponse => handleResponse(api, apiResponse, onError, onResponse || undefined))
-    // tslint:disable-next-line
-    .catch(error => console.log('unhandled', error));
+const makeCall = async (api: Api, onResponse: Handler) => {
+ await fetch(api.endpoint)
+   .then(apiResponse => handleResponse(api, apiResponse, onResponse))
+   // tslint:disable-next-line
+   .catch(error => handleResponse(api, error, onResponse));
 };
-
+ 
 /**
- * Handles the Response
- */
-const handleResponse = (api: Api, response: Response, onError: Handler, onResponse?: Handler) => {
-  const apiResponse: ApiResponse = {
-    code: response.status,
-    message: response.statusText,
-  };
-
-  if (!response.ok) {
-    onError(api, apiResponse);
-    return;
-  }
-
-  if (onResponse) {
-    onResponse(api, apiResponse);
-  }
+* Handles the Response
+*/
+const handleResponse = (api: Api, response: Response, onResponse: Handler) => {
+ const apiResponse: ApiResponse = {
+   code: response.status,
+   message: response.statusText,
+ };
+
+ onResponse(api, apiResponse);
 };
-
-export default { begin, makeCall, handleResponse };
+ 
+export default { begin, makeCall, handleResponse };


Please revert

I fully reworked it according to your main idea

katiawheeler · 2020-04-30T20:43:19Z

Also, @geniusit important to note - I am almost done refactoring this library to utilize hooks. I can incorporate your changes in that refactor, or if you'd like I can finish the refactor and you can make the changes using hooks. Whatever you'd like to do I'm fine with 😄

katiawheeler · 2020-05-04T13:24:08Z

@geniusit just checking in that you saw my previous comment and didn't need any further help!

geniusit · 2020-05-04T13:26:03Z

@katiawheeler just workin on it ! Send you message in the day.
I did a really nice job, I'll send you message in a few hours

geniusit · 2020-05-04T13:46:33Z

Here is the final result of my HealthCheck :

I use https://quarkus.io/guides/microprofile-health

I got this kind of response from my backend :

{
    "status": "DOWN",
    "checks": [
        {
            "name": "Micro Service - Crossing series ",
            "status": "UP"
        },
        {
            "name": "Micro Service - Crossing Scheduler",
            "status": "UP"
        },
        {
            "name": "Micro Service - Vehicle Regex",
            "status": "DOWN",
            "data": {
                "Message": "RESTEASY004655: Unable to invoke request: java.net.ConnectException: Connection refused: connect"
            }
        },
        {
            "name": "Kafka cluster",
            "status": "UP"
        }
    ]
}

It's my first React project and Im very happy of the result. Learning React with react-healthy is perfect.
The question is : when to call onError() and when to call onResponse() ?
Does onError means 500, 503, ... or does onError means that service is Down ?
What does onError means ?

Here is my onBegin signature :

export const begin = async (apis: ApiWithResponse[], onResponse: Handler, interval?: number) => {

In my onResponse Handler I manage everything. Im looking for a nicer way to do it.

I changed to fetch like this :

await fetch(api.api.endpoint)
   .then(apiResponse => handleResponse(api, apiResponse, onResponse))
   // tslint:disable-next-line
   .catch(error => handleError(api, onResponse));
};

BTW I added material-ui. I like it ;)
With that component : https://material-ui.com/components/expansion-panels/

I don't know hooks. I will learn it. I don't know the best way to use react-healthy. I copy/paste some files of your project into mine.
For now I continue to use react-health only to fetch the data ... Is is worth to continue to use react-healthy or copy everything into my project. That the question ... :)

geniusit · 2020-05-04T20:23:58Z

I did a new commit. Some things to say :
I've tested with StatusPage component and it works well.
However with Healthy component I got the following error :

Uncaught (in promise) TypeError: onDown is not a function
at handleDown (index.js:347)
at index.js:316

I will look on it. Im not yet familiar with the Healthy component.

Also, in Handler is change the signature. The Response is now optional (onDown management)
I take this choice instead of creating two separate Handler.
But it implies to add strictNullCheck in tslint.json.

Also in my impl I add a way to parse the json returned by my api call.
Like this :

// Examine the text in the response
  response.json().then(function(data) {

    const apiResponse: ApiResponse = {
      code: response.status,
      message: response.statusText,
      body: data,
    };

    200 == apiResponse.code ? onSuccess(api, apiResponse) : onError(api, apiResponse);

  });

Here is my Response object definition :

export interface Response {
  /** HTTP Error Code */
  code: number;
  /** Status message */
  message: string;
  /** Body */
  body: HealthCheck;
}

export interface HealthCheck {
  status: string;
  checks: Check [];
}

interface Check {
  name: string;
  status: string;
}

export interface ApiWithResponse {
  api: Api;
  response: Response;
}

Do you want to implement it in your lib. Notice that it's tight coupling with quarkus health check and this is why I think we shouldn't implement it in the react-healthy lib.

Now we have a clear separation of the 3 scenarios : success / failure / down.

I hope that it doesn't break the Healthy component too much.
I will try to look on it asap.

You decide how we merge my code in the master branch. Im very interested by React hooks :)

katiawheeler · 2020-05-04T21:15:10Z

src/pages/StatusPage/index.tsx

-  };
+ public handleError = (api: Api, response: Response) => {
+  const apis = [...this.state.apis];
+  const index = apis.findIndex(stateApi => stateApi.api === api);


Should check api.endpoint equality

katiawheeler · 2020-05-04T21:15:19Z

src/pages/StatusPage/index.tsx

-  public componentDidMount = async () => {
-    const apis = this.props.apis.map(api => ({ api, response: null, hasError: false }));
+  const apis = [...this.state.apis];
+  const index = apis.findIndex(stateApi => stateApi.api === api);


Should check api.endpoint equality

katiawheeler · 2020-05-04T21:15:34Z

src/pages/StatusPage/index.tsx

      apis,
-      hasError: false,
+      hasError: hasError,


Can shorten to hasError,

katiawheeler · 2020-05-04T21:15:59Z

src/pages/StatusPage/index.tsx

+  this.setState({
+    ...this.state,
+    apis,
+    hasError: hasError,


Can shorten to hasError,

katiawheeler · 2020-05-04T21:30:47Z

So, it looks like your onDown essentially replaces the onError - my initial thinking behind onError is that if the call fails in some way (a typical HTTP Error being thrown) or if the response doesn't match our "success" criteria (your code == 200, etc. checks), it would throw the onError to alert the user that the API is down or unreachable. I am fine renaming it to onApiDown if that seems to make more sense, but I don't necessarily think we need both of those functions.

katiawheeler · 2020-05-04T21:31:21Z

Love the material design, by the way! Very slick!

katiawheeler · 2020-05-04T21:33:55Z

Also, I'll try to pull your branch either today or tomorrow and make sure that everything looks smoothly. Then I'll go ahead and merge it in and update the hooks refactor.

To answer your question of whether or not to implement your HealthCheck work into this lib, I think a better way to handle it would be to modularize the response type and open it up to allow users (yourself included) to pass their own custom response by implementing a base interface. I'll also take a stab at this when I refactor. I think this would be a good thing to have.

geniusit · 2020-05-11T11:11:48Z

So, it looks like your onDown essentially replaces the onError - my initial thinking behind onError is that if the call fails in some way (a typical HTTP Error being thrown) or if the response doesn't match our "success" criteria (your code == 200, etc. checks), it would throw the onError to alert the user that the API is down or unreachable. I am fine renaming it to onApiDown if that seems to make more sense, but I don't necessarily think we need both of those functions.

geniusit · 2020-05-11T11:12:10Z

Also, I'll try to pull your branch either today or tomorrow and make sure that everything looks smoothly. Then I'll go ahead and merge it in and update the hooks refactor.

To answer your question of whether or not to implement your HealthCheck work into this lib, I think a better way to handle it would be to modularize the response type and open it up to allow users (yourself included) to pass their own custom response by implementing a base interface. I'll also take a stab at this when I refactor. I think this would be a good thing to have.

geniusit · 2020-05-11T12:14:45Z

So I ve removed onApiDown handler. Indeed it makes the same than onError handler.
Basically I wanted to differentiate both onError / onApiDown because having only one handler implies this :

the response of the handleResponse become optional parameter :

const handleResponse = (api: Api, onSuccess: Handler, onError: Handler, response?: Response) => {

a if statement has to be made in the handleResponse :

if(response){ ...

in the handleResponse (consumer) a ternary condition :

response != null ? apis[index].response = response : apis[index].response = null;

I let you decide what you prefer. Me I prefer to have 3 separates Handler because it evics to have if statement in the handleResponse... What do you think ?

geniusit · 2020-05-11T12:16:02Z

BTW the Healthy component is still broke... I got the "We are currently experiencing issues with our Api1 service" message even if the API is up. Please could you check?
Thank you !

api down management

4af5e5b

rename function

e88f2bf

katiawheeler requested changes Apr 30, 2020

View reviewed changes

onSuccess - onError - onDown

70a1a4b

katiawheeler reviewed May 4, 2020

View reviewed changes

katiawheeler requested changes May 4, 2020

View reviewed changes

geniusit force-pushed the enhancement/api_down_management branch from cb684ce to 5e57f15 Compare May 11, 2020 11:11

geniusit closed this May 11, 2020

geniusit reopened this May 11, 2020

DEWEER Bertrand (EXT EXTIA) added 3 commits May 11, 2020 13:12

quick changes due to comment

5e57f15

rename onDown to onApiDown

6aba28c

remove onApiDown

d02b1bf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api down management #12

api down management #12

geniusit commented Apr 29, 2020

geniusit commented Apr 29, 2020

geniusit commented Apr 30, 2020 •

edited

katiawheeler commented Apr 30, 2020

katiawheeler left a comment

katiawheeler Apr 30, 2020

geniusit May 4, 2020

geniusit May 4, 2020

katiawheeler May 4, 2020

katiawheeler Apr 30, 2020

geniusit May 4, 2020

katiawheeler May 4, 2020

katiawheeler Apr 30, 2020

katiawheeler Apr 30, 2020

geniusit May 4, 2020

katiawheeler May 4, 2020

katiawheeler Apr 30, 2020

geniusit May 4, 2020

katiawheeler commented Apr 30, 2020

katiawheeler commented May 4, 2020

geniusit commented May 4, 2020

geniusit commented May 4, 2020 •

edited

geniusit commented May 4, 2020 •

edited

katiawheeler May 4, 2020

katiawheeler May 4, 2020

katiawheeler May 4, 2020

katiawheeler May 4, 2020

katiawheeler commented May 4, 2020

katiawheeler commented May 4, 2020

katiawheeler commented May 4, 2020

geniusit commented May 11, 2020

geniusit commented May 11, 2020

geniusit commented May 11, 2020

geniusit commented May 11, 2020

api down management #12

Are you sure you want to change the base?

api down management #12

Conversation

geniusit commented Apr 29, 2020

geniusit commented Apr 29, 2020

geniusit commented Apr 30, 2020 • edited

katiawheeler commented Apr 30, 2020

katiawheeler 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

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

katiawheeler commented Apr 30, 2020

katiawheeler commented May 4, 2020

geniusit commented May 4, 2020

geniusit commented May 4, 2020 • edited

geniusit commented May 4, 2020 • edited

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

katiawheeler commented May 4, 2020

katiawheeler commented May 4, 2020

katiawheeler commented May 4, 2020

geniusit commented May 11, 2020

geniusit commented May 11, 2020

geniusit commented May 11, 2020

geniusit commented May 11, 2020

geniusit commented Apr 30, 2020 •

edited

geniusit commented May 4, 2020 •

edited

geniusit commented May 4, 2020 •

edited