Node.js implementation - Githubissues

valdemon commented 6 years ago

Hi, at the beginning thanks for sharing your great work on microservices health check subject!

Here's the initial version of the Node.js support: https://www.npmjs.com/package/healthchecks-api. Evaluation, review and any feedback would be welcome!

Best regards

HootAdam commented 6 years ago

Hi @valdemon. Thanks. We will have a look and give the framework a try.

HootAdam commented 6 years ago

Hi @valdemon, I downloaded and ran the integration today and it seems to work well with the MGE which is great 😄. I was noticing something though that I am not sure I understand. It appears in the demo that Service-2 shows up as a WARN on http://localhost:9000/#/status/http/demo-app:3000 but when i navigate into Service-2 at http://localhost:9000/#/status/http/demo-app:3000/traverse/service-2 it appears as a CRIT. It seems like this is change in the way we report downstream failure as we don't change the value (ie CRIT to WARN) at the different levels. Can you give us a bit of background about how this is happening and why?

valdemon commented 6 years ago

Hi @HootAdam , I've introduced aditional semantic of not critical service dependency. It's a configuration option for a service dependency, see: https://github.com/Bauer-Xcel-Media/node-healthchecks-api/blob/master/test/integration/conf/demo-app.yml#L25. As a use case you can imagine an app, say an API gateway, which aggregates several services and when one which is not critical for the main business is down, I wouldn't like to report the whole App as CRIT (red) cause the main business can still be operating. This is an option though, so one can use it or not. Does it make sense for you?

HootAdam commented 6 years ago

Yeah I think this make sense for your use case and since it is an additional config, I don't see any harm. Is there any way you can highlight this more in the README as added functionality as it isn't in the spec?

I'm going to take a look more at the code and will also have a look at the service /status/... responses.

valdemon commented 6 years ago

It's actually already mentioned in https://www.npmjs.com/package/healthchecks-api#functionality. Maybe not highlighted enough, I'll add some code snippet there, or extend the spec of options a little bit.

HootAdam commented 6 years ago

Yeah I see it there, one thing I'd like to highlight is it is an implementation that is specific to this library, and not part of others. Just so people don't get confused and assume it is part of other language implementations.

valdemon commented 6 years ago

Ok, got the point. Will add that as soon as I'm at the keyboard. Currently writing to you with mobile ;)

valdemon commented 6 years ago

@HootAdam could you please check the note, before I'll publish a new version:

NOTE

The critical/non-critical dependency atribute is an additional (optional) semantic of this module only.

Health Checks API does not specify such.

Classifying a particular dependency as non-critical (critical: false attribute of a dependency configuration) results in reporting it being in a WARN state at the dependency owner level, when the dependency is reported being in either WARN or CRIT state at its own level.

Example configuration for non-critical dependency:
checks:
  - name: service-2
    critical: false
    check: http
    url: http://service-2:3002
By default all dependencies are classified as critical.

HootAdam commented 6 years ago

@valdemon Yes that looks great

valdemon commented 6 years ago

@HootAdam: Released in a meantime - https://www.npmjs.com/package/healthchecks-api

HootAdam commented 6 years ago

@valdemon - Things are looking good. 😄 Before we add your implementation to the list in this repo, there are a few things we should fix/clarify:

Each service in the integration tests has a check called self-check but when I try to curl /status/self-check I get the following error ["CRIT",{"description":"Unknow status endpoint","result":"CRIT","details":"Status endpoint does not exist: /status/self-check"}] - Also sub note here, there is a spelling mistake in the error message. Unknow -> should be Unknown
It took me a bit to understand that the meta data values for the service /status/about endpoint seem to come from both the service.yml and the package.json files. This is fine, but could probably be explained in the README in more detail. From looking at the code, it seems like the service.yml file is the preferable location for these fields?
The about endpoint appears to be missing the statsLinks and logsLinks values from the spec https://hootsuite.github.io/health-checks-api/#status-about-get
Small thing, but the services in the integration test all have the same description Service-1 awesome service instead of the correct number in the description.
The host field is meant to give the name of the host (server, vm...) serving the app/service. This is helpful when you have many workers for the same service and use a load balancer to access them. It allows you to see a specific host in the pool is having issues and then know how to access it for troubleshooting. I may be wrong but it appears in the code that you are building a public URL for the service?
I tried running the unit tests by installing dependencies with npm install and then ran npm test but all tests fail with and error message like this

  - Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /Users/adam.arsenault/projects/node-healthchecks-api/test/unit/checks/mongo.test.js:33
    const testStatusChange = async (event, expected, connectCallTimes = 1, initialStatus) => {
                                   ^
    SyntaxError: Unexpected token (

      at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:403:17)

Any ideas how to fix this?

valdemon commented 6 years ago

Hi @HootAdam , thanks for the review! Will explain/check/correct all these points hopefully in next hours (currently busy with my daily basis tasks).

Just a fast hint for the last point - please note that for now the module requires nodejs >= 8.0.0, which is set in the package.json and emphasized with the REDADME.md badge. We could babelize the stuff when the support for previous node versions would be considered as a must, but, as you know, this brings a nice additional bunch of dependencies ;)

valdemon commented 6 years ago

@HootAdam - I've added you as a collaborator to the project, if you don't mind. Please take a look at the linked PR which covers your listed above issues. Once merged the new release will be created automatically in NPM.

valdemon commented 6 years ago

@HootAdam thanks again for the review. Released: https://www.npmjs.com/package/healthchecks-api/v/0.1.5

HootAdam commented 6 years ago

@valdemon Thanks :) Here is the PR that adds your Node implementation to the list: https://github.com/hootsuite/health-checks-api/pull/7

valdemon commented 6 years ago

Closing the issue. Pleasure to cooperate with you @HootAdam!

hootsuite / health-checks-api

Node.js implementation #6