Skip to content

Commit f718243

Browse files
committed
fix: name failed checks in Dagster failures and log them at error level
The failure description leads with the failed checks and assets, failure metadata lists the failed checks and counts the selection, and the selection is logged as a count with the full list at debug level.
1 parent 97dd18a commit f718243

14 files changed

Lines changed: 408 additions & 19 deletions

File tree

‎crates/sqlbuild-core/src/dagster_invocation/constants.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,3 +33,5 @@ pub(crate) const CODE_SPAN_PADDED_EDGES: [char; 2] = [CODE_SPAN_MARK, ' '];
3333
pub const DEFAULT_ASSET_KIND: &str = "asset";
3434
/// The default translator's description name of a node without one.
3535
pub const DEFAULT_ASSET_NAME: &str = "unknown";
36+
/// Failed checks and models a failure description names before "and N more".
37+
pub const MAX_SUMMARIZED_FAILURES: usize = 10;
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
//! The leading summary of a failed invocation: what failed, before how the command exited.
2+
3+
use crate::dagster_invocation::constants::MAX_SUMMARIZED_FAILURES;
4+
use crate::dagster_invocation::models::{AssetFailure, FailedCheck};
5+
6+
fn check_line(check: &FailedCheck) -> String {
7+
let mut line: String = format!("{} {}", check.kind, check.name);
8+
match (&check.asset_name, &check.column_name) {
9+
(Some(asset), Some(column)) => line.push_str(&format!(" on {asset}.{column}")),
10+
(Some(asset), None) => line.push_str(&format!(" on {asset}")),
11+
(None, _) => {}
12+
}
13+
match check.row_count {
14+
Some(1) => line.push_str(": 1 failing row"),
15+
Some(count) => line.push_str(&format!(": {count} failing rows")),
16+
None => line.push_str(&format!(": {}", check.status)),
17+
}
18+
line
19+
}
20+
21+
fn asset_line(failure: &AssetFailure) -> String {
22+
let mut line: String = format!("{} {}: {}", failure.kind, failure.name, failure.status);
23+
if let Some(phase) = &failure.failed_phase {
24+
line.push_str(&format!(" during {phase}"));
25+
}
26+
line
27+
}
28+
29+
/// `Failed: …` followed by one line per failed check, then per failed asset, at most [`MAX_SUMMARIZED_FAILURES`] of them and `and N more`; empty when nothing is known to have failed.
30+
#[must_use]
31+
pub fn failure_summary(checks: &[FailedCheck], assets: &[AssetFailure]) -> String {
32+
let lines: Vec<String> = checks
33+
.iter()
34+
.map(check_line)
35+
.chain(assets.iter().map(asset_line))
36+
.collect();
37+
if lines.is_empty() {
38+
return String::new();
39+
}
40+
let mut counts: Vec<String> = Vec::new();
41+
for (count, singular, plural) in [
42+
(checks.len(), "check", "checks"),
43+
(assets.len(), "asset", "assets"),
44+
] {
45+
match count {
46+
0 => {}
47+
1 => counts.push(format!("1 {singular}")),
48+
_ => counts.push(format!("{count} {plural}")),
49+
}
50+
}
51+
let mut summary: Vec<String> = vec![format!("Failed: {}", counts.join(", "))];
52+
summary.extend(
53+
lines
54+
.iter()
55+
.take(MAX_SUMMARIZED_FAILURES)
56+
.map(|line| format!(" {line}")),
57+
);
58+
if lines.len() > MAX_SUMMARIZED_FAILURES {
59+
summary.push(format!(
60+
" and {} more",
61+
lines.len() - MAX_SUMMARIZED_FAILURES
62+
));
63+
}
64+
summary.join("\n")
65+
}

‎crates/sqlbuild-core/src/dagster_invocation/main/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ pub mod caller_json_output_path;
44
pub mod dagster_name_segment;
55
pub mod dagster_tag_key;
66
pub mod diagnostic_tail;
7+
pub mod failure_summary;
78
pub mod fallback_asset_description;
89
pub mod has_explicit_scenario_selector;
910
pub mod json_output_requested;

