Skip to content

Commit 741341f

Browse files
georgehaogeorgehao
committed
address comments
1 parent a58e728 commit 741341f

File tree

4 files changed

+160
-164
lines changed

4 files changed

+160
-164
lines changed

‎crates/node/src/add_ons/rollup.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::{
22
args::ScrollRollupNodeConfig,
3-
pprof::{start_pprof_server, PprofConfig},
3+
pprof::PprofConfig,
44
};
55

66
use reth_chainspec::NamedChain;
@@ -74,7 +74,7 @@ impl RollupManagerAddOn {
7474
let pprof_config = PprofConfig::new(addr)
7575
.with_default_duration(self.config.pprof_args.default_duration);
7676

77-
match start_pprof_server(pprof_config).await {
77+
match pprof_config.launch_server().await {
7878
Ok(handle) => {
7979
info!(target: "rollup_node::pprof", "pprof server started successfully");
8080
// Spawn the pprof server task

‎crates/node/src/args.rs‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -826,12 +826,7 @@ pub struct RollupNodeGasPriceOracleArgs {
826826
#[derive(Debug, Clone, clap::Args)]
827827
pub struct PprofArgs {
828828
/// Enable the pprof HTTP server for performance profiling
829-
#[arg(
830-
id = "pprof.enabled",
831-
long = "pprof.enabled",
832-
default_value_t = false,
833-
help = "Enable the pprof HTTP server"
834-
)]
829+
#[arg(id = "pprof.enabled", long = "pprof.enabled", help = "Enable the pprof HTTP server")]
835830
pub enabled: bool,
836831

837832
/// The address to bind the pprof HTTP server to
@@ -840,7 +835,7 @@ pub struct PprofArgs {
840835
long = "pprof.addr",
841836
value_name = "PPROF_URL",
842837
help = "Address to bind the pprof HTTP server (e.g., 0.0.0.0:6868)",
843-
default_value = "0.0.0.0:6868"
838+
default_value = constants::DEFAULT_PPROF_URL
844839
)]
845840
pub addr: String,
846841

@@ -850,7 +845,7 @@ pub struct PprofArgs {
850845
value_name = "PPROF_DEFAULT_DURATION",
851846
long = "pprof.default-duration",
852847
help = "Default CPU profiling duration in seconds",
853-
default_value_t = 30
848+
default_value_t = constants::DEFAULT_PPROF_DEFAULT_DURATION
854849
)]
855850
pub default_duration: u64,
856851
}

‎crates/node/src/constants.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,9 @@ pub(crate) const SCROLL_MAINNET_SIGNER: Address =
6464
/// The authorized signer address for Scroll sepolia.
6565
pub(crate) const SCROLL_SEPOLIA_SIGNER: Address =
6666
address!("0x687E0E85AD67ff71aC134CF61b65905b58Ab43b2");
67+
68+
/// The url for pprof
69+
pub(crate) const DEFAULT_PPROF_URL: &str = "0.0.0.0:6868";
70+
71+
/// The default duration for pprof
72+
pub(crate) const DEFAULT_PPROF_DEFAULT_DURATION: u64 = 30;

‎crates/node/src/pprof.rs‎

Lines changed: 149 additions & 154 deletions
Original file line numberDiff line numberDiff line change
@@ -61,182 +61,177 @@ impl PprofConfig {
6161
self.default_duration = seconds;
6262
self
6363
}
64-
}
6564

