8000 feat: add /client_config endpoint to view runtime client config by ppca · Pull Request #8379 · near/nearcore · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add /client_config endpoint to view runtime client config #8379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

feat: add /client_config endpoint to view runtime client config #8379

wants to merge 2 commits into from

Conversation

ppca
Copy link
Contributor
@ppca ppca commented Jan 17, 2023

add endpoint /client_config to dump runtime client config

Context

https://pagodaplatform.atlassian.net/browse/ND-273

Testing and QA

tested by starting a localnet and tested endpoint on localnet
local_client_config

@ppca ppca marked this pull request as ready for review January 17, 2023 22:55
@ppca ppca requested a review from a team as a code owner January 17, 2023 22:55
@ppca ppca requested a review from nikurt January 17, 2023 22:55
@wacban
Copy link
Contributor
wacban commented Jan 18, 2023

Please have a look at the failing tests in buildkite.
Can you make the json pretty (multiline)?

@ppca
Copy link
Contributor Author
ppca commented Jan 18, 2023

Please have a look at the failing tests in buildkite. Can you make the json pretty (multiline)?

buildkite is good now :)
I created a ticket for making the json output nice: https://pagodaplatform.atlassian.net/browse/ND-282, we should make it easy to apply to all endpoints. Looks reasonable to be a separate task.

@wacban
Copy link
Contributor
wacban commented Jan 19, 2023

@ppca n00b question here: Do you know how to actually change the config to see that it's updated in the ui that you added? I mean changing it dynamically (what Nikolay is working on) rather than stopping neard, changing it in the config file and restarting neard.

nikurt
nikurt reviewed Jan 19, 2023
View reviewed changes
core/chain-configs/src/client_config.rs
@@ -70,7 +70,7 @@ impl GCConfig {
}
}

#[derive(Clone, Serialize, Deserialize)]
#[derive(Clone, Debug, Serialize, Deserialize)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to another PR merging, please re-implement Debug and Serialize by implementing them for MutableConfigValue.

@nikurt
Copy link
Contributor
nikurt commented Jan 19, 2023

@ppca n00b question here: Do you know how to actually change the config to see that it's updated in the ui that you added? I mean changing it dynamically (what Nikolay is working on) rather than stopping neard, changing it in the config file and restarting neard.

echo '{"verbose_module": ""}' > ~/.near/log_config.json
killall -SIGHUP neard

Same for config.json. Change expected_shutdown and send SIGHUP.

8000

@wacban
Copy link
Contributor
wacban commented Jan 19, 2023

I tried killall -SIGHUP neard but it complaned that dyn_config doesn't exist. I put the expected_shutdown in dyn_config and with sighup I was able to make it work (which is a fancy way of saying that I unintentionally killed neard by setting the shutdown height too low). This however is not part of client config and so it doesn't show up when testing this PR.

@wacban
Copy link
Contributor
wacban commented Jan 19, 2023

One thing bothers me with this approach: you're making the client config a part of the general use RPC which I believe node owners typically have open to anyone. I'm not sure if all node owners will want to make their configs public. Could we make this part of the debug page instead? Node owners that care abour privacy can disable that with enable_debug_rpc = false. It may simplify the implementation as well because you'd only be adding a subpage rather than a full RPC method though I may be wrong.

@nikurt
Copy link
Contributor
nikurt commented Jan 19, 2023 •
edited
Loading

SIGHUP and dyn_congig

Before #8240 was merged, you could have dyn_config.json changed at runtime. If that file wasn't available you would get an info(?) or a warning(?) printed. But it's not an error, totally normal.

After #8240, dyn_config.json is no longer a thing. The only field that can be updated at runtime is expected_shutdown in config.json, but we can add more :)

@wacban
Copy link
Contributor
wacban commented Jan 19, 2023

After #8240, dyn_config.json is no longer a thing. The only field that can be updated at runtime is expected_shutdown in config.json, but we can add more :)

hehe, ok, I'm behind, I'll rebase on top of master and test again ;)

@nikurt
Copy link
Contributor
nikurt commented Jan 19, 2023 •
edited
Loading

privacy

None of the fields of ClientConfig are sensitive. /client_config is a handler on port 3030, which usually validators don't open. Some validators share their metrics and debug pages with us. Both /client_config and /debug/client_config would work.

Overall, I agree with @wacban, it's a good idea to be a bit more cautious, and make this a debug handler. Would be even better to add a link from /debug to the ClientConfig handler. Helps with discover-ability of this handler. Maybe we also should link /status and /metrics 🤔 but let's not go too far in this PR yet :)

@nikurt
Copy link
Contributor
nikurt commented Jan 19, 2023

And because a new handler is good news, please mention it in CHANGELOG.md

@ppca ppca closed this Jan 19, 2023
@ppca ppca deleted the xiangyi/ND-273 branch January 19, 2023 16:00
@ppca ppca mentioned this pull request Jan 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikurt nikurt nikurt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ppca @wacban @nikurt
0