Skip to content

OBSL-1025: emit metrics[] in scan-results payload when measurements present#2774

Merged
mivds merged 52 commits into
mainfrom
obsl-1025-extend-payload-builder-with-metrics-field
Jul 20, 2026
Merged

OBSL-1025: emit metrics[] in scan-results payload when measurements present#2774
mivds merged 52 commits into
mainfrom
obsl-1025-extend-payload-builder-with-metrics-field

Conversation

@mivds

@mivds mivds commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR chain (soda-core)

  1. OBSL-1013: ship table discovery in soda-core (soda data-source discover) #2773 — ship table/column discovery
  2. OBSL-1025: emit metrics[] in scan-results payload when measurements present #2774 — emit metrics[] in scan-results payload ← this PR
  3. OBSL-1027: SQL AST window functions + UNION-in-CTE #2776 — SQL AST window functions + UNION-in-CTE
  4. OBSL-1019/1007 (soda-core part): raw feature-config passthroughs on the dataset-config DTO + MM/dialect seams #2777 — raw feature-config passthrough (config DTO)
  5. All-datasource dialect groundwork: time/interval, percentile, typed timestamp literal, NaN filter #2787 — all-datasource dialect groundwork

Summary

Extends v4's _build_check_collection_results_json_dict to emit a top-level metrics: [...] array — the prerequisite for metric monitoring (OBSL-1008 / OBSL-1024) to send measurements.

  • Adds an optional measurement_dicts: list[dict] field to CheckCollectionResult (defaults empty; distinct from the engine-internal measurements: list[Measurement]). MM callers populate it; contracts leave it empty.
  • The builder collects measurement dicts across results and adds payload["metrics"] only when at least one is present.
  • Byte-identical for contracts: when there are no measurements the "metrics" key is absent entirely (not null, not []), so existing contract payloads are unchanged.

Stacked on #2773 (OBSL-1013). Review/merge that first; this branch targets it.

Test Plan

  • Unit: contracts-only payload omits the metrics key; measurements present → metrics: [...] populated
  • Regression: full unit suite (986) passes; pre-commit (isort/black/autoflake) clean
  • MM tickets (OBSL-1008/1024) will populate measurement_dicts with v3 metric.get_cloud_dict()-shaped dicts

🤖 Generated with Claude Code

mivds and others added 11 commits June 30, 2026 15:44
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ect DQN via dialect hooks

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… DQNs

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nmatch

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… present (OBSL-1025)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mivds and others added 4 commits July 1, 2026 23:30
…handler-level test (OBSL-1013)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…core' into obsl-1025-extend-payload-builder-with-metrics-field
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>
…core' into obsl-1025-extend-payload-builder-with-metrics-field
@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mivds mivds self-assigned this Jul 4, 2026
mivds and others added 11 commits July 5, 2026 01:35
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…core' into obsl-1025-extend-payload-builder-with-metrics-field
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- 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>
…L-1013)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lumn-discovery-in-soda-core' into obsl-1013-discovery-ship-tablecolumn-discovery-in-soda-core

# Conflicts:
#	soda-tests/tests/unit/test_discovery_payload.py
…-1013)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1013)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mivds and others added 13 commits July 13, 2026 13:32
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>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- 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>
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>
…n 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>
…cated 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>
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>
@m1n0

m1n0 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Reviewed as a small incremental change to the shared payload builder. No blocking findings. Verified: no pre-existing metrics key in the builder dict, the dataclass field addition is safe (default after defaults), the byte-identical-for-contracts claim holds, and both branches are pinned by tests. CI green.

One robustness point worth a look before the MM PRs build on this:

  • payload["metrics"] bypasses to_jsonnable (added after the builder's call). Today that's rescued by an accident: _execute_cqrs_request runs to_jsonnable(request_body) for debug logging (soda_cloud.py:1177), which mutates the body in place before requests serializes it with stock json.dumps. In v3 the metrics dicts were only JSON-safe because the whole body went through JsonHelper.to_jsonnable (cloud/soda_cloud.py:184→223) — get_cloud_dict() values can be Decimal/timedelta/datetime. So an MM caller attaching raw v3-shaped dicts works today only via the logging side effect; anyone making that log line lazy breaks sends. Suggest making it explicit — payload["metrics"] = to_jsonnable(all_measurement_dicts) (and consciously pick remove_null_values_in_dicts, since the accidental pass currently strips nulls inside measurement dicts too) — or pin the "pre-serialised" contract with a test that json.dumps(payload) succeeds with realistic (Decimal/datetime) values.

Nit: getattr(r, "measurement_dicts", None) — the sibling accumulations (post_processing_stages, token_usage) use direct attribute access; the field lives on the base dataclass, so r.measurement_dicts works and won't hide typos.

Base automatically changed from obsl-1013-discovery-ship-tablecolumn-discovery-in-soda-core to main July 20, 2026 08:12
…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>
@mivds

mivds commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Both points addressed in 2057a7c:

  • metrics serialisation — made explicit: payload["metrics"] = to_jsonnable(all_measurement_dicts), so it no longer relies on the debug-log to_jsonnable side effect mutating the shared list at the send site. Kept remove_null_values_in_dicts at its default True to match the null-stripping the payload dict already gets, and pinned the pre-serialised contract with a json.dumps(payload) test over Decimal/datetime values.
  • getattr nit — confirmed measurement_dicts is a declared field on CheckCollectionResult (default_factory=list), so switched to direct r.measurement_dicts like the sibling accumulations.

mivds and others added 2 commits July 20, 2026 10:54
…oad-builder-with-metrics-field

# Conflicts:
#	soda-core/src/soda_core/cli/cli.py
#	soda-core/src/soda_core/cli/handlers/data_source.py
#	soda-postgres/src/soda_postgres/common/data_sources/postgres_data_source.py
#	soda-postgres/tests/unit/test_postgres_dialect.py
#	soda-tests/tests/integration/test_discovery.py
#	soda-tests/tests/unit/test_cli_data_source_handlers.py
#	soda-tests/tests/unit/test_cli_discover.py
#	soda-tests/tests/unit/test_discovery_payload.py
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>
@sonarqubecloud

Copy link
Copy Markdown

@mivds
mivds merged commit c50a8d8 into main Jul 20, 2026
22 checks passed
@mivds
mivds deleted the obsl-1025-extend-payload-builder-with-metrics-field branch July 20, 2026 16:03
@mivds
mivds restored the obsl-1025-extend-payload-builder-with-metrics-field branch July 20, 2026 16:04
@mivds
mivds deleted the obsl-1025-extend-payload-builder-with-metrics-field branch July 20, 2026 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants