Adding Health endpoint #836
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #836 +/- ##
==========================================
+ Coverage 60.62% 60.66% +0.04%
==========================================
Files 215 218 +3
Lines 23099 23204 +105
==========================================
+ Hits 14003 14076 +73
- Misses 7947 7973 +26
- Partials 1149 1155 +6 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
do we have any tests for this new endpoint ? |
otherview
force-pushed
the
pedro/health_endpoint
branch
from
b124448
to
710e6c6
Compare
otherview
force-pushed
the
pedro/health_endpoint
branch
from
710e6c6
to
ed0097c
Compare
api/admin/admin.go
Outdated
There was a problem hiding this comment.
The admin endpoints were buckled in this package and reformatted to follow the same pattern as other endpoints
api/admin_server.go
Outdated
There was a problem hiding this comment.
Potentially, this can live in the init functions of the thor and thor-solo ?
|
I got this when I sync a new node, is it expected? Edit: I just saw previous comment. IMO it is healthy if its syncing like this curl http://localhost:2113/admin/health{"healthy":false,"blockIngestion":{"bestBlock":"0x00014fe5a9e48a02268655e8206bb4cbcc95a1ae0d1f5f4360f697f02077c727","bestBlockIngestionTimestamp":"2024-11-04T10:49:16.06031Z"},"chainSync":false}
|
|
Edit: nvm: looks like it has 503, wasn't seeing it in testing locally |
|
Yeah, @darrenvechain is right, it's going to be useless as far as the AWS ALB is concerned if it doesn't return a 5XX status code when the node is unhealthy. I understand this is unfortunate and counter intuitive, but unless you build it like that I have to implement some sort of wrapper, which is the exact thing we're trying to replace. Tagging @otherview for visibility. Note that I mentioned this constraint in the related issue. |
I understand what you mean, it's the regular and expected behaviour to sync to the lastest block. This is the standard I've seen in other nodes, as it helps node operators to know when the node is 100% ready to process blockchain operations. |
Yeah I got you covered :) It's returning a 503 when |
otherview
force-pushed
the
pedro/health_endpoint
branch
from
885ddd5
to
17c2ce9
Compare
|
I would suggest to only leverage best block's timestamp in this function. |
health/health.go
Outdated
| func New(repo *chain.Repository, p2p *comm.Communicator, timeBetweenBlocks time.Duration) *Health { | ||
| return &Health{ | ||
| repo: repo, | ||
| timeBetweenBlocks: timeBetweenBlocks + delayBuffer, | ||
| p2p: p2p, | ||
| } | ||
| } |
There was a problem hiding this comment.
I refactored the health service to accept components and work in a pull fashion.
I think it looks better, thanks guys 🙏
api/admin/health/health_api.go
Outdated
| } | ||
| } | ||
|
|
||
| acc, err := h.healthStatus.Status(maxTimeBetweenSlots, minPeerCount) |
There was a problem hiding this comment.
Added http query params so it's possible to access different tolerance in health constraints.
| type Status struct { | ||
| Healthy bool `json:"healthy"` | ||
| BlockIngestion *BlockIngestion `json:"blockIngestion"` | ||
| ChainBootstrapped bool `json:"chainBootstrapped"` |
There was a problem hiding this comment.
ChainBootstrapped can be removed considering BlockIngestion?
Description
A health check service that lives inside the node as an API endpoint.
I think it's a good illustrative start for a PR, there are a few tech choices made that I consider would be worth discussing, such as:
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist: