Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: enable async stack trace by default #6539

Merged
merged 5 commits into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/compute/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use serde::{Deserialize, Serialize};
#[derive(Debug, Clone, ArgEnum)]
pub enum AsyncStackTraceOption {
Off,
On,
On, // default
Verbose,
}

Expand Down Expand Up @@ -71,7 +71,7 @@ pub struct ComputeNodeOpts {
pub enable_jaeger_tracing: bool,

/// Enable async stack tracing for risectl.
#[clap(long, arg_enum, default_value_t = AsyncStackTraceOption::Off)]
#[clap(long, arg_enum, default_value_t = AsyncStackTraceOption::On)]
pub async_stack_trace: AsyncStackTraceOption,

/// Path to file cache data directory.
Expand Down
14 changes: 5 additions & 9 deletions src/stream/src/task/stream_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -627,10 +627,10 @@ impl LocalStreamManagerCore {
);

let monitor = tokio_metrics::TaskMonitor::new();
let trace_reporter = self
let trace = self
.stack_trace_manager
.as_mut()
.map(|(m, _)| m.register(actor_id));
.map(|(m, c)| (m.register(actor_id), c.clone()));

let handle = {
let context = self.context.clone();
Expand All @@ -642,15 +642,11 @@ impl LocalStreamManagerCore {
}
};
#[auto_enums::auto_enum(Future)]
let traced = match trace_reporter {
Some(trace_reporter) => trace_reporter.trace(
let traced = match trace {
Some((reporter, config)) => reporter.trace(
actor,
format!("Actor {actor_id}: `{}`", mview_definition),
TraceConfig {
report_detached: true,
verbose: true,
interval: Duration::from_secs(1),
},
config,
),
None => actor,
};
Expand Down
15 changes: 3 additions & 12 deletions src/utils/async_stack_trace/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ impl<F: Future, const VERBOSE: bool> Future for StackTraced<F, VERBOSE> {
let this = self.project();

// For assertion.
#[cfg(debug_assertions)]
let old_current = try_with_context(|c| c.current());

let this_node = match this.state {
Expand Down Expand Up @@ -173,7 +174,7 @@ impl<F: Future, const VERBOSE: bool> Future for StackTraced<F, VERBOSE> {
};

// The current node must be the this_node.
assert_eq!(this_node, with_context(|c| c.current()));
debug_assert_eq!(this_node, with_context(|c| c.current()));

let r = match this.inner.poll(cx) {
// The future is ready, clean-up this span by popping from the context.
Expand All @@ -190,6 +191,7 @@ impl<F: Future, const VERBOSE: bool> Future for StackTraced<F, VERBOSE> {
};

// The current node must be the same as we started with.
#[cfg(debug_assertions)]
assert_eq!(old_current.unwrap(), with_context(|c| c.current()));

r
Expand Down Expand Up @@ -235,20 +237,9 @@ pub trait StackTrace: Future + Sized {

/// Similar to [`stack_trace`], but the span is a verbose one, which means it will be traced
/// only if the verbose configuration is enabled.
#[cfg(not(debug_assertions))]
fn verbose_stack_trace(self, span: impl Into<SpanValue>) -> StackTraced<Self, true> {
StackTraced::new(self, span)
}

/// Similar to [`stack_trace`], but the span is a verbose one, which means it will be traced
/// only if the verbose configuration is enabled.
///
/// With `debug_assertions` on, this span will be disabled statically to avoid affecting
/// performance too much. Therefore, `verbose` mode in [`TraceConfig`] is ignored.
Comment on lines -246 to -247
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously I thought the bad performance with verbose span, non-verbose configuration, and debug profile was caused by the extra code of matching the thread-local context. But actually, I just forgot to pass the config to the trace root and always use verbose: true configuration, as above. 馃ぃ

#[cfg(debug_assertions)]
fn verbose_stack_trace(self, _span: impl Into<SpanValue>) -> Self {
self
}
}
impl<F> StackTrace for F where F: Future {}

Expand Down