66-
/// Start the pprof HTTP server
67-
///
68-
/// This function spawns a background task that runs an HTTP server for pprof endpoints.
69-
/// The server will run until the returned handle is dropped or the task is cancelled.
70-
///
71-
/// # Arguments
72-
/// * `config` - Configuration for the pprof server
73-
///
74-
/// # Returns
75-
/// A `JoinHandle` that can be used to manage the server task
76-
///
77-
/// # Example
78-
/// ```no_run
79-
/// use rollup_node::pprof::{start_pprof_server, PprofConfig};
80-
/// use std::net::SocketAddr;
81-
///
82-
/// #[tokio::main]
83-
/// async fn main() -> eyre::Result<()> {
84-
/// let config = PprofConfig::new("127.0.0.1:6868".parse()?);
85-
/// let handle = start_pprof_server(config).await?;
86-
///
87-
/// // Server runs in background
88-
/// // ...
89-
///
90-
/// // Wait for server to complete (or cancel it)
91-
/// handle.await??;
92-
/// Ok(())
93-
/// }
94-
/// ```
95-
pub async fn start_pprof_server(
96-
config: PprofConfig,
97-
) -> Result<tokio::task::JoinHandle<Result<()>>> {
98-
let listener = TcpListener::bind(config.addr).await?;
99-
let addr = listener.local_addr()?;
65+
/// Start the pprof HTTP server
66+
///
67+
/// This function spawns a background task that runs an HTTP server for pprof endpoints.
68+
/// The server will run until the returned handle is dropped or the task is cancelled.
69+
///
70+
/// # Returns
71+
/// A `JoinHandle` that can be used to manage the server task
72+
///
73+
/// # Example
74+
/// ```no_run
75+
/// use rollup_node::pprof::PprofConfig;
76+
/// use std::net::SocketAddr;
77+
///
78+
/// #[tokio::main]
79+
/// async fn main() -> eyre::Result<()> {
80+
/// let config = PprofConfig::new("127.0.0.1:6868".parse()?);
81+
/// let handle = config.launch_server().await?;
82+
///
83+
/// // Server runs in background
84+
/// // ...
85+
///
86+
/// // Wait for server to complete (or cancel it)
87+
/// handle.await??;
88+
/// Ok(())
89+
/// }
90+
/// ```
91+
pub async fn launch_server(self) -> Result<tokio::task::JoinHandle<Result<()>>> {
92+
let listener = TcpListener::bind(self.addr).await?;
93+
let addr = listener.local_addr()?;
10094

101-
info!("Starting pprof server on http://{}", addr);
102-
info!("CPU profile endpoint: http://{}/debug/pprof/profile?seconds=30", addr);
95+
info!("Starting pprof server on http://{}", addr);
96+
info!("CPU profile endpoint: http://{}/debug/pprof/profile?seconds=30", addr);
10397

104-
let handle = tokio::spawn(async move {
105-
loop {
106-
let (stream, peer_addr) = match listener.accept().await {
107-
Ok(conn) => conn,
108-
Err(e) => {
109-
error!("Failed to accept connection: {}", e);
110-
continue;
111-
}
112-
};
98+
let default_duration = self.default_duration;
99+
let handle = tokio::spawn(async move {
100+
loop {
101+
let (stream, peer_addr) = match listener.accept().await {
102+
Ok(conn) => conn,
103+
Err(e) => {
104+
error!("Failed to accept connection: {}", e);
105+
continue;
106+
}
107+
};
113108

114-
let io = TokioIo::new(stream);
115-
let default_duration = config.default_duration;
109+
let io = TokioIo::new(stream);
116110

117-
tokio::spawn(async move {
118-
let service = service_fn(move |req| handle_request(req, default_duration));
111+
tokio::spawn(async move {
112+
let service = service_fn(move |req| Self::handle_request(req, default_duration));
119113

120-
if let Err(err) = http1::Builder::new().serve_connection(io, service).await {
121-
error!("Error serving connection from {}: {}", peer_addr, err);
122-
}
123-
});
124-
}
125-
});
114+
if let Err(err) = http1::Builder::new().serve_connection(io, service).await {
115+
error!("Error serving connection from {}: {}", peer_addr, err);
116+
}
117+
});
118+
}
119+
});
126120

127-
Ok(handle)
128-
}
121+
Ok(handle)
122+
}
129123

