-
Notifications
You must be signed in to change notification settings - Fork 19
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
add health check endpoint to signature-aggregator #442
Conversation
tested by putting a 30 second sleep into the signature-aggregator e2e test and then while it was sleeping did: ``` $ curl localhost:8082/health {"status":"up","details":{"signature-aggregator":{"status":"up","timestamp":"2024-08-20T01:23:14.351599017Z"}}} ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed on a call. we should just add a separate endpoint path to the existing ListenAndServe
call, rather than serving on another port, so that the health check won't start saying "healthy" until the main ListenAndServe
is actually up and running.
healthChecker := health.NewChecker( | ||
health.WithCheck(health.Check{ | ||
Name: "signature-aggregator", | ||
Check: func(context.Context) error { return nil }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is a no-op for now. Can you create a ticket and/or add todo comments here to make this health check meaningful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raised an issue here: #447
But, note, this actually already is meaningful, in that the http.ListenAndServe()
call made in the aggregator includes both the health check path and also the signature request path, so when this health check starts responding with "healthy", that's an indication that all of the paths served by that ListenAndServe()
call are ready, so it tells us that the service is ready to handle signature requests.
Why this should be merged
fixes #424
How this was tested
by putting a 30 second sleep into the signature-aggregator e2e test and then while it was sleeping did: