Commit c50a8d8
OBSL-1025: emit metrics[] in scan-results payload when measurements present (#2774)
* docs(obsl-1013): discovery implementation plan
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(obsl-1013): revise plan — DatasetIdentifier.from_object, per-object DQN via dialect hooks
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(discovery): DatasetIdentifier.from_object builds dialect-correct DQNs
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(discovery): DiscoveryRun discovers, filters, maps to DQNs
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(discovery): DQN-only v4 payload builder + sender
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(discovery): handle_discover_data_source handler
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(discovery): soda data-source discover CLI command
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(discovery): integration snapshot of DQN-only v4 payload
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* style(discovery): apply isort + black (pre-commit)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(discovery): push include/exclude to SQL LIKE filters; drop fnmatch
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(cloud): emit metrics[] in scan-results payload when measurements present (OBSL-1025)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(discovery): open data source connection in discover handler; add handler-level test (OBSL-1013)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(test): scope discover-handler lifecycle test to postgres (OBSL-1013)
The handler discovers with unscoped prefixes, which doesn't surface the
test table on databricks/sparkdf shared metastores. The test verifies
the handler's connection open/close lifecycle, which is
dialect-independent, so one dialect suffices.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* chore: retrigger CI (SonarCloud analysis crash)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs: tighten comments and docs (OBSL-1013)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs: tighten comments and docs (OBSL-1025)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Stamp scan id and honor SODA_SCAN_DEFINITION in discovery (OBSL-1035)
- Add `"scanId": os.environ.get("SODA_SCAN_ID", None)` to build_discovery_payload
- Extract resolve_scan_definition_name helper with precedence: CLI arg > SODA_SCAN_DEFINITION env > default
- Use resolve_scan_definition_name in handle_discover_data_source
- TDD: 5 new unit tests covering scanId env set/unset and all 3 resolution precedence levels
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
* Send required scan timestamps and hasErrors on discovery results (OBSL-1013)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Exclude system schemas from discovery, ported from soda-library (OBSL-1013)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Exit RESULTS_NOT_SENT_TO_CLOUD for pre-send discovery failures (OBSL-1013)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs: forward-facing comments
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs: forward-facing comments
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Report discovery early failures to Soda Cloud via mark_scan_as_failed
Previously the discovery CLI exited RESULTS_NOT_SENT_TO_CLOUD (4) at every
failure stage without sending anything to Soda Cloud, so for managed scans the
launcher's fallback marked the scan failed with a single generic line and the
engine's diagnostics were lost.
Now the handler parses the Soda Cloud config first (without it nothing can
reach Cloud: exit 4, launcher fallback covers visibility) and captures the
run's log records with a Logs collector. For the later stages (data source
YAML unparseable, connection failure, discovery query failure) it calls the
new report_scan_execution_failure helper: with SODA_SCAN_ID set it marks the
pre-created Cloud scan FAILED with the full log records and exits LOG_ERRORS
(3, failure already visible on Cloud); if that call is rejected or raises it
exits 4 so the launcher fallback still fires. Ad-hoc runs (no scan id) exit
LOG_ERRORS without sending, matching contract verification. A rejected
results upload keeps exiting 4, unchanged.
The helper lives in soda_core.cli.handlers.failure_reporting as a stable
import point: the soda-profiling and soda-metric-monitoring flows in
soda-extensions will reuse it. mark_scan_as_failed now returns a success bool
so callers can fall back to exit-code-based visibility; existing callers
ignore the return value.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Harden discovery catch-all and surface rejected mark_scan_as_failed calls
Follow-up to the discovery failure-reporting flow:
- handle_discover_data_source: an unexpected exception after the query phase
(payload build, connection close, an exception escaping the results send)
previously propagated to the CLI top-level handler and exited LOG_ERRORS
with nothing sent to Soda Cloud — a stuck managed scan. A catch-all now
routes it through report_scan_execution_failure. The Logs collector is also
constructed before the opening info line so the run's first record reaches
the failure payload.
- Contract flows adopt the mark_scan_as_failed success bool: both the
per-file (CheckCollectionImpl.verify) and session-level combined-upload
mark sites now set sending_results_to_soda_cloud_failed when Cloud rejects
the mark, so the exit code goes > 3 and the launcher fallback marks the
scan failed instead of a silent LOG_ERRORS. The verify_api.py mark site is
left as-is: it sits in an except block that re-raises, the exception maps
to LOG_ERRORS in the CLI handler, and there is no result object to flag —
adopting the bool there would change the API's raise contract.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Inject resolved dependencies into the discovery handler; centralize resolution and failure mapping
The discovery handler now receives a DataSourceImpl and a required SodaCloud
instead of file paths. Parsing/creation moves to reusable utilities in
soda_core.cli.handlers.dependencies (resolve_soda_cloud, resolve_data_source),
and the exit-code/failure-report mapping lives in one wiring-level construct
(run_with_failure_reporting) that owns the Logs collector so resolution
failures are captured too. Handlers signal engine failures by raising
(ScanExecutionFailedException when already logged); a rejected results upload
returns RESULTS_NOT_SENT_TO_CLOUD directly. Behavior contract (stage table)
unchanged.
Handler tests inject fakes directly; resolution utilities and the exit
mapping get their own focused tests (incl. the cloud-first reorder proof and
the raising parse shapes).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Polish dependency resolution: note log-level asymmetry, annotate response
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Add local (no Soda Cloud) flow for soda data-source discover
Without -sc, discovery now runs locally: the discovered DQNs are printed
to the console (one per line, plus a summary count) and nothing is
registered in Soda Cloud. The Cloud flow with -sc is unchanged.
Local failures follow ad-hoc semantics — logged and mapped to
LOG_ERRORS; there is no scan lifecycle and no failure reporting. One
guard in the wiring, before choosing the flow: SODA_SCAN_ID set without
-sc is a launcher misconfiguration, not a local run — it exits
RESULTS_NOT_SENT_TO_CLOUD so the launcher's fallback marks the scan
failed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Discover views, warn on ignored local flag, make scan definition name mandatory
Three follow-ups to the discover command:
- DiscoveryRun now passes object_types explicitly (TABLE, VIEW,
MATERIALIZED_VIEW): the metadata query's None default is tables-only
for backward compatibility, which silently dropped views — v3
discovery had no type filter. Unknown types still coerce to TABLE and
are included, as before.
- The local flow warns when --scan-definition-name is provided without
-sc: it names the Soda Cloud scan definition and has nothing to apply
to in a local run.
- The Cloud flow's scan definition name is mandatory — CLI arg >
SODA_SCAN_DEFINITION env, no more per-data-source default (an implicit
name would silently register a new scan definition on typos or missing
config). The name resolves inside the wrapped command, so a missing
name takes the standard failure mapping: managed scans get marked
failed with the captured logs and exit 3; ad-hoc runs exit 3. The
handler now takes the resolved name as a required parameter. The
launcher always passes --scan-definition-name, so managed runs are
unaffected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Centralize failure logging in run_with_failure_reporting
New ScanExecutionFailedException convention: handlers never log failures
and never catch-log-rethrow — the exception carries a user-facing
message for expected/validation failures, and unexpected exceptions
propagate raw. run_with_failure_reporting is the single logging site,
distinguishing only the log form: SEFE logs the clean message (no
traceback), everything else logs with the traceback. Both arms then
report via report_scan_execution_failure with the captured records
(the wrapper's failure line is logged before the records are captured,
so it lands in the report).
Accordingly: the discovery handler's catch-log-rethrow block is gone
(raw propagation, traceback logged by the wrapper) and
resolve_scan_definition_name no longer pre-logs — its message moved into
the exception. The local flow is untouched: it has no wrapper, so the
local handler keeps its own single catch-log → LOG_ERRORS.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Unify resolvers on the exception flow; purify run_with_failure_reporting
All CLI dependency resolvers now share one contract: expected/unusable
configuration shapes (missing flag, unparseable/invalid config,
validation rejection) raise ScanExecutionFailedException carrying the
user-facing message — nothing logged at the raise site — while genuinely
unexpected failures (e.g. ImportError from a missing plugin) propagate
raw so the caller logs them with the traceback, preserving the
deliberate traceback-for-data-source-creation asymmetry.
run_with_failure_reporting sheds all construction knowledge: it takes
the already-constructed reporting channel (soda_cloud) and a zero-arg
command, and owns only the Logs lifecycle, the two log-form arms (SEFE
clean, everything else with traceback) and failure reporting. The
discover wiring resolves the reporting channel first in a small guard
(unusable → exit 4, nothing can be reported); the data source and the
mandatory scan definition name resolve inside the wrapped command so
their failures take the standard mark-with-logs mapping. The local flow
adopts the same exception contract with ad-hoc semantics (log →
LOG_ERRORS). One nuance: cloud-resolution failure records are no longer
captured by the wrapper's Logs — they were never deliverable anyway.
ScanExecutionFailedException now extends SodaCoreException (audited: the
only 'except SodaCoreException' site wraps contract-verification query
execution, which the discover SEFE path never crosses), and
SodaCoreException's docstring states its real role as the package-wide
base class.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Type the from_object parameters
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Address end-state review: YAML parse errors take the clean path
- Both resolvers add YamlParserException to their expected-shape catch
tuples: a YAML syntax error and a nonexistent file path — the two most
common user mistakes — raise it from the parsing layer and now map to
a clean ScanExecutionFailedException message instead of a raw
traceback. The tuples are symmetric across the two resolvers.
- Root fix in SodaCloud.from_yaml_source: a config missing the top-level
'soda_cloud' key now raises InvalidSodaCloudConfigurationException
(same contract as the sibling api_key checks) instead of logging a
debug line and crashing on a None dereference.
- exit_with_code is annotated NoReturn so bindings after guard except
arms (e.g. soda_cloud in the discover wiring) are provably assigned.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Move the insert-scan-results transport onto SodaCloud behind a typed DTO
send_discovery_results reached into SodaCloud._execute_command from the
discovery module; the transport now lives where it belongs:
- SodaCoreInsertScanResultsDTO (soda_cloud_dto): TypedDict mirroring the
backend SodaCoreInsertScanResultsCommand field set — required keys are
the backend's @NotNull/@notempty fields plus the type discriminator,
everything else optional via a total=False split (PEP 563-safe
__required_keys__). Intended shared shape for all result-inserting
flows; profiling adopts it in soda-extensions.
- SodaCloud.insert_scan_results(payload) -> bool: owns the
_execute_command call, same accepted-bool contract as
mark_scan_as_failed. The contract flow keeps its richer
send_check_collection_results (response post-processing) — deliberate,
converged separately.
- build_discovery_payload returns the DTO; send_discovery_results is
deleted; the discovery handler calls soda_cloud.insert_scan_results.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Address Sonar findings: redundant catches, single-invocation exception tests, wiring complexity
- dependencies.py: drop pydantic ValidationError from the resolver catch
tuples — it subclasses ValueError in pydantic v2, so the tuple entry was
dead; the coverage is now noted in the resolver docstrings.
- test_cli_dependencies.py / test_cli_data_source_handlers.py: hoist path
construction and mock setup out of pytest.raises blocks so only the call
under test can throw; every assertion is kept.
- cli.py: extract _resolve_soda_cloud_for_discovery_or_exit and
_discover_data_source_locally from the discover wiring to bring its
cognitive complexity under the threshold; behavior unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Final polish: Fabric system schemas, single local logging site, co-located resolvers
- FabricSqlDialect.is_system_schema extends (not replaces) the base
information_schema exclusion: discovery now includes views, so Fabric
was the one dialect where INFORMATION_SCHEMA objects would surface as
discoverable datasets.
- The local discovery handler no longer catch-logs: failures propagate
raw and the local wiring — the single logging site — maps them to
LOG_ERRORS, so connection-open failures are no longer mislabeled as
query failures and the wiring's generic arm is reachable again.
- resolve_scan_definition_name moves to handlers/dependencies.py next to
the other resolvers (its tests move to test_cli_dependencies.py).
- discovery_payload reads the scan id through EnvConfigHelper, the
canonical accessor.
- The DTO's type discriminator is Literal["sodaCoreInsertScanResults"].
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Thread the failure-reporting collector into the wrapped command
run_with_failure_reporting owns a Logs() collector and reports its records
on failure, but until now the wrapped command received nothing. A command
that constructs its own inner Logs (e.g. via execute_check_collections with
logs=None building per-impl Logs) displaced the wrapper's collector, so
records emitted downstream never reached the wrapper's failure report.
The command now receives the wrapper's own logs, so a command running a
check-collection session can thread it through (each impl built with
logs=logs) and keep the failure report complete — matching the shared-logs
pattern #2779 relies on. Discovery constructs no inner Logs (DiscoveryRun
emits via soda_logger, which already lands in the active wrapper collector),
so its wiring accepts and ignores the logs argument.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Address review (#2774): serialise metrics explicitly, drop defensive getattr
- payload["metrics"] is attached after the payload's own to_jsonnable wrap, so
Decimal/datetime/timedelta values in measurement dicts previously stayed
JSON-safe only via the debug-log to_jsonnable side effect at the send site.
Run them through to_jsonnable explicitly and pin the contract with a
json.dumps test over Decimal/datetime values.
- measurement_dicts is a declared field on CheckCollectionResult (default
[]), so use direct attribute access like the sibling accumulations rather
than getattr(..., None).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Fix SonarCloud reliability finding: keep payload dict-typed
to_jsonnable returns `object`, so `payload = to_jsonnable({...})` made payload
statically `object` and Sonar flagged the metrics subscript-assign as
"not subscriptable" (reliability C). Build the dict first (typed `dict`) and
normalize it in place — to_jsonnable mutates and returns the same object, so
behavior is identical and the `# type: ignore` is no longer needed.
Also avoid a float-equality assertion smell in the metrics test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>1 parent af0e982 commit c50a8d8
3 files changed
Lines changed: 135 additions & 29 deletions
File tree
- soda-core/src/soda_core
- check_collections
- common
- soda-tests/tests/unit
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
90 | 90 | | |
91 | 91 | | |
92 | 92 | | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
93 | 98 | | |
94 | 99 | | |
95 | 100 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1686 | 1686 | | |
1687 | 1687 | | |
1688 | 1688 | | |
1689 | | - | |
1690 | | - | |
1691 | | - | |
1692 | | - | |
1693 | | - | |
1694 | | - | |
1695 | | - | |
1696 | | - | |
1697 | | - | |
1698 | | - | |
1699 | | - | |
1700 | | - | |
1701 | | - | |
1702 | | - | |
1703 | | - | |
1704 | | - | |
1705 | | - | |
1706 | | - | |
1707 | | - | |
1708 | | - | |
1709 | | - | |
1710 | | - | |
1711 | | - | |
1712 | | - | |
1713 | | - | |
1714 | | - | |
1715 | | - | |
1716 | | - | |
1717 | | - | |
| 1689 | + | |
| 1690 | + | |
| 1691 | + | |
| 1692 | + | |
| 1693 | + | |
| 1694 | + | |
| 1695 | + | |
| 1696 | + | |
| 1697 | + | |
| 1698 | + | |
| 1699 | + | |
| 1700 | + | |
| 1701 | + | |
| 1702 | + | |
| 1703 | + | |
| 1704 | + | |
| 1705 | + | |
| 1706 | + | |
| 1707 | + | |
| 1708 | + | |
| 1709 | + | |
| 1710 | + | |
| 1711 | + | |
| 1712 | + | |
| 1713 | + | |
| 1714 | + | |
| 1715 | + | |
| 1716 | + | |
| 1717 | + | |
| 1718 | + | |
| 1719 | + | |
| 1720 | + | |
| 1721 | + | |
| 1722 | + | |
| 1723 | + | |
| 1724 | + | |
| 1725 | + | |
| 1726 | + | |
| 1727 | + | |
| 1728 | + | |
| 1729 | + | |
| 1730 | + | |
| 1731 | + | |
| 1732 | + | |
| 1733 | + | |
| 1734 | + | |
| 1735 | + | |
| 1736 | + | |
| 1737 | + | |
1718 | 1738 | | |
1719 | 1739 | | |
1720 | 1740 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
0 commit comments