130-
/// Handle HTTP requests to pprof endpoints
131-
async fn handle_request(
132-
req: Request<Incoming>,
133-
default_duration: u64,
134-
) -> Result<Response<Full<Bytes>>, hyper::Error> {
135-
match (req.method(), req.uri().path()) {
136-
(&Method::GET, "/debug/pprof/profile") => {
137-
// Parse duration from query parameters
138-
let duration = req
139-
.uri()
140-
.query()
141-
.and_then(|q| {
142-
q.split('&')
143-
.find(|pair| pair.starts_with("seconds="))
144-
.and_then(|pair| pair.strip_prefix("seconds="))
145-
.and_then(|s| s.parse::<u64>().ok())
146-
})
147-
.unwrap_or(default_duration);
124+
/// Handle HTTP requests to pprof endpoints
125+
async fn handle_request(
126+
req: Request<Incoming>,
127+
default_duration: u64,
128+
) -> Result<Response<Full<Bytes>>, hyper::Error> {
129+
match (req.method(), req.uri().path()) {
130+
(&Method::GET, "/debug/pprof/profile") => {
131+
// Parse duration from query parameters
132+
let duration = req
133+
.uri()
134+
.query()
135+
.and_then(|q| {
136+
q.split('&')
137+
.find(|pair| pair.starts_with("seconds="))
138+
.and_then(|pair| pair.strip_prefix("seconds="))
139+
.and_then(|s| s.parse::<u64>().ok())
140+
})
141+
.unwrap_or(default_duration);
148142

149-
info!("Starting CPU profile for {} seconds", duration);
150-
handle_cpu_profile(duration).await
151-
}
152-
_ => {
153-
warn!("Not found: {} {}", req.method(), req.uri().path());
154-
Ok(Response::builder()
155-
.status(StatusCode::NOT_FOUND)
156-
.body(Full::new(Bytes::from("Not Found")))
157-
.unwrap())
143+
info!("Starting CPU profile for {} seconds", duration);
144+
Self::handle_cpu_profile(duration).await
145+
}
146+
_ => {
147+
warn!("Not found: {} {}", req.method(), req.uri().path());
148+
Ok(Response::builder()
149+
.status(StatusCode::NOT_FOUND)
150+
.body(Full::new(Bytes::from("Not Found")))
151+
.unwrap())
152+
}
158153
}
159154
}
160-
}
161155

162-
/// Handle CPU profiling requests
163-
async fn handle_cpu_profile(duration_secs: u64) -> Result<Response<Full<Bytes>>, hyper::Error> {
164-
// Validate duration
165-
if duration_secs == 0 || duration_secs > 600 {
166-
let error_msg = "Invalid duration: must be between 1 and 600 seconds";
167-
warn!("{}", error_msg);
168-
return Ok(Response::builder()
169-
.status(StatusCode::BAD_REQUEST)
170-
.body(Full::new(Bytes::from(error_msg)))
171-
.unwrap());
172-
}
173-
174-
info!("Collecting CPU profile for {} seconds...", duration_secs);
175-
176-
// Start profiling
177-
let guard = match pprof::ProfilerGuardBuilder::default().build() {
178-
Ok(guard) => guard,
179-
Err(e) => {
180-
error!("Failed to start profiler: {}", e);
181-
let error_msg = format!("Failed to start profiler: {}", e);
156+
/// Handle CPU profiling requests
157+
async fn handle_cpu_profile(duration_secs: u64) -> Result<Response<Full<Bytes>>, hyper::Error> {
158+
// Validate duration
159+
if duration_secs == 0 || duration_secs > 600 {
160+
let error_msg = "Invalid duration: must be between 1 and 600 seconds";
161+
warn!("{}", error_msg);
182162
return Ok(Response::builder()
183-
.status(StatusCode::INTERNAL_SERVER_ERROR)
163+
.status(StatusCode::BAD_REQUEST)
184164
.body(Full::new(Bytes::from(error_msg)))
185165
.unwrap());
186166
}
187-
};
188167

189-
// Profile for the specified duration
190-
tokio::time::sleep(Duration::from_secs(duration_secs)).await;
168+
info!("Collecting CPU profile for {} seconds...", duration_secs);
191169

192-
// Generate report
193-
match guard.report().build() {
194-
Ok(report) => {
195-
// Encode as protobuf
196-
match report.pprof() {
197-
Ok(profile) => {
198-
// The profile object needs to be converted to bytes
199-
let body = match profile.write_to_bytes() {
200-
Ok(bytes) => bytes,
201-
Err(e) => {
202-
error!("Failed to encode profile: {}", e);
203-
let error_msg = format!("Failed to encode profile: {}", e);
204-
return Ok(Response::builder()
205-
.status(StatusCode::INTERNAL_SERVER_ERROR)
206-
.body(Full::new(Bytes::from(error_msg)))
207-
.unwrap());
208-
}
209-
};
170+
// Start profiling
171+
let guard = match pprof::ProfilerGuardBuilder::default().build() {
172+
Ok(guard) => guard,
173+
Err(e) => {
174+
error!("Failed to start profiler: {}", e);
175+
let error_msg = format!("Failed to start profiler: {}", e);
176+
return Ok(Response::builder()
177+
.status(StatusCode::INTERNAL_SERVER_ERROR)
178+
.body(Full::new(Bytes::from(error_msg)))
179+
.unwrap());
180+
}
181+
};
210182

211-
info!("Successfully collected CPU profile ({} bytes)", body.len());
183+
// Profile for the specified duration
184+
tokio::time::sleep(Duration::from_secs(duration_secs)).await;
212185

213-
Ok(Response::builder()
214-
.status(StatusCode::OK)
215-
.header("Content-Type", "application/octet-stream")
216-
.header(
217-
"Content-Disposition",
218-
format!("attachment; filename=\"profile-{}.pb\"", duration_secs),
219-
)
220-
.body(Full::new(Bytes::from(body)))
221-
.unwrap())
222-
}
223-
Err(e) => {
224-
error!("Failed to generate pprof format: {}", e);
225-
let error_msg = format!("Failed to generate pprof format: {}", e);
226-
Ok(Response::builder()
227-
.status(StatusCode::INTERNAL_SERVER_ERROR)
228-
.body(Full::new(Bytes::from(error_msg)))
229-
.unwrap())
186+
// Generate report
187+
match guard.report().build() {
188+
Ok(report) => {
189+
// Encode as protobuf
190+
match report.pprof() {
191+
Ok(profile) => {
192+
// The profile object needs to be converted to bytes
193+
let body = match profile.write_to_bytes() {
194+
Ok(bytes) => bytes,
195+
Err(e) => {
196+
error!("Failed to encode profile: {}", e);
197+
let error_msg = format!("Failed to encode profile: {}", e);
198+
return Ok(Response::builder()
199+
.status(StatusCode::INTERNAL_SERVER_ERROR)
200+
.body(Full::new(Bytes::from(error_msg)))
201+
.unwrap());
202+
}
203+
};
204+
205+
info!("Successfully collected CPU profile ({} bytes)", body.len());
206+
207+
Ok(Response::builder()
208+
.status(StatusCode::OK)
209+
.header("Content-Type", "application/octet-stream")
210+
.header(
211+
"Content-Disposition",
212+
format!("attachment; filename=\"profile-{}.pb\"", duration_secs),
213+
)
214+
.body(Full::new(Bytes::from(body)))
215+
.unwrap())
216+
}
217+
Err(e) => {
218+
error!("Failed to generate pprof format: {}", e);
219+
let error_msg = format!("Failed to generate pprof format: {}", e);
220+
Ok(Response::builder()
221+
.status(StatusCode::INTERNAL_SERVER_ERROR)
222+
.body(Full::new(Bytes::from(error_msg)))
223+
.unwrap())
224+
}
230225
}
231226
}
232-
}
233-
Err(e) => {
234-
error!("Failed to generate report: {}", e);
235-
let error_msg = format!("Failed to generate report: {}", e);
236-
Ok(Response::builder()
237-
.status(StatusCode::INTERNAL_SERVER_ERROR)
238-
.body(Full::new(Bytes::from(error_msg)))
239-
.unwrap())
227+
Err(e) => {
228+
error!("Failed to generate report: {}", e);
229+
let error_msg = format!("Failed to generate report: {}", e);
230+
Ok(Response::builder()
231+
.status(StatusCode::INTERNAL_SERVER_ERROR)
232+
.body(Full::new(Bytes::from(error_msg)))
233+
.unwrap())
234+
}
240235
}
241236
}
242237
}

0 commit comments

Comments
 (0)