Skip to content

Add metrics for recipe metadata in scheduler, UI, and CLI #4399

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.

Sign up for GitHub

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

Merged
Kvadratni merged 1 commit into from
Aug 28, 2025
Merged

Add metrics for recipe metadata in scheduler, UI, and CLI #4399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 18 additions & 1 deletion crates/goose-cli/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -941,8 +941,25 @@ pub async fn cli() -> Result<()> {
.and_then(|name| name.to_str())
.unwrap_or(&recipe_name);

tracing::info!(counter.goose.recipe_runs = 1,
let recipe_version =
crate::recipes::search_recipe::retrieve_recipe_file(&recipe_name)
.ok()
.and_then(|rf| {
goose::recipe::template_recipe::parse_recipe_content(
&rf.content,
rf.parent_dir.to_string_lossy().to_string(),
)
.ok()
.map(|(r, _)| r.version)
})
.unwrap_or_else(|| "unknown".to_string());

tracing::info!(
counter.goose.recipe_runs = 1,
recipe_name = %recipe_display_name,
recipe_version = %recipe_version,
session_type = "recipe",
interface = "cli",
"Recipe execution started"
);

Expand Down
17 changes: 17 additions & 0 deletions crates/goose-server/src/routes/reply.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ struct ChatRequest {
session_id: Option<String>,
session_working_dir: String,
scheduled_job_id: Option<String>,
recipe_name: Option<String>,
recipe_version: Option<String>,
Copy link
Collaborator

@DOsinga DOsinga Aug 28, 2025

Choose a reason for hiding this comment

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

do we need to pass this in? do we not have this information in the session meta data, that seems like a better place. same actually for the schedule_job_id I think.

Copy link
Contributor Author

@Kvadratni Kvadratni Aug 28, 2025

Choose a reason for hiding this comment

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

we do not have it currently no

Copy link
Collaborator

@DOsinga DOsinga Aug 28, 2025

Choose a reason for hiding this comment

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

so we want to move the source of truth for conversations to the server, not add stuff to the reply handler. I also don't think this does record what you want it to record. I think you want to measure how often recipes are used, not how many messages users send to recipes. not that it is doing that, if you resume a conversation, it won't remember it was started by a recipe.

I would suggest to wait with this until the https://github.com/block/goose/pull/4216/files lands and then add the logging to agent/start which should know whether it was done through a recipe (and if not, that would be the right place to add).

}

pub struct SseResponse {
Expand Down Expand Up @@ -183,6 +185,19 @@ async fn reply_handler(
"Session started"
);

if let Some(recipe_name) = &request.recipe_name {
let recipe_version = request.recipe_version.as_deref().unwrap_or("unknown");

tracing::info!(
counter.goose.recipe_runs = 1,
recipe_name = %recipe_name,
recipe_version = %recipe_version,
session_type = "app",
interface = "ui",
"Recipe execution started"
);
}

let (tx, rx) = mpsc::channel(100);
let stream = ReceiverStream::new(rx);
let cancel_token = CancellationToken::new();
Expand Down Expand Up @@ -585,6 +600,8 @@ mod tests {
session_id: Some("test-session".to_string()),
session_working_dir: "test-working-dir".to_string(),
scheduled_job_id: None,
recipe_name: None,
recipe_version: None,
})
.unwrap(),
))
Expand Down
47 changes: 47 additions & 0 deletions crates/goose-server/src/routes/schedule.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,53 @@ async fn run_now_handler(
.await
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;

let recipe_display_name = match scheduler.list_scheduled_jobs().await {
Ok(jobs) => jobs
.into_iter()
.find(|job| job.id == id)
.and_then(|job| {
std::path::Path::new(&job.source)
.file_name()
.and_then(|name| name.to_str())
.map(|s| s.to_string())
})
.unwrap_or_else(|| id.clone()),
Err(_) => id.clone(),
};

let recipe_version = match scheduler.list_scheduled_jobs().await {
Ok(jobs) => jobs
.into_iter()
.find(|job| job.id == id)
.and_then(|job| {
std::fs::read_to_string(&job.source)
.ok()
.and_then(|content| {
goose::recipe::template_recipe::parse_recipe_content(
&content,
std::path::Path::new(&job.source)
.parent()
.unwrap_or_else(|| std::path::Path::new(""))
.to_string_lossy()
.to_string(),
)
.ok()
.map(|(r, _)| r.version)
})
})
.unwrap_or_else(|| "unknown".to_string()),
Err(_) => "unknown".to_string(),
};
Copy link
Collaborator

@DOsinga DOsinga Aug 28, 2025

Choose a reason for hiding this comment

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

you filtering through the list of jobs twice here, once to find the name, once to find the version? make that into one thing

Copy link
Contributor Author

@Kvadratni Kvadratni Aug 28, 2025

Choose a reason for hiding this comment

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

yeah that's on me. let me fix this quick


tracing::info!(
counter.goose.recipe_runs = 1,
recipe_name = %recipe_display_name,
recipe_version = %recipe_version,
session_type = "schedule",
interface = "server",
"Recipe execution started"
);

tracing::info!("Server: Calling scheduler.run_now() for job '{}'", id);

match scheduler.run_now(&id).await {
Expand Down
11 changes: 10 additions & 1 deletion ui/desktop/src/hooks/useChatEngine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,16 @@ export const useChatEngine = ({
api: getApiUrl('/reply'),
id: chat.id,
initialMessages: chat.messages,
body: { session_id: chat.id, session_working_dir: window.appConfig.get('GOOSE_WORKING_DIR') },
body: {
session_id: chat.id,
session_working_dir: window.appConfig.get('GOOSE_WORKING_DIR'),
...(chat.recipeConfig?.title
? {
Copy link
Collaborator

@DOsinga DOsinga Aug 28, 2025

Choose a reason for hiding this comment

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

title is required, so you want to check on recipeConfig here, not title

Copy link
Contributor Author

@Kvadratni Kvadratni Aug 28, 2025

Choose a reason for hiding this comment

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

that's what this is doing
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

Copy link
Collaborator

@DOsinga DOsinga Aug 28, 2025

Choose a reason for hiding this comment

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

It's checking chat.recipeConfig?.title not hat.recipeConfig

recipe_name: chat.recipeConfig.title,
recipe_version: chat.recipeConfig?.version ?? 'unknown',
}
: {}),
},
onFinish: async (_message, _reason) => {
stopPowerSaveBlocker();

Expand Down
Loading