-
Notifications
You must be signed in to change notification settings - Fork 414
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: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
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_trace
for runtime trace control - Refactors tracing initialization to support dynamic reload using tracing-subscriber's reload layer
- Renames existing reload handle from
RELOAD_HANDLE
toLOG_RELOAD_HANDLE
for 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"); |
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))); | |
} | |
} |
Copilot uses AI. Check for mistakes.
let _ = trace_reload_handle.reload(vec![]); | ||
change_note.push_str("trace disabled"); |
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)); | |
} | |
} |
Copilot uses AI. Check for mistakes.
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![]); |
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); | |
} |
Copilot uses AI. Check for mistakes.
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![]); |
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); | |
} |
Copilot uses AI. Check for mistakes.
/// 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(); | ||
|
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(); |
Copilot uses AI. Check for mistakes.
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_trace
to enable or disable trace on the fly.PR Checklist
Please convert it to a draft if some of the following conditions are not met.