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

The /client.config endpoint must query the services health endpoint #1599

Closed
paulodiniz opened this issue Apr 20, 2021 · 5 comments · Fixed by #1799
Closed

The /client.config endpoint must query the services health endpoint #1599

paulodiniz opened this issue Apr 20, 2021 · 5 comments · Fixed by #1799
Assignees

Comments

@paulodiniz
Copy link
Contributor

We must query the health endpoint for each service returned on the controller api call to check the status

@paulodiniz
Copy link
Contributor Author

paulodiniz commented Apr 23, 2021

After a call with @lucapette we came up the following proposal

The return /components stays as it is now

{
  "components": [
    "api-communication",
    "media-resolver",
    "integration-webhook",
    "api-admin",
    "api-websocket",
    "frontend-chat-plugin",
    "frontend-ui",
    "sources-chat-plugin",
    "sources-facebook",
    "sources-google",
    "sources-twilio"
  ]
}

The /client.config transforms the output, but it doesn't check the status

{
  "components":  {
     "sources-facebook": {},
     "sources-google": {}
  }
}

Benefits: By removing the enabled field on the return, we would remove the overhead of having and checking a service healthcheck for each component

What do you think?

cc @lucapette @chrismatix @AitorAlgorta

@chrismatix
Copy link
Contributor

How would the output of the status command work then? Currently, it shows all components and whether or not they are running.

While I think this is a fast way to make this work, I would re-iterate the proposal I made on Slack. Instead of dumbing down the endpoint, we can make a better distinction between apps (user runnables in our cluster) and components (things you can configure in the airy.yaml i.e. a source).

So to make /client.config useful for both use cases we could have /components return something like this:

{
  "sources-facebook-connector": {
    "enabled": true,
    "component": "sources-facebook"
  },
  "sources-facebook-events-router": {
    "enabled": true,
    "component": "sources-facebook"
  },
  "api-communication": {
    "enabled": true,
    "component": "api-communication"
  }
}
  • The component identifier can be set as a Kubernetes annotation on the deployments/statefulsets
  • The frontend can very easily .reduce on the component identifier to get the view that it needs

It's also worth considering renaming enabled to running because it more accurately reflects the check that the endpoint is doing.

Since we are already going for introspection it makes more sense to me to follow through with it.

@AitorAlgorta
Copy link
Contributor

it’s easier to find for us the right thing if the component name is they key, like it was in the beginning

"source-facebook": {
    "enabled": true,
    "healthy": false
}

@chrismatix
Copy link
Contributor

@AitorAlgorta that's true but long term it makes more sense if the endpoint reflects the state of the system more precisely. Say for instance you want to display exactly which apps are failing and why in the UI.

Plus transforming the above payload is as simple as:

Object.keys(payload).reduce((result, key) => {
    const {healthy, enabled, component} = payload[key];
    return {
        ...result,
        [component]: {
            enabled,
            healthy: result[component] ? result[component].healthy && healthy : healthy
        }
    }
}, {})

By keeping the response generic it will be easier to adapt to new requirements on the client.

@AitorAlgorta
Copy link
Contributor

yeah I am good with both options actually. I understand that this one makes more sense and that one extra step to have the desired structure is not a big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants