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.

Sign up for GitHub

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

Jump to bottom

feat: healthcheck #401

Merged
merged 44 commits into from
Dec 12, 2023
Merged

feat: healthcheck #401

merged 44 commits into from
Dec 12, 2023

Conversation

frisonisland
Copy link
Contributor

@frisonisland frisonisland commented Dec 7, 2023 •
edited
Loading

Goal

The goal is to introduce the three health check endpoints (/health, /health/live, /health/ready) that should be available on every Carbonio service.

Health check implementation

  • Implemented endpoint/routing with HealthServlet
  • Implemented HealthUseCase to aggregate readiness/liveliness based on different possible service healthchecks
  • Implemented DatabaseServiceDependency to check database status. Has options to avoid keep polling connections (polling interval) and live + ready is based on the same check on connection availability.

Testing

  • added test containers dependency
  • updated JettyServerFactory to allow setting filters and listeners
  • Tested health check with Consul in production environment

frisonisland and others added 17 commits December 6, 2023 12:33
@frisonisland
feat: add /health/live servlet
6e4f10c 
- adds liveliness endpoint for healthcheck
@frisonisland
feat: add DatabaseHealthCheck and HealthService
bced0a6 
- adds DatabaseHealthCheck to check db is live and ready + first tests
- adds HealthService to check on all registered HealthChecks
@keshavbhatt
feat: make DbConnection AutoClosable
e5d1402
@keshavbhatt
chore: add negative test cases + refactor DatabaseHealthCheck
fee72e9
@keshavbhatt
chore: javadoc and refactor
5eae073 
- java doc HealthCheck interface
@keshavbhatt
chore: refactor
c3c3c9f
@frisonisland
test: HealthServletTest do not check JSON body yet
3635a8d
@frisonisland
test: HealthServletTest add MariaDB container
4c914c2
@frisonisland
test: HealthServletTest use container user password
8f2c947
@frisonisland
feat: HealthService add isLive() + tests
92da412
@frisonisland
feat: HealthServlet add ready/live routing
cf4ac91 
- adds also DbPool startup in HealthServletTest
@frisonisland
feat: DatabaseService check db connection for both live and ready
f4091f0 
- add tests for DatabaseService on live
@frisonisland
test: HealthServlet test 200/500
18c5247 
- add test for HealthServlet when db started/not started
@frisonisland
feat: add /health endpoint with dependencies
834cc7d 
- adds HealthResponse POJO for health JSON response
- document DbPool getDatabaseConnection instance-level method
- small refactors in methods naming and variables
@frisonisland
refactor: rename HealthService.java to HealthUseCase
311dd27
@frisonisland
refactor: rename DatabaseService.java.java to DatabaseServiceDependency
4f33d74
@frisonisland
refactor: DatabaseServiceDependency check ready/live after interval
ae27519 
- update HealthServletTest to use mariadb 10.4.31
@frisonisland frisonisland marked this pull request as ready for review December 7, 2023 16:38
@frisonisland frisonisland requested a review from a team as a code owner December 7, 2023 16:38
@keshavbhatt
chore: update javadoc
165d809
@keshavbhatt
fix: refactor DatabaseServiceDependency and tests
5697344 
- fix doCheckStatus to return correct status(was returning true always)
- simplify canConnectToDatabase and make it private
- align tests with changes
@keshavbhatt
chore: refactor HealthResponse
21dc82e 
- improve builder
- add javadoc
@keshavbhatt
chore: refactor HealthServlet
59e3bcc 
- add logging
- simplify response creation
- handle unknown routes (return 404)
- improve test readability
@ppolito-zextras ppolito-zextras self-requested a review December 11, 2023 11:46
@frisonisland
chore: avoid declaring jna for all projects
489fa28 
- found jna is needed by zmconfigd Jython and no others, directly. Was updated beacuse needed by TestContainers
@frisonisland
chore: use new healthcheck in service
0ed7828
@frisonisland frisonisland marked this pull request as ready for review December 11, 2023 14:24
@ppolito-zextras ppolito-zextras removed their request for review December 12, 2023 09:58
keshavbhatt
keshavbhatt requested changes Dec 12, 2023
View reviewed changes
Copy link
Member

@keshavbhatt keshavbhatt left a comment

Choose a reason for hiding this comment

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

I just ask if it is okay to rely on this endpoint in production to suggest if mailbox is healthy. I suggest to wait for addition of other required service dependencies.

packages/appserver-service/carbonio-mailbox.hcl Show resolved Hide resolved
@frisonisland frisonisland merged commit 237ee66 into devel Dec 12, 2023
@frisonisland frisonisland deleted the feat/healthcheck branch December 12, 2023 16:34
@frisonisland frisonisland mentioned this pull request Dec 15, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonersivri sonersivri sonersivri approved these changes

@matteobaglini matteobaglini matteobaglini approved these changes

@keshavbhatt keshavbhatt keshavbhatt approved these changes

@mbarto mbarto Awaiting requested review from mbarto

@AlessioCoser AlessioCoser Awaiting requested review from AlessioCoser

@zextrasGiovanniDefacci zextrasGiovanniDefacci Awaiting requested review from zextrasGiovanniDefacci

@aheeva-yuliya aheeva-yuliya Awaiting requested review from aheeva-yuliya

@ppolito-zextras ppolito-zextras Awaiting requested review from ppolito-zextras

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@frisonisland @sonersivri @matteobaglini @keshavbhatt @ppolito-zextras