feat(dynamic config): `/debug/client_config` handler to examine the current value of `ClientConfig` #8400

ppca · 2023-01-19T16:03:47Z

accidentally deleted the prev branch with PR here: #8379 (comment)

nikurt · 2023-01-19T16:11:17Z

core/chain-configs/src/updateable_config.rs

@@ -10,7 +10,7 @@ use std::sync::{Arc, Mutex};
 /// When initializing sub-objects (e.g. `ShardsManager`), please make sure to
 /// 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)]


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.

created ticket: https://pagodaplatform.atlassian.net/browse/ND-283 will also add TODO in code.

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)

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 🤔

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.

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.

Custom serializer: #8401

nikurt · 2023-01-19T16:11:55Z

chain/jsonrpc/src/lib.rs

@@ -1535,6 +1558,7 @@ pub fn start_http(
            )
            .service(debug_html)
            .service(display_debug_html)
+            .service(web::resource("/client_config").route(web::get().to(client_config_handler)))


As suggested by @wacban , let's move this to /debug/client_config and link to it from the /debug page.

Done.
Debug page:

the client_config I get from clicking into the client_config link:

wacban

Congratulations on your first PR in Pagoda :)

wacban · 2023-01-19T17:57:57Z

chain/jsonrpc/src/lib.rs

+            .service(
+                web::resource("/debug/client_config").route(web::get().to(client_config_handler)),
+            )


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.

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

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.

…Value

ppca requested a review from a team as a code owner January 19, 2023 16:03

ppca requested review from mzhangmzz, wacban and nikurt January 19, 2023 16:03

nikurt reviewed Jan 19, 2023

View reviewed changes

wacban approved these changes Jan 19, 2023

View reviewed changes

ppca added 4 commits January 23, 2023 07:07

add endpoint /client_config to dump runtime client config

83ca59f

formatting

5668eab

move /client_config to /debug/client_config and link it in /debug page

9f2fe56

add TODO for implementing Serialize and Deserialize for MutableConfig…

422f524

…Value

ppca force-pushed the xiangyi/ND-273 branch from 659ea22 to 422f524 Compare January 23, 2023 15:07

add to changelog

24d9eed

ppca merged commit 1f7fb08 into master Jan 23, 2023

ppca deleted the xiangyi/ND-273 branch January 23, 2023 17:26

ppca mentioned this pull request Jan 23, 2023

feat: better Serializer for MutableConfigValue #8421

Merged

nikurt changed the title ~~Xiangyi/nd 273~~ feat(dynamic config): /debug/client_config handler to examine the current value of ClientConfig Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dynamic config): `/debug/client_config` handler to examine the current value of `ClientConfig` #8400

feat(dynamic config): `/debug/client_config` handler to examine the current value of `ClientConfig` #8400

ppca commented Jan 19, 2023

nikurt Jan 19, 2023

ppca Jan 19, 2023

ppca Jan 19, 2023 •

edited

Loading

ppca Jan 19, 2023

ppca Jan 19, 2023

ppca Jan 19, 2023 •

edited

Loading

ppca Jan 19, 2023

nikurt Jan 19, 2023

ppca Jan 19, 2023

wacban left a comment

wacban Jan 19, 2023

ppca Jan 19, 2023

wacban Jan 19, 2023

feat(dynamic config): /debug/client_config handler to examine the current value of ClientConfig #8400

feat(dynamic config): /debug/client_config handler to examine the current value of ClientConfig #8400

Conversation

ppca commented Jan 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppca Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppca Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wacban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(dynamic config): `/debug/client_config` handler to examine the current value of `ClientConfig` #8400

feat(dynamic config): `/debug/client_config` handler to examine the current value of `ClientConfig` #8400

ppca Jan 19, 2023 •

edited

Loading

ppca Jan 19, 2023 •

edited

Loading