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

Update MicroProfile Health guide to MicroProfile Health 2.0 #3129

Merged
merged 1 commit into from
Jul 8, 2019
Merged

Update MicroProfile Health guide to MicroProfile Health 2.0 #3129

merged 1 commit into from
Jul 8, 2019

Conversation

xstefank
Copy link
Member

@xstefank xstefank commented Jul 7, 2019

The associated quickstart PR is quarkusio/quarkus-quickstarts#238.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I have added a couple of comments and suggestions.

I'm not sure about the order of readiness and liveness. Readiness comes first in the lifetime of an application so I would have put it first but it kinda breaks how you present everything so feel free to ignore if it doesn't make sense.

Importing the `smallrye-health` extension directly exposes three REST endpoints:

- `/health/live` - The application is up and running.
- `/health/ready` - The application is ready to serve requests.
Copy link
Member

Choose a reason for hiding this comment

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

I would put this one first.


- `/health/live` - The application is up and running.
- `/health/ready` - The application is ready to serve requests.
- `/health` - Accumulating all health check procedures in the application.
Copy link
Member

Choose a reason for hiding this comment

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

Someone told me it was deprecated? Should we talk about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The qualifier is, not the endpoint, at least not in 2.0 up to my knowledge.

- `/health/ready` - The application is ready to serve requests.
- `/health` - Accumulating all health check procedures in the application.

To check that smallrye-health extension is working as expected:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To check that smallrye-health extension is working as expected:
To check that the `smallrye-health` extension is working as expected:

following CDI qualifiers:

- `@Liveness` - the liveness check accessible at `/health/live`
- `@Readiness` - the readiness check accessible at `/health/ready`
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I would put this one first.


- `@Liveness` - the liveness check accessible at `/health/live`
- `@Readiness` - the readiness check accessible at `/health/ready`
- `@Health` - *deprecated* unspecified health procedure accessible at `/health`
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this one: Quarkus is not 1.0 yet so let's not advertise deprecated features.


== Negative health check procedure
NOTE: If you access `http://localhost:8080/health` you will get back both checks.
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I would remove that.

Copy link
Member Author

Choose a reason for hiding this comment

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

/health is still present in MP Health 2.0. There is a possibility that it will be extended in the future. I don't think it's deprecated so I would keep the note for now.


In this section we create another health check procedure which simulates a connection to
an external service provider such as a database. For simplicity reasons, we only determine
More information about which health check procedures should be used when is detailed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
More information about which health check procedures should be used when is detailed
More information about which health check procedures should be used in which situation is detailed

== Negative health check procedures

In this section, we extend our `Database connection health check` with the option of
stating that our application is not ready to process requests as underlying database
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stating that our application is not ready to process requests as underlying database
stating that our application is not ready to process requests as the underlying database

it isn't influenced by the readiness checks.

As we shouldn't leave this application with a readiness check in a DOWN state and
because we are running Quarkus dev mode you can add `database.up=true` in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
because we are running Quarkus dev mode you can add `database.up=true` in
because we are running Quarkus in dev mode you can add `database.up=true` in

done by using the `withData(key, value)` method of the health check response
builder API.

Let's create new health check procedure `org.acme.health.DataHealthCheck`:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Let's create new health check procedure `org.acme.health.DataHealthCheck`:
Let's create a new health check procedure `org.acme.health.DataHealthCheck`:

@xstefank
Copy link
Member Author

xstefank commented Jul 7, 2019

@gsmet Thanks for the review. I've incorporated everything but the ordering suggestion. In my view:

  • liveness resembles more the original intent of health checks
  • the app can be "live" and "not ready", but not the other way around so I would say that liveness comes first in the lifetime of the app
  • if liveness check fails the app is restarted which is more critical from the point of the view of the app
  • kubernetes documentation also lists liveness first but for some reason, the spec does otherwise :)

In the end, for me, liveness and readiness are different use-cases so the order doesn't really matter to me. But for this PR, it would mean that I need to rewrite it again because it doesn't make sense to have a database connection (current Readiness example) as a liveness check.

Does it make sense, please?

@gsmet
Copy link
Member

gsmet commented Jul 8, 2019

@xstefank yes, thanks, merging!

@gsmet gsmet merged commit 86ce144 into quarkusio:master Jul 8, 2019
@gsmet gsmet added this to the 0.19.0 milestone Jul 8, 2019
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.

None yet

2 participants