Implement new RPC endpoint: health check #3729
Conversation
pavitthrap
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #3729 +/- ##
===========================================
- Coverage 0.18% 0.18% -0.01%
===========================================
Files 306 306
Lines 278640 281246 +2606
===========================================
Hits 512 512
- Misses 278128 280734 +2606 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I notice that the two error paths for this endpoint are using unallocated HTTP status codes in addition to an application-level error message body. I think perhaps we should consider using a well-defined HTTP status code and perhaps make the message body more detailed (e.g. inform the caller to check /v2/neighbors to diagnose why the node can't report health data).
I decided to go ahead with the unallocated HTTP status codes based on a discussion on the old PR for this same issue. @wileyj do you have any opinions on how to proceed here? |
i see what you're doing - i tend to agree with @jcnelson here in that we shouldn't define unused http status codes (even though the endpoint errors don't specifically line up to a defined status). I think a generic |
| @@ -2091,6 +2102,27 @@ pub trait Requestable: std::fmt::Display { | |||
| fn make_request_type(&self, peer_host: PeerHost) -> HttpRequestType; | |||
| } | |||
|
|
|||
| // TODO: DRY up from PoxSyncWatchdog | |||
There was a problem hiding this comment.
It's fine if you want to address this in this PR :)
There was a problem hiding this comment.
I think that might not be straightforward given the structure of the codebase. Let me know if it is straightforward though.
There was a problem hiding this comment.
LGTM! Only minor things need to be done before this can be merged. Thanks for getting this over the finish line, @pavitthrap!
|
|
|
Closing this PR for now. It's stale, and would require some effort to revive. If someone wants to take it on, they can always revive it later. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
(peer_version & 0x000000ff) <= network_epoch(as opposed to(peer_version & 0x000000ff) >= network_epoch).Applicable issues
Feedback needed
get_valid_initial_neighborsI am passing in the peer_version. Alternatively I could pass in the network_epoch.handle_get_healthin rpc.rs, I need feedback on how I computedlast_processed_burn_heightandburnchain_height. The former should be the last processed burn height (so less than the actual burn chain tip if ibd=true. The latter should be the actual current height of the burnchain. I want to mimic how ibd is calculated inpox_sync_wait.Checklist
docs/rpc/openapi.yamlandrpc-endpoints.mdfor v2 endpoints,event-dispatcher.mdfor new events)