-
Notifications
You must be signed in to change notification settings - Fork 107
feat: add runtime feature toggling via CLI and RPC #417
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.
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
Conversation
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
|
hi @MicaiahReid let me know what you think about this implementation. |
crates/cli/src/cli/mod.rs
Outdated
| Ok(()) | ||
| } | ||
| None => { | ||
| if let Some(error) = result.get("error") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we push common RPC response handling into one helper ?
| /// "params": [true] | ||
| /// } | ||
| /// ``` | ||
| #[rpc(meta, name = "setInstructionProfiling")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toggle is good can we get error codes/messages when meta is None too ?
crates/core/src/lib.rs
Outdated
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct FeatureStates { | ||
| pub instruction_profiling_enabled: bool, | ||
| pub max_profiles: usize, | ||
| pub log_bytes_limit: Option<usize>, | ||
| pub loaded_plugins: Vec<PluginInfo>, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good , maybe we can make it Default
| } | ||
| } | ||
|
|
||
| fn rpc_call( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure but maybe Don’t spawn curl Use a Rust HTTP client
| /// RPC URL of the running surfpool instance | ||
| #[arg(long = "rpc-url", default_value = "http://localhost:8899")] | ||
| pub rpc_url: String, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we enabled a bool parsed by clap.Free form strings are error prone ?
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
|
Thanks for the energy you've put into this @AvhiMaz ! I don't have a strong opinion wether or not we should have a cheatcode to achieve this at runtime, I don't believe it's been requested at this point, but if you have a use case for this, could we be building off #423 instead? Thanks, happy to breakdown specs further in the future if it's helpful 🙏 |
appreciate the clarity! i notice #423 is already merged - does that fully address what you had in mind, or are there gaps this issue should still cover? if there's additional work needed, i'd love a bit more detailed spec, would love to contribute. |
|
We should be good for now, let's reconsider this feature if we see some demand. |
Implement comprehensive feature toggling system allowing users to experiment with different feature sets at runtime without restarting.
Changes:
setLogBytesLimit, getFeatureStates
surfpool featureandsurfpool pluginforruntime management
thread-safe state updates
yes/no, on/off, 1/0)
Closes #308