‎crates/sqlbuild-core/src/dagster_invocation/models.rs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,14 @@ pub struct AssetFailure {
1313
pub failed_sql: Option<String>,
1414
pub last_recorded_sql: Option<String>,
1515
}
16+
17+
/// The display facts of one failed check (audit, SQL test or Python check) in an execution document.
18+
#[derive(Clone, Debug, Default, PartialEq, Eq)]
19+
pub struct FailedCheck {
20+
pub kind: String,
21+
pub name: String,
22+
pub asset_name: Option<String>,
23+
pub column_name: Option<String>,
24+
pub status: String,
25+
pub row_count: Option<i64>,
26+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,20 @@
11
//! Shared builders for Dagster invocation tests.
22
3+
use crate::dagster_invocation::models::FailedCheck;
4+
35
/// Owned copies of string literals.
46
pub(super) fn owned(values: &[&str]) -> Vec<String> {
57
values.iter().map(|value| (*value).to_owned()).collect()
68
}
9+
10+
/// A failed `not_null` audit on `model.order_id`.
11+
pub(super) fn not_null_audit(model: &str, row_count: Option<i64>) -> FailedCheck {
12+
FailedCheck {
13+
kind: "audit".to_owned(),
14+
name: "not_null".to_owned(),
15+
asset_name: Some(model.to_owned()),
16+
column_name: Some("order_id".to_owned()),
17+
status: "error".to_owned(),
18+
row_count,
19+
}
20+
}

‎crates/sqlbuild-core/src/dagster_invocation/tests/test_texts.rs‎

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
//! Check names, log lines and failure text match the Dagster integration's output.
22
33
use crate::dagster_invocation::main::asset_failure_text::asset_failure_text;
4+
use crate::dagster_invocation::main::failure_summary::failure_summary;
45
use crate::dagster_invocation::main::normalize_check_name::normalize_check_name;
56
use crate::dagster_invocation::main::wrap_selectors::wrap_selectors;
6-
use crate::dagster_invocation::models::AssetFailure;
7+
use crate::dagster_invocation::models::{AssetFailure, FailedCheck};
8+
use crate::dagster_invocation::tests::helpers::not_null_audit;
79
use crate::dagster_invocation::tests::test_types::{
8-
CheckNameTestCase, FailureTextTestCase, WrapTestCase,
10+
CheckNameTestCase, FailureSummaryTestCase, FailureTextTestCase, WrapTestCase,
911
};
1012

1113
#[test]
@@ -111,3 +113,62 @@ fn given_failed_asset_when_formatting_then_lists_error_relation_and_sql() {
111113
);
112114
}
113115
}
116+
117+
#[test]
118+
fn given_failed_checks_and_assets_when_summarizing_then_names_them_first_and_caps_the_list() {
119+
let failed_model = AssetFailure {
120+
kind: "model".to_owned(),
121+
name: "customers".to_owned(),
122+
status: "failed".to_owned(),
123+
failed_phase: Some("execute".to_owned()),
124+
..AssetFailure::default()
125+
};
126+
let sql_test = FailedCheck {
127+
kind: "sql_test".to_owned(),
128+
name: "orders_totals".to_owned(),
129+
asset_name: Some("orders".to_owned()),
130+
column_name: None,
131+
status: "fail".to_owned(),
132+
row_count: None,
133+
};
134+
let many: Vec<FailedCheck> = (0..12)
135+
.map(|index| index + 2)
136+
.map(|index: i64| not_null_audit(&format!("orders_{index:02}"), Some(index)))
137+
.collect();
138+
let many_lines: String = (2..12)
139+
.map(|index| {
140+
format!("\n audit not_null on orders_{index:02}.order_id: {index} failing rows")
141+
})
142+
.collect();
143+
let test_cases = [
144+
FailureSummaryTestCase {
145+
description: "nothing failed",
146+
checks: Vec::new(),
147+
assets: Vec::new(),
148+
expected_summary: String::new(),
149+
},
150+
FailureSummaryTestCase {
151+
description: "checks come before assets, with their failing rows or status",
152+
checks: vec![not_null_audit("orders", Some(1)), sql_test],
153+
assets: vec![failed_model],
154+
expected_summary: "Failed: 2 checks, 1 asset\n audit not_null on orders.order_id: \
155+
1 failing row\n sql_test orders_totals on orders: fail\n \
156+
model customers: failed during execute"
157+
.to_owned(),
158+
},
159+
FailureSummaryTestCase {
160+
description: "a long list ends with how many more failed",
161+
checks: many,
162+
assets: Vec::new(),
163+
expected_summary: format!("Failed: 12 checks{many_lines}\n and 2 more"),
164+
},
165+
];
166+
for test_case in test_cases {
167+
assert_eq!(
168+
failure_summary(&test_case.checks, &test_case.assets),
169+
test_case.expected_summary,
170+
"{}",
171+
test_case.description
172+
);
173+
}
174+
}

