Skip to content

Conversation

waynexia
Copy link
Member

@waynexia waynexia commented Jul 29, 2025

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.

❯ curl --data "true" 0:4000/debug/enable_trace
trace enabled%

❯ curl --data "false" 0:4000/debug/enable_trace 
trace disabled%

PR Checklist

Please convert it to a draft if some of the following conditions are not met.

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.
  • API changes are backward compatible.
  • Schema or data changes are backward compatible.

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@github-actions github-actions bot added size/XS docs-not-required This change does not impact docs. labels Jul 29, 2025
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@github-actions github-actions bot added size/S and removed size/XS labels Aug 1, 2025
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@waynexia waynexia marked this pull request as ready for review August 1, 2025 22:11
@waynexia waynexia requested a review from a team as a code owner August 1, 2025 22:11
@github-actions github-actions bot added docs-required This change requires docs update. and removed docs-not-required This change does not impact docs. labels Aug 1, 2025
@waynexia waynexia changed the title [WIP] feat: dynamic reload trace layer feat: dynamic enable or disable trace Aug 1, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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 to LOG_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

Comment on lines +36 to +41
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");
Copy link
Preview

Copilot AI Aug 1, 2025

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.

Suggested change
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.

Comment on lines +40 to +41
let _ = trace_reload_handle.reload(vec![]);
change_note.push_str("trace disabled");
Copy link
Preview

Copilot AI Aug 1, 2025

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.

Suggested change
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.

Comment on lines +454 to +457
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![]);
Copy link
Preview

Copilot AI Aug 1, 2025

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.

Suggested change
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![]);
Copy link
Preview

Copilot AI Aug 1, 2025

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.

Suggested change
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.

Comment on lines +52 to +65
/// 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();

Copy link
Preview

Copilot AI Aug 1, 2025

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<...>;

Suggested change
/// 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.

@waynexia waynexia marked this pull request as draft August 12, 2025 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required This change requires docs update. documentation size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants