Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions nexus/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ tokio-postgres = { workspace = true, features = ["with-serde_json-1"] }
tokio-util = { workspace = true, features = ["codec"] }
tough.workspace = true
tufaceous-artifact.workspace = true
usdt.workspace = true
uuid.workspace = true

nexus-auth.workspace = true
Expand Down
24 changes: 23 additions & 1 deletion nexus/src/app/background/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
//! None of this file is specific to Nexus or any of the specific background
//! tasks in Nexus, although the design is pretty bespoke for what Nexus needs.

use crate::app::background::probes;

use super::BackgroundTask;
use super::TaskName;
use assert_matches::assert_matches;
Expand Down Expand Up @@ -129,6 +131,7 @@ impl Driver {
name.clone(),
)]));
let task_exec = TaskExec::new(
&name,
taskdef.period,
taskdef.task_impl,
activator.clone(),
Expand Down Expand Up @@ -189,6 +192,7 @@ impl Driver {
/// If the task is currently running, it will be activated again when it
/// finishes.
pub(super) fn activate(&self, task: &TaskName) {
probes::background__task__activate!(|| task.as_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize the existing terminology is a little confusing here but I wonder if we should call this signal or wakeup or something? It just sounds too much like the probe that would fire when the task is activated. I suggested "signal" because I believe that's the term we use for this elsewhere ("activated by explicit signal").

I'm a little worried this call site won't cover all possible activations. I think consumers can hang onto the activator and activate directly with that?

It might also be nice to have a flag for why this was activated (timeout vs. explicit signal).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realize the existing terminology is a little confusing here but I wonder if we should call this signal or wakeup or something? It just sounds too much like the probe that would fire when the task is activated. I suggested "signal" because I believe that's the term we use for this elsewhere ("activated by explicit signal").

Sounds good, I'll rename it.

I'm a little worried this call site won't cover all possible activations. I think consumers can hang onto the activator and activate directly with that?

I see that now, thanks. I can move this into the internal activation function instead.

It might also be nice to have a flag for why this was activated (timeout vs. explicit signal).

I'm kind of confused now. The background-task-activate-start probe also includes the reason. I was under the impression that the proposed background-task-signal probe was only for explicit signals, but you're proposing including the reason there as well. Could you elaborate? I must have missed the intention somewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right -- in #8424 I only mentioned explicit activation, and you're probably right that that's all we need a probe for right now. (What I was thinking when I wrote the comment above is that it'd be nice to be able to trace other types of activation, including periodic activations and activation by a dependent task. But periodic activation only ever happens from exactly one spot (with only one unique stack) so it's not that interesting. Activation by a dependency could be interesting (e.g., if you wanted to know which dependency did it and what it was doing), but I don't think we could add a useful probe for it without restructuring a lot of code. This happens when the tokio task for this background task notices that one of its dependent watch channels' values has changed. We could add a probe there, but it wouldn't have any useful context. What would be useful would be a probe when that watch channel's value actually got changed (via a send), but that point has no knowledge that it's even related to a background task. So in short, I'd ignore all of this and just do basically what you have here.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a little worried this call site won't cover all possible activations. I think consumers can hang onto the activator and activate directly with that?

I think there's no way around this without bigger changes to the APIs. The Activator is an argument to setting up the background task, in the TaskDefinition type. The task name is not part of the Activator, since that type is entirely generic, and gets wired up in the task setup in Driver::register(). That means it's not possible to emit the probe with the name from inside the Activator type, since that type doesn't store the name.

So if we want to emit this probe when the explicit signal is made from outside the task (as opposed to the one fired when the actual implementation responds to that, which is already covered by the other probes), we either need to include the name in the Activator or remove the name from the probe entirely.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. But without the name and being in the context that actually triggered the activation, I'm not sure how useful the probe is above what we've already got with the activation-start probes. We could just leave this one out for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree it's not useful without the name. The only downside is that we don't see the request for explicit activation, just when the task starts doing its work in response to that. As long as the activating code isn't off the rails, doing something like signaling the task in a loop, that should be fine :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed in c873b46

self.task_required(task).activator.activate();
}

Expand Down Expand Up @@ -234,6 +238,8 @@ pub struct TaskDefinition<'a, N: ToString, D: ToString> {
/// Encapsulates state needed by the background tokio task to manage activation
/// of the background task
struct TaskExec {
/// the name of this background task
name: String,
/// how often the background task should be activated
period: Duration,
/// impl of the background task
Expand All @@ -251,13 +257,23 @@ struct TaskExec {

impl TaskExec {
fn new(
name: impl ToString,
period: Duration,
imp: Box<dyn BackgroundTask>,
activation: Activator,
opctx: OpContext,
status_tx: watch::Sender<TaskStatus>,
) -> TaskExec {
TaskExec { period, imp, activation, opctx, status_tx, iteration: 0 }
let name = name.to_string();
TaskExec {
name,
period,
imp,
activation,
opctx,
status_tx,
iteration: 0,
}
}

/// Body of the tokio task that manages activation of this background task
Expand Down Expand Up @@ -318,10 +334,16 @@ impl TaskExec {
});

// Do it!
probes::background__task__activate__start!(|| {
(&self.name, self.iteration, format!("{reason:?}"))
});
let details = self.imp.activate(&self.opctx).await;
let details_str = serde_json::to_string(&details).unwrap_or_else(|e| {
format!("<<failed to serialize task status: {}>>", e)
});
probes::background__task__activate__done!(|| {
(&self.name, self.iteration, &details_str)
});

let elapsed = start_instant.elapsed();

Expand Down
22 changes: 22 additions & 0 deletions nexus/src/app/background/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,25 @@ impl TaskName {
&self.0
}
}

#[usdt::provider(provider = "nexus")]
mod probes {
/// Fires just before explicitly activating the named background task.
fn background__task__activate(task_name: &str) {}

/// Fires just before running the activate method of the named task.
fn background__task__activate__start(
task_name: &str,
iteration: u64,
reason: &str,
) {
}

/// Fires just after completing the task, with the result as JSON.
fn background__task__activate__done(
task_name: &str,
iteration: u64,
details: &str,
) {
}
}
27 changes: 27 additions & 0 deletions tools/dtrace/nexus/trace-bgtasks.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#!/usr/sbin/dtrace -Zqs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like the other one, some example output would be useful here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in c873b46

nexus*:::background-task-activate-start
{
this->task_name = copyinstr(arg0);
iters[this->task_name] = arg1;
reasons[this->task_name] = copyinstr(arg2);
ts[this->task_name] = timestamp;
}

nexus*:::background-task-activate-done
/iters[this->task_name]/
{
this->task_Name = copyinstr(arg0);
this->duration = timestamp - ts[this->task_name];
printf(
"task_name=\"%s\" iteration=%d reason=%s details=%s duration=%dus\n",
this->task_name,
iters[this->task_name],
reasons[this->task_name],
copyinstr(arg2),
this->duration / 1000
);
iters[this->task_name] = 0;
reasons[this->task_name] = 0;
ts[this->task_name] = 0;
}
Loading