‎crates/sqlbuild-core/src/dagster_invocation/tests/test_types.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
//! Cases for Dagster invocation assembly.
22
3-
use crate::dagster_invocation::models::AssetFailure;
3+
use crate::dagster_invocation::models::{AssetFailure, FailedCheck};
44

55
pub(super) struct ArgumentsTestCase {
66
pub description: &'static str,
@@ -59,3 +59,10 @@ pub(super) struct FallbackDescriptionTestCase {
5959
pub materialization: Option<&'static str>,
6060
pub expected_description: &'static str,
6161
}
62+
63+
pub(super) struct FailureSummaryTestCase {
64+
pub description: &'static str,
65+
pub checks: Vec<FailedCheck>,
66+
pub assets: Vec<AssetFailure>,
67+
pub expected_summary: String,
68+
}

‎crates/sqlbuild-python/src/bindings/_helpers/dagster_invocation/failures.rs‎

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,22 @@
22
33
use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyResult, Python};
44
use pyo3::pyfunction;
5-
use pyo3::types::{PyDict, PyDictMethods, PyString};
5+
use pyo3::types::{PyDict, PyDictMethods, PyList, PyListMethods, PyString};
66
use sqlbuild_core::dagster_invocation::main::asset_failure_text::asset_failure_text;
77
use sqlbuild_core::dagster_invocation::main::diagnostic_tail::diagnostic_tail;
8-
use sqlbuild_core::dagster_invocation::models::AssetFailure;
8+
use sqlbuild_core::dagster_invocation::main::failure_summary::failure_summary;
9+
use sqlbuild_core::dagster_invocation::models::{AssetFailure, FailedCheck};
910
use sqlbuild_host::entries::models::ThirdPartyHost;
1011

1112
use crate::bindings::_helpers::boundary::python_calls::KeywordArguments;
1213
use crate::bindings::_helpers::dagster_invocation::access::{
1314
constant_contains, get, items, text_of,
1415
};
16+
use crate::bindings::_helpers::dagster_invocation::results::{SKIPPED_CHECK_STATUS, warn_severity};
1517
use crate::bindings::_helpers::records::fields::field as native_field;
1618

19+
/// Failed checks listed in failure metadata; `failed_check_count` counts them all.
20+
const MAX_FAILED_CHECK_METADATA: usize = 100;
1721
const LIVE_FAILURE_MESSAGE: &str =
1822
"SQLBuild resource attempt failed; see final execution diagnostics";
1923
const UNKNOWN: &str = "unknown";
@@ -46,6 +50,43 @@ fn asset_failure(asset: &Bound<'_, PyAny>) -> PyResult<AssetFailure> {
4650
})
4751
}
4852

