feat(dynamic config): /debug/client_config handler to examine the current value of ClientConfig - #8400
Conversation
| /// pass this wrapper instead of passing a value from a single moment in time. | ||
| /// See `expected_shutdown` for an example how to use it. | ||
| #[derive(Clone)] | ||
| #[derive(Clone, Debug, Serialize, Deserialize)] |
There was a problem hiding this comment.
I would prefer to have a custom implementation for Serialize and Deserialize.
field_name and last_update are internal fields used only for metrics.
It's fine to add a TODO and come to it later.
There was a problem hiding this comment.
created ticket: https://pagodaplatform.atlassian.net/browse/ND-283 will also add TODO in code.
There was a problem hiding this comment.
I think currently there's issue with not including field_name in deserialize, because current implementation of set_metrics https://github.com/near/nearcore/blob/master/core/chain-configs/src/updateable_config.rs#L53 rely on the field_name that comes from the MutableConfigValue. If user supplies a config with an update-able field only containing the value, the set_metrics would not work. Tho all other functionalities are supposed to work if I make another fn new(value) that only intakes the value and fills in a default name and current timestamp to create the MutableConfigValue object.
If we want to only include value in serialize and deserialize, and we want the set_metrics to still work, we should first move the set_metrics functionality out of the implementations of MutableConfigValue type and get the name of the updated field from outer json, e.g. instead of taking name1 from the filed_name of {"name2": {"value": 100, "field_name": "name1", "utc_time": xxx}}, we take name2 which is the name of this mutableConfigValue field.
Serialize would be ok to only include value, but it confuses users about what to input when they change config unless we make serialize and deserialize consistent.
(Not sure if moving set_metrics out will work tho)
There was a problem hiding this comment.
but doing expected_shutdown: 123 in config.json and killall -SIGHUP neard seem to already be effective at shutting down the node. I guess that means the current default serde Deserialize already handles missing field_name, but not sure what field_name the set_metrics is using 🤔
There was a problem hiding this comment.
ok I see. so when initializing client, we initialized expected_shutdown: MutableConfigValue::new(None, "expected_shutdown"),, later when user updates their config using expected_shutdown: {a number}, only the value field of expected_shutdown is modified not the field_name, so set_metrics would also be doing alright.
So only including value field in deserialize and serialize would have no issues.
There was a problem hiding this comment.
took another look at dynamic config PR, looks like the current workflow of updating expected_shutdown does not really need a Deserializer. What happens in such workflow is: user supplies config.json, we read it in as Config type, where expected_shutdown is of type Option<BlockHeight>, then we use this value to update the value_field of client_config.expected_shutdown, which is of type MutableConfigValue<Option<BlockHeight>>.
Deserializer for MutableConfigValue currently is only useful for the /client_config endpoint, when deserializing the response of client config. In this case, the all 3 fields of MutableConfigValue are present and correct at all times, we don't need such deserializer that only takes value field. And this is a usecase where user input is not involved and we don't need to worry about users having to input field_name and last_update either.
| ) | ||
| .service(debug_html) | ||
| .service(display_debug_html) | ||
| .service(web::resource("/client_config").route(web::get().to(client_config_handler))) |
There was a problem hiding this comment.
As suggested by @wacban , let's move this to /debug/client_config and link to it from the /debug page.
There was a problem hiding this comment.
Done.
Debug page:
the client_config I get from clicking into the client_config link:
wacban
left a comment
There was a problem hiding this comment.
Congratulations on your first PR in Pagoda :)
| .service( | ||
| web::resource("/debug/client_config").route(web::get().to(client_config_handler)), | ||
| ) |
There was a problem hiding this comment.
Just looking at the other debug pages, they all go through /debug/pages/{the_thing} e.g. http://127.0.0.1:3030/debug/pages/last_blocks. Also they don't seem to be registered here individually but rather handled elsewhere. Can you make client_config consistent with the current approach and move it with the rest of the debug pages?
Hmm I'm having seconds thoughts: all other debug pages are actual html pages and they are served from within display_debug_html. Please consider using that but if it doesn't make sense feel free to keep this code as is.
There was a problem hiding this comment.
I initially wanted to do /debug/pages/client_config, but as you said that is achieved in display_debug_html. And further digging seems to suggest that all these debug/pages/{} pages in core/chain/jsonrpc/res/{}.html, for example https://github.com/near/nearcore/blob/63d77d49abf6ad5cda3ae6ad1488c33efda3b342/chain/jsonrpc/res/tier1_network_info.html. I think the actual calls to get the info are issued from this html code, which means in order to put things in display_debug_html, there's additional work to write such a html like https://github.com/near/nearcore/blob/63d77d49abf6ad5cda3ae6ad1488c33efda3b342/chain/jsonrpc/res/tier1_network_info.html, which does look like a lot of work to me :(
Thus I instead just added hyper link to core/chain/jsonrpc/res/debug.html s.t. client_config points to debug/client_config. Experience would be same for RPC endpoint users, but agree that code is not as clean.
I would be happy to create such html and serve within display_debug_html if creating such a html can be non-manual. @wacban
There was a problem hiding this comment.
Yeah, I thoughts there might be some html needed. In that case let's stick to what you have now. Thanks for the detailed explanation.
/debug/client_config handler to examine the current value of ClientConfig
accidentally deleted the prev branch with PR here: #8379 (comment)