Skip to content
This repository was archived by the owner on Jan 7, 2019. It is now read-only.

Write HealthCheck route #154

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Write HealthCheck route #154

wants to merge 4 commits into from

Conversation

Arlevoy
Copy link
Contributor

@Arlevoy Arlevoy commented Oct 2, 2018

No description provided.

@bam-bot-tech
Copy link

Warnings
⚠️

health-check-route.s.md: Does not seem to be included in the root readme

⚠️

health-check-route.s.md: Does not seem to be included in the root summary

Generated by 🚫 dangerJS


# Why

In order to monitor correctly their environments, some Cloud services require a HealthCheck route which returns a status depending on how the API is running.
Copy link
Member

Choose a reason for hiding this comment

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

could apply to any backend service


## Checks

- [ ] Do a call to every DB used by the API
Copy link
Member

Choose a reason for hiding this comment

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

make a call / database


- [ ] Do a call to every DB used by the API
- [ ] Send a 2xx status code status in case of API running correctly and 5xx status code if not
- [ ] Do the less data usage DB calls
Copy link
Member

Choose a reason for hiding this comment

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

Make -> why?

return res.status(200).send({ status: 200, message: "OK" });
}

nextFetchingDate = Date.now() + TIME_CHECKING_INTERVAL;
Copy link
Member

Choose a reason for hiding this comment

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

maybe put nextFetchingDate before the catch

- [ ] Do a call to every DB used by the API
- [ ] Send a 2xx status code status in case of API running correctly and 5xx status code if not
- [ ] Do the less data usage DB calls
- [ ] Include a timestamp in order not to reduce the number of successive calls
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can remove this check?

```javascript
app.get("/health", async (req, res) => {
try {
await findAllDataCollectors(); // DataCollectors ~ 100 entries
Copy link
Member

Choose a reason for hiding this comment

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

you can be more generic, like RDS.getAll

}

nextFetchingDate = Date.now() + TIME_CHECKING_INTERVAL;
await Promise.all([AppsDynamoRepo.getDynamoHealth(), getHealth()]);
Copy link
Member

Choose a reason for hiding this comment

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

mayeb more generic in your examples -> what is getHealth

@Arlevoy Arlevoy force-pushed the health-check-route branch from 467572a to cd24cf7 Compare October 9, 2018 11:51
## Checks

- [ ] Make a call to every database used by the API
- [ ] Send a 2xx status code status in case of API running correctly and 5xx status code if not
Copy link
Contributor

Choose a reason for hiding this comment

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

Send a 2xx status code status if the API is running correctly, 5xx status code if not


- [ ] Make a call to every database used by the API
- [ ] Send a 2xx status code status in case of API running correctly and 5xx status code if not
- [ ] Make the less data usage database calls: the health check route is likely to be called very often in short period of time
Copy link
Contributor

Choose a reason for hiding this comment

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

Make database calls retrieving as little data as possible


## Examples

In the examples below the API is concentrating calls to one database RDS and one DynamoDB
Copy link
Contributor

Choose a reason for hiding this comment

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

concentrating? Did you mean making?


- There is a call to one of the database but not the other
- The call is using too much data
- There is a 503 if the DB is down
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain why 503

await knex.raw("select 1+1 as result");
};

DynamoDB.getDynamoHealth = (): Promise<Array<Object>> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe remove typing as it is not used anywhere else

@Arlevoy Arlevoy force-pushed the health-check-route branch from e5e4e70 to 95ffe1f Compare October 18, 2018 07:18
@louiszawadzki
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

4 participants