feat: add syncing_status query parameter to eth/v1/node/health #5790
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
force-pushed
the
nflaig/health-syncing-status
branch
from
c43687c
to
1bc571b
Compare
nflaig
force-pushed
the
nflaig/health-syncing-status
branch
from
1bc571b
to
0cbccc2
Compare
| @@ -66,10 +66,18 @@ export function getNodeApi( | |||
| return {data: sync.getSyncStatus()}; | |||
| }, | |||
|
|
|||
| async getHealth(_req, res) { | |||
| async getHealth(options, _req, res) { | |||
There was a problem hiding this comment.
Uhmm I didn't noticed that request and response objects where being leaked to the API implementation. I see it was introduced by #4994 but why not keep the interaction in the Fastify's response object in packages/api/src/beacon/server/node.ts?
There was a problem hiding this comment.
Looked at this as well but the only "clean way" to achieve this without too many type hacks is approach used in #3349 but in this case the getHealth type is incorrect, it doesn't actually return a res.response of type number.
If you think passing response object to API implementation is not desired we should update types accordingly and use the other approach to implementing this.
There was a problem hiding this comment.
@nazarhussain any opinions here?
There was a problem hiding this comment.
Passing res directly to API handler is a pattern which does not meant it's leaking anything.
We can define a data structure for the implementation of API endpoints, that way we can handle the status code at the server implementation side.
So all implementation handlers should return;
{
body: unknown;
status?: number;
}
And if status is not present then we consider 200 as default.
There was a problem hiding this comment.
Health API is really the only one right now that needs to modify the status code.
The interface of response is limited to setting the status code
lodestar/packages/api/src/interfaces.ts
Line 27 in 7b5fc63
There was a problem hiding this comment.
In that case we can make the response status code structure as optional. e.g.
() => T | {body: T, status; number}
This will require no refactor on API implementation side and minimum change on the server side.
There was a problem hiding this comment.
True, that could be a good pattern to give some API implementations more control over the response. Maybe we want to allow to set null explicitly as body as well, that way we can more closely follow the spec for APIs that should not return a json body (see #5790 (comment)).
Proposed type would look like this then
() => T | {body: T | null, status: number}There was a problem hiding this comment.
So will you make the change in this PR or separate?
There was a problem hiding this comment.
Better to do it in this PR, allows to remove the workaround in tests, should reduce overall diff
There was a problem hiding this comment.
Request and response objects are no longer passed to API implementation. After looking into updating return type to something like () => T | {body: T | null, status: number} it does not look that clean and requires too many type checks in other place unrelated to health API.
Better to address in a separate PR.
| >; | ||
| return { | ||
| ok: false, | ||
| status: err.status, |
There was a problem hiding this comment.
Assuming this was just missed previously as the code here is not really type-safe, didn't find a quick solution to fix that but we should always set status in client response.
| } | ||
|
|
||
| if (err instanceof TimeoutError) { | ||
| return { | ||
| ok: false, | ||
| status: HttpStatusCode.INTERNAL_SERVER_ERROR, |
There was a problem hiding this comment.
This status code is kinda wrong but 408 would also be wrong as it would indicate a server timeout whereas this error happens due to client timeout.
If server doesn't respond within client timeout we can assume a server issue, setting 500 here is the most accurate. Alternative would be to just throw the error here but this makes the error logs less informative if a timeout happens.
| @@ -26,7 +26,7 @@ export function getFetchOptsSerializer<Fn extends (...args: any) => any, ReqType | |||
| return { | |||
| url: urlFormater(req.params ?? {}), | |||
| method: routeDef.method, | |||
| query: req.query, | |||
| query: Object.keys(req.query ?? {}).length ? req.query : undefined, | |||
There was a problem hiding this comment.
Fixes appending '?' to URL if query params are omitted, e.g. /eth/v1/node/health instead of /eth/v1/node/health?.
I would assume most/(all) HTTP servers will ignore this anyways, but it might be misleading if req.url is logged on the server.
|
Would like to get @nazarhussain thoughts on #5790 (comment) first before we merge this. Putting as draft for now. Thanks for all the reviews :) |
| query: options?.syncingStatus !== undefined ? {syncing_status: options.syncingStatus} : {}, | ||
| }), | ||
| parseReq: ({query}) => [{syncingStatus: query.syncing_status}], | ||
| schema: {query: {syncing_status: Schema.Uint}}, |
There was a problem hiding this comment.
I would recommend moving the range check in the schema.
There was a problem hiding this comment.
Do you have an example API that does this? Based on current types this is not possible as far as I can see, we only support pre-defined schemas
lodestar/packages/api/src/utils/schema.ts
Line 25 in a127714
We enforce basic validation through schema but the error response and logging is not ideal in my opinion
JSON API response
Schema validation error
[
{
"instancePath": "/syncing_status",
"schemaPath": "#/properties/syncing_status/minimum",
"keyword": "minimum",
"params": {
"comparison": ">=",
"limit": 0
},
"message": "must be >= 0"
}
]API error
{
"statusCode": 400,
"error": "Bad Request",
"message": "Invalid syncing status code: 600"
}Error log
Schema validation error
Eph 0/2 1.126[rest] error: Req req-o getHealth error querystring/syncing_status must be >= 0
Error: querystring/syncing_status must be >= 0
at defaultSchemaErrorFormatter (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/context.js:108:10)
at wrapValidationError (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/validation.js:228:17)
at validate (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/validation.js:157:16)
at preValidationCallback (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/handleRequest.js:92:25)
at handler (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/handleRequest.js:76:7)
at handleRequest (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/handleRequest.js:24:5)
at runPreParsing (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/route.js:569:5)
at next (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/hooks.js:179:7)
at handleResolve (/home/nico/projects/ethereum/lodestar/node_modules/fastify/lib/hooks.js:196:5)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
API error
Eph 0/0 5.017[rest] warn: Req req-9 getHealth failed reason=Invalid syncing status code: 600
In both cases as it is right now, it is better to use API error. Schema validation could be made more flexible but there are not that many values in the spec that enforce things like minimum / maximum.
|
Noticed something else while testing, right now we always return a json body in the response. > curl -s http://localhost:9596/eth/v1/node/health
{}In case of success, this is due to default behavior if no returnType is defined lodestar/packages/api/src/utils/server/genericJsonServer.ts Lines 51 to 55 in a127714 And in case of errors, we return them in response body as well > curl -s http://localhost:9596/eth/v1/node/health?syncing_status=600 | jq
{
"statusCode": 400,
"error": "Bad Request",
"message": "Invalid syncing status code: 600"
}At least for the error response, I think this is even a good thing as just returning the status code is not that helpful. |
|
#5790 (comment) has been addressed now by updating the handler. > curl -s http://localhost:9596/eth/v1/node/healthNo longer returns a response body. Errors will still return a response body with more error details. |
|
|
||
| export type ServerApi<T extends Record<string, APIClientHandler>> = { | ||
| [K in keyof T]: ( | ||
| ...args: [...args: Parameters<T[K]>, req?: GenericRequestObject, res?: GenericResponseObject] | ||
| ...args: [...args: Parameters<T[K]>, opts?: GenericOptions] |
There was a problem hiding this comment.
I removed req / res but added generic opts instead, we have API implementations that pass additional options, like publishBlock. Might not be a bad thing in general to allow this as API implementations are called internally as well.
|
@nazarhussain Thanks for suggesting an implementation on how we could resolve the issue of setting the status code. This is really similar to what I attempted as well locally but it creates too much noise in my opinion just for solving an issue of a single API. I think if we want to implement a solution that gets rid of all custom route handlers, e.g. getState or getStateProof API To achieve this we would have to give the API implementation the option to return a "raw" response, no modification at all in generic handler, skip calls like I went for solution now which only modifies the health API and does some minor clean up of other things but I really want to keep the diff of this PR smaller. Another refactoring PR might make sense which addresses more than just the requirements of getHealth API or we could just revisit this when implementing #5128. |
|
I don't see the change as noise rather a feature to make the API implementation flexible. It's upto us we want that feature, and either we want that feature in this PR or a separate PR. I would not suggest to remove the support for custom handlers, it's a flexibility that we may need anytime if not now. |
|
I looked at this for like 4 hours, tried different type and different places how to modify the type. Nothing seemed like a good solution to me, for example having to add this I would like a solution that addresses more than just the status code issue of health API. For this PR, I feel like 2 type casts with comments that explain why it is done seems alright to me (health API is just a bit of an outlier). It also keeps the changes this of this PR isolated to health API, I noticed that forwarding request for example has unwanted side-effects as we override the options some API implementations like Also considering that health API is broken at the moment (always returns 200) and there are now tests that ensure it works correctly reduces the risk of a regression due to missing type safety by casting the types. |
Shouldn't remove support for them but rather make them less needed. Still need to consider the benefit of this but having the API implementation in a single place seems beneficial to me. |
|
🎉 This PR is included in v1.10.0 🎉 |
Motivation
Closes #5702
Description
syncing_statusquery parameter toeth/v1/node/health