53+
/// A check of the execution document that failed with error severity, or `None`.
54+
fn failed_check(check: &Bound<'_, PyAny>) -> PyResult<Option<FailedCheck>> {
55+
if get(check, "passed")?.is_truthy()?
56+
|| get(check, "status")?.eq(SKIPPED_CHECK_STATUS)?
57+
|| warn_severity(check)?
58+
{
59+
return Ok(None);
60+
}
61+
let row_count: Bound<'_, PyAny> = get(check, "row_count")?;
62+
Ok(Some(FailedCheck {
63+
kind: text_of(check, "kind")?,
64+
name: text_of(check, "name")?,
65+
asset_name: optional_text(&get(check, "asset_name")?)?,
66+
column_name: optional_text(&get(check, "attached_column_name")?)?,
67+
status: text_of(check, "status")?,
68+
row_count: row_count.extract()?,
69+
}))
70+
}
71+
72+
fn failed_check_metadata<'py>(
73+
py: Python<'py>,
74+
checks: &[FailedCheck],
75+
) -> PyResult<Bound<'py, PyList>> {
76+
let listed: Bound<'py, PyList> = PyList::empty(py);
77+
for check in checks.iter().take(MAX_FAILED_CHECK_METADATA) {
78+
let item: Bound<'py, PyDict> = PyDict::new(py);
79+
item.set_item("kind", &check.kind)?;
80+
item.set_item("name", &check.name)?;
81+
item.set_item("asset", check.asset_name.as_deref())?;
82+
item.set_item("column", check.column_name.as_deref())?;
83+
item.set_item("status", &check.status)?;
84+
item.set_item("failing_rows", check.row_count)?;
85+
listed.append(item)?;
86+
}
87+
Ok(listed)
88+
}
89+
4990
/// The Dagster failure description and metadata of a failed invocation.
5091
#[pyfunction]
5192
#[pyo3(signature = (**options))]
@@ -85,13 +126,14 @@ pub(crate) fn dagster_failure<'py>(
85126
let metadata: Bound<'py, PyDict> = PyDict::new(py);
86127
metadata.set_item("command", &command_text)?;
87128
metadata.set_item("project_dir", project_dir.str()?)?;
88-
metadata.set_item("selection", selection.join(" "))?;
129+
metadata.set_item("selection_count", selection.len())?;
89130
metadata.set_item("selector_file", selector_file)?;
90131
let mut description: String =
91132
format!("SQLBuild CLI command failed with exit code {returncode}: {command_text}");
92133
match execution_payload {
93134
Some(payload) => {
94135
let mut incomplete: Vec<String> = Vec::new();
136+
let mut failures: Vec<AssetFailure> = Vec::new();
95137
let mut details: Vec<String> = Vec::new();
96138
let limits: Bound<'py, PyDict> = PyDict::new(py);
97139
for asset in items(&payload, "assets")? {
@@ -102,13 +144,24 @@ pub(crate) fn dagster_failure<'py>(
102144
let failure: AssetFailure = asset_failure(&asset)?;
103145
incomplete.push(format!("{}:{} ({status})", failure.kind, failure.name));
104146
details.push(asset_failure_text(&failure));
147+
failures.push(failure);
105148
let microbatch: Bound<'py, PyAny> = get(&asset, "microbatch")?;
106149
if microbatch.is_instance_of::<PyDict>() && !get(&microbatch, "limit")?.is_none() {
107150
limits.set_item(text_of(&asset, "name")?, microbatch)?;
108151
}
109152
}
153+
let mut checks: Vec<FailedCheck> = Vec::new();
154+
for check in items(&payload, "checks")? {
155+
checks.extend(failed_check(&check)?);
156+
}
110157
metadata.set_item("execution_status", text_of(&payload, "status")?)?;
111158
metadata.set_item("incomplete_assets", incomplete.join(", "))?;
159+
metadata.set_item("failed_check_count", checks.len())?;
160+
metadata.set_item("failed_checks", failed_check_metadata(py, &checks)?)?;
161+
let summary: String = failure_summary(&checks, &failures);
162+
if !summary.is_empty() {
163+
description = format!("{summary}\n\n{description}");
164+
}
112165
if !limits.is_empty() {
113166
metadata.set_item("microbatch_limits", limits)?;
114167
}

‎crates/sqlbuild-python/src/bindings/_helpers/dagster_invocation/results.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ const MATERIALIZATION: &str = "materialization";
2929
const CHECK: &str = "check";
3030
const AUDIT_CHECK_KIND: &str = "audit";
3131
/// The status of an audit that did not run; Dagster gets no check result for it.
32-
const SKIPPED_CHECK_STATUS: &str = "skipped";
32+
pub(crate) const SKIPPED_CHECK_STATUS: &str = "skipped";
3333

3434
/// DAG nodes upstream first, or in artifact order when edges form a cycle.
3535
pub(crate) fn sorted_nodes<'py>(dag: &Bound<'py, PyAny>) -> PyResult<Vec<Bound<'py, PyAny>>> {
@@ -223,7 +223,8 @@ struct CheckFilter<'a> {
223223
emitted_asset_paths: Option<&'a HashSet<AssetPath>>,
224224
}
225225

226-
fn warn_severity(check: &Bound<'_, PyAny>) -> PyResult<bool> {
226+
/// Whether a check result is a warning rather than an error.
227+
pub(crate) fn warn_severity(check: &Bound<'_, PyAny>) -> PyResult<bool> {
227228
let py: Python<'_> = check.py();
228229
let warning: String = constant(py, ThirdPartyHost::WarningCheckSeverity)?.extract()?;
229230
let insufficient: String = constant(py, ThirdPartyHost::InsufficientCheckStatus)?.extract()?;

‎src/sqlbuild/integrations/dagster/_helpers/invocation.py‎

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,40 @@ def _log_invocation(*, context: Any, invocation: SqlBuildCliInvocation) -> None:
3636
logger.info(" %s", " ".join(invocation.command))
3737
if invocation.selection:
3838
if invocation.selector_file_path:
39-
logger.info("SQLBuild selector file:")
40-
logger.info(" %s", invocation.selector_file_path)
41-
logger.info("SQLBuild selected assets from Dagster (%s):", len(invocation.selection))
42-
for line in dagster_selector_log_lines(selectors=list(invocation.selection)):
43-
logger.info(" %s", line)
39+
logger.info(
40+
"SQLBuild selected assets from Dagster: %s (selector file: %s)",
41+
len(invocation.selection),
42+
invocation.selector_file_path,
43+
)
44+
else:
45+
logger.info("SQLBuild selected assets from Dagster: %s", len(invocation.selection))
46+
logger.debug(
47+
"SQLBuild selectors from Dagster:\n%s",
48+
"\n".join(
49+
f" {line}"
50+
for line in dagster_selector_log_lines(selectors=list(invocation.selection))
51+
),
52+
)
53+
54+
55+
def _log_failed_check(*, context: Any, dg: Any, result: Any) -> None:
56+
"""Log a check that did not pass, so it shows in the default run log view."""
57+
58+
if result.passed:
59+
return
60+
logger: Any = getattr(context, "log", None) if context is not None else None
61+
if logger is None:
62+
return
63+
asset: str = "/".join(str(part) for part in result.asset_key.path)
64+
row_count: object = (result.metadata or {}).get("row_count")
65+
detail: str = ""
66+
count: object = getattr(row_count, "value", row_count)
67+
if isinstance(count, int) and not isinstance(count, bool):
68+
detail = f": {count} failing row" if count == 1 else f": {count} failing rows"
69+
if result.severity == dg.AssetCheckSeverity.WARN:
70+
logger.warning("SQLBuild check %s on %s warned%s", result.check_name, asset, detail)
71+
return
72+
logger.error("SQLBuild check %s on %s failed%s", result.check_name, asset, detail)
4473

4574

4675
def _log_skipped_checks(*, context: Any, payload: Mapping[str, Any]) -> None:

0 commit comments

Comments
 (0)