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.

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

Jump to bottom

fix: add extra detail to summary showing items from remote cache #7697

Closed
wants to merge 2 commits into from
Closed

fix: add extra detail to summary showing items from remote cache #7697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffc2bbf
fix: add extra detail to summary showing items from remote cache
arlyon Mar 11, 2024
d50fb03
update integration tests
arlyon Mar 12, 2024
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
2 changes: 1 addition & 1 deletion crates/turborepo-cache/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ impl From<turborepo_api_client::Error> for CacheError {
}
}

#[derive(Debug, Clone, PartialEq, Copy)]
#[derive(Debug, Clone, PartialEq, Copy, Serialize)]
pub enum CacheSource {
Local,
Remote,
Expand Down
41 changes: 30 additions & 11 deletions crates/turborepo-lib/src/run/summary/execution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use chrono::{DateTime, Local};
use serde::Serialize;
use tokio::sync::mpsc;
use turbopath::{AbsoluteSystemPathBuf, AnchoredSystemPath};
use turborepo_cache::CacheSource;
use turborepo_ui::{color, cprintln, BOLD, BOLD_GREEN, BOLD_RED, MAGENTA, UI, YELLOW};

use super::TurboDuration;
Expand Down Expand Up @@ -36,6 +37,8 @@ pub struct ExecutionSummary<'a> {
failed: usize,
// number of tasks that had a cache hit
cached: usize,
// number of cache hits that came from the remote cache
cached_remote: usize,
// number of tasks that started
attempted: usize,
pub(crate) start_time: i64,
Expand All @@ -59,7 +62,8 @@ impl<'a> ExecutionSummary<'a> {
command,
success: state.success,
failed: state.failed,
cached: state.cached,
cached: state.cached_local + state.cached_remote,
cached_remote: state.cached_remote,
attempted: state.attempted,
// We're either at some path in the repo, or at the root, which is an empty path
repo_path: package_inference_root.unwrap_or_else(|| AnchoredSystemPath::empty()),
Expand All @@ -82,6 +86,12 @@ impl<'a> ExecutionSummary<'a> {
String::new()
};

let cache_line = if self.cached_remote > 0 {
format!("{} (of which {} remote)", self.cached, self.cached_remote)
Copy link
Member

Choose a reason for hiding this comment

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

Don't want to bikeshed this a ton, but I feel like we can simplify this to just 10 (8 remote) or 10 (2 local, 8 remote).

} else {
self.cached.to_string()
};

let mut line_data = vec![
(
"Tasks",
Expand All @@ -95,10 +105,9 @@ impl<'a> ExecutionSummary<'a> {
"Cached",
format!(
"{}, {} total",
color!(ui, BOLD, "{} cached", self.cached),
color!(ui, BOLD, "{} cached", cache_line),
self.attempted
)
.to_string(),
),
),
(
"Time",
Expand Down Expand Up @@ -159,14 +168,23 @@ impl<'a> ExecutionSummary<'a> {
fn successful(&self) -> usize {
self.success + self.cached
}

fn add_cached(&mut self, source: CacheSource) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this used anywhere so I think it can get removed? In general I think we don't want to be changing the counts on the ExecutionSummary and have all modifications happen to SummaryState

self.cached += 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To preserve run summary output we just add a new field for remote rather than splitting into two. Naturally cached - cached_remote = cached_local if we need it one day.

match source {
CacheSource::Remote => self.cached_remote += 1,
CacheSource::Local => {}
}
}
}

/// The final states of all task executions
#[derive(Debug, Default, Clone)]
pub struct SummaryState {
pub attempted: usize,
pub failed: usize,
pub cached: usize,
pub cached_local: usize,
pub cached_remote: usize,
pub success: usize,
pub tasks: Vec<TaskState>,
}
Expand All @@ -182,7 +200,8 @@ impl SummaryState {
match event {
Event::Building => self.attempted += 1,
Event::BuildFailed => self.failed += 1,
Event::Cached => self.cached += 1,
Event::Cached(CacheSource::Local) => self.cached_local += 1,
Event::Cached(CacheSource::Remote) => self.cached_remote += 1,
Event::Built => self.success += 1,
Event::Canceled => (),
}
Expand All @@ -208,7 +227,7 @@ struct TrackerMessage {
enum Event {
Building,
BuildFailed,
Cached,
Cached(CacheSource),
Built,
// Canceled due to external signal or internal failure
Canceled,
Expand Down Expand Up @@ -329,7 +348,7 @@ impl TaskTracker<chrono::DateTime<Local>> {
// internal turbo error
pub fn cancel(self) {}

pub async fn cached(self) -> TaskExecutionSummary {
pub async fn cached(self, source: CacheSource) -> TaskExecutionSummary {
let Self {
sender,
started_at,
Expand All @@ -351,7 +370,7 @@ impl TaskTracker<chrono::DateTime<Local>> {
};
sender
.send(TrackerMessage {
event: Event::Cached,
event: Event::Cached(source),
state: Some(state),
})
.await
Expand Down Expand Up @@ -449,7 +468,7 @@ mod test {
let tracker = summary.task_tracker(bar.clone());
tasks.push(tokio::spawn(async move {
let tracker = tracker.start().await;
tracker.cached().await;
tracker.cached(CacheSource::Local).await;
}));
}
{
Expand All @@ -472,7 +491,7 @@ mod test {

let state = summary.finish().await.unwrap();
assert_eq!(state.attempted, 4);
assert_eq!(state.cached, 1);
assert_eq!(state.cached_local, 1);
assert_eq!(state.failed, 1);
assert_eq!(state.success, 1);
let foo_state = state.tasks.iter().find(|task| task.task_id == foo).unwrap();
Expand Down
8 changes: 5 additions & 3 deletions crates/turborepo-lib/src/task_graph/visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use regex::Regex;
use tokio::sync::{mpsc, oneshot};
use tracing::{debug, error, Instrument, Span};
use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPath};
use turborepo_cache::CacheSource;
use turborepo_ci::{Vendor, VendorBehavior};
use turborepo_env::{EnvironmentVariableMap, ResolvedEnvMode};
use turborepo_repository::{
Expand Down Expand Up @@ -663,7 +664,8 @@ enum ExecOutcome {
}

enum SuccessOutcome {
CacheHit,
/// The task was a cache hit, from the given source
CacheHit(CacheSource),
Run,
}

Expand Down Expand Up @@ -709,7 +711,7 @@ impl ExecContext {
match result {
ExecOutcome::Success(outcome) => {
let task_summary = match outcome {
SuccessOutcome::CacheHit => tracker.cached().await,
SuccessOutcome::CacheHit(source) => tracker.cached(source).await,
SuccessOutcome::Run => tracker.build_succeeded(0).await,
};
callback.send(Ok(())).ok();
Expand Down Expand Up @@ -784,7 +786,7 @@ impl ExecContext {
);
self.hash_tracker
.insert_cache_status(self.task_id.clone(), status);
return ExecOutcome::Success(SuccessOutcome::CacheHit);
return ExecOutcome::Success(SuccessOutcome::CacheHit(status.source));
}
Ok(None) => (),
Err(e) => {
Expand Down
2 changes: 2 additions & 0 deletions turborepo-tests/integration/tests/run-summary/error.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Validate that there was a failed task and exitCode is 1 (which is what we get fr
"success": 0,
"failed": 1,
"cached": 0,
"cachedRemote": 0,
"attempted": 1,
"startTime": [0-9]+, (re)
"endTime": [0-9]+, (re)
Expand Down Expand Up @@ -102,6 +103,7 @@ success should be 1, and attempted should be 2
"success": 1,
"failed": 1,
"cached": 0,
"cachedRemote": 0,
"attempted": 2,
"startTime": [0-9]+, (re)
"endTime": [0-9]+, (re)
Expand Down
2 changes: 2 additions & 0 deletions turborepo-tests/integration/tests/run-summary/single-package.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Check
[
"attempted",
"cached",
"cachedRemote",
"command",
"endTime",
"exitCode",
Expand Down Expand Up @@ -130,6 +131,7 @@ Check Rust implementation
[
"attempted",
"cached",
"cachedRemote",
"command",
"endTime",
"exitCode",
Expand Down
Loading