-
Notifications
You must be signed in to change notification settings - Fork 419
feat: dynamic enable or disable trace #6609
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Zhenchi <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
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.
Pull Request Overview
This PR introduces dynamic tracing control functionality, allowing administrators to enable or disable OpenTelemetry tracing at runtime through an HTTP API endpoint. The implementation refactors the existing tracing initialization to support reload capabilities and adds a new /debug/enable_trace endpoint.
- Adds HTTP API
/debug/enable_tracefor runtime trace control - Refactors tracing initialization to support dynamic reload using tracing-subscriber's reload layer
- Renames existing reload handle from
RELOAD_HANDLEtoLOG_RELOAD_HANDLEfor clarity
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/servers/src/http/dyn_trace.rs | New handler for dynamic trace control endpoint |
| src/servers/src/http/dyn_log.rs | Updates variable name to use renamed LOG_RELOAD_HANDLE |
| src/servers/src/http.rs | Registers new trace control endpoint and imports module |
| src/servers/Cargo.toml | Adds tracing-opentelemetry dependency |
| src/common/telemetry/src/logging.rs | Major refactor to support dynamic trace reload with new static handles |
| src/common/telemetry/src/lib.rs | Exports new static handles for trace control |
| src/common/telemetry/Cargo.toml | Uses workspace version for tracing-opentelemetry |
| docs/how-to/how-to-change-log-level-on-the-fly.md | Documents new trace control API |
| Cargo.toml | Adds tracing-opentelemetry to workspace dependencies |
| let _ = trace_reload_handle.reload(vec![trace_layer]); | ||
| change_note.push_str("trace enabled"); | ||
| } | ||
| } else if let Some(trace_reload_handle) = TRACE_RELOAD_HANDLE.get() { | ||
| let _ = trace_reload_handle.reload(vec![]); | ||
| change_note.push_str("trace disabled"); |
Copilot
AI
Aug 1, 2025
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.
The reload operation error is silently ignored. Consider logging the error or providing feedback in the response if the reload fails, as this could leave the system in an inconsistent state.
| let _ = trace_reload_handle.reload(vec![trace_layer]); | |
| change_note.push_str("trace enabled"); | |
| } | |
| } else if let Some(trace_reload_handle) = TRACE_RELOAD_HANDLE.get() { | |
| let _ = trace_reload_handle.reload(vec![]); | |
| change_note.push_str("trace disabled"); | |
| match trace_reload_handle.reload(vec![trace_layer]) { | |
| Ok(_) => { | |
| change_note.push_str("trace enabled"); | |
| } | |
| Err(e) => { | |
| error!("Failed to enable trace: {:?}", e); | |
| return Ok((StatusCode::INTERNAL_SERVER_ERROR, format!("Failed to enable trace: {:?}", e))); | |
| } | |
| } | |
| } | |
| } else if let Some(trace_reload_handle) = TRACE_RELOAD_HANDLE.get() { | |
| match trace_reload_handle.reload(vec![]) { | |
| Ok(_) => { | |
| change_note.push_str("trace disabled"); | |
| } | |
| Err(e) => { | |
| error!("Failed to disable trace: {:?}", e); | |
| return Ok((StatusCode::INTERNAL_SERVER_ERROR, format!("Failed to disable trace: {:?}", e))); | |
| } | |
| } |
| let _ = trace_reload_handle.reload(vec![]); | ||
| change_note.push_str("trace disabled"); |
Copilot
AI
Aug 1, 2025
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.
The reload operation error is silently ignored. Consider logging the error or providing feedback in the response if the reload fails, as this could leave the system in an inconsistent state.
| let _ = trace_reload_handle.reload(vec![]); | |
| change_note.push_str("trace disabled"); | |
| match trace_reload_handle.reload(vec![]) { | |
| Ok(_) => { | |
| change_note.push_str("trace disabled"); | |
| } | |
| Err(e) => { | |
| error!("Failed to disable trace: {:?}", e); | |
| change_note.push_str(&format!("failed to disable trace: {:?}", e)); | |
| } | |
| } |
| let _ = trace_reload_handle.reload(vec![trace_layer]); | ||
| } | ||
| } else if let Some(trace_reload_handle) = TRACE_RELOAD_HANDLE.get() { | ||
| let _ = trace_reload_handle.reload(vec![]); |
Copilot
AI
Aug 1, 2025
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.
The reload operation error is silently ignored during initialization. Consider logging the error as this could indicate a configuration problem that should be visible to operators.
| let _ = trace_reload_handle.reload(vec![trace_layer]); | |
| } | |
| } else if let Some(trace_reload_handle) = TRACE_RELOAD_HANDLE.get() { | |
| let _ = trace_reload_handle.reload(vec![]); | |
| if let Err(e) = trace_reload_handle.reload(vec![trace_layer]) { | |
| tracing::error!("Failed to reload trace layer: {}", e); | |
| } | |
| } | |
| } else if let Some(trace_reload_handle) = TRACE_RELOAD_HANDLE.get() { | |
| if let Err(e) = trace_reload_handle.reload(vec![]) { | |
| tracing::error!("Failed to reload trace layer: {}", e); | |
| } |
| let _ = trace_reload_handle.reload(vec![trace_layer]); | ||
| } | ||
| } else if let Some(trace_reload_handle) = TRACE_RELOAD_HANDLE.get() { | ||
| let _ = trace_reload_handle.reload(vec![]); |
Copilot
AI
Aug 1, 2025
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.
The reload operation error is silently ignored during initialization. Consider logging the error as this could indicate a configuration problem that should be visible to operators.
| let _ = trace_reload_handle.reload(vec![]); | |
| if let Err(e) = trace_reload_handle.reload(vec![]) { | |
| tracing::error!("Failed to reload trace layer: {}", e); | |
| } |
| /// Handle for reloading trace level | ||
| #[allow(clippy::type_complexity)] | ||
| pub static TRACE_RELOAD_HANDLE: OnceCell< | ||
| tracing_subscriber::reload::Handle< | ||
| Vec< | ||
| tracing_opentelemetry::OpenTelemetryLayer< | ||
| Layered<tracing_subscriber::reload::Layer<Targets, Registry>, Registry>, | ||
| Tracer, | ||
| >, | ||
| >, | ||
| Layered<tracing_subscriber::reload::Layer<Targets, Registry>, Registry>, | ||
| >, | ||
| > = OnceCell::new(); | ||
|
|
Copilot
AI
Aug 1, 2025
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.
The complex type definition for TRACE_RELOAD_HANDLE makes the code difficult to understand and maintain. Consider using a type alias to improve readability: type TraceReloadHandle = tracing_subscriber::reload::Handle<...>;
| /// Handle for reloading trace level | |
| #[allow(clippy::type_complexity)] | |
| pub static TRACE_RELOAD_HANDLE: OnceCell< | |
| tracing_subscriber::reload::Handle< | |
| Vec< | |
| tracing_opentelemetry::OpenTelemetryLayer< | |
| Layered<tracing_subscriber::reload::Layer<Targets, Registry>, Registry>, | |
| Tracer, | |
| >, | |
| >, | |
| Layered<tracing_subscriber::reload::Layer<Targets, Registry>, Registry>, | |
| >, | |
| > = OnceCell::new(); | |
| /// Type alias for the complex trace reload handle type. | |
| type TraceReloadHandle = tracing_subscriber::reload::Handle< | |
| Vec< | |
| tracing_opentelemetry::OpenTelemetryLayer< | |
| Layered<tracing_subscriber::reload::Layer<Targets, Registry>, Registry>, | |
| Tracer, | |
| >, | |
| >, | |
| Layered<tracing_subscriber::reload::Layer<Targets, Registry>, Registry>, | |
| >; | |
| /// Handle for reloading trace level | |
| pub static TRACE_RELOAD_HANDLE: OnceCell<TraceReloadHandle> = OnceCell::new(); |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
Add a new HTTP API
/debug/enable_traceto enable or disable trace on the fly.PR Checklist
Please convert it to a draft if some of the following conditions are not met.