Publish sdiag to perfpipe_fair_sdiag_v2 via graph_api (#141)

[Yash0270]

Summary:

Routes Slurm sdiag scrapes into a Scuba-backed perfpipe category (perfpipe_fair_sdiag_v2, namespace ai_infra, oncall fair_efficiency) via the existing graph_api exporter. Same shape as every other perfpipe_fair_* category (node_data, job_data, sacct_running, sacct, scontrol_data): graph_api writes ODS metrics and scribe_logs; scribe routes per-category to a Scuba dataset of the same name. No new exporter.

Three code changes:

1. Sdiag dataclass + SlurmCliClient.sdiag_structured() extended to capture the ~50 fields exposed by sdiag --all --json on Slurm 23.2+ that were previously dropped: schedule cycle depth/last, backfill cycle/depth/queue-length/table-size, timing metadata (req_time, gettimeofday_latency, job_states_ts, parts_packed), RPC blobs (rpcs_by_message_type_json, rpcs_by_user_json) as flat JSON strings. bf_active is Optional[bool] so the JSON serializer emits real true/false. cluster: Optional[str] = None is also on the base dataclass (populated at the collect-sdiag site, not by the parser); derived_cluster is intentionally omitted because sdiag is a cluster-wide slurmctld stat and tagging it with a partition-split identifier would generate misleading duplicate rows.

2. sdiag_scribe_category kwarg threaded through GraphAPI.__init__ (alongside the existing node_scribe_category, job_scribe_category, statvfs_scribe_category, pure_scribe_category). DataIdentifier.SDIAG is registered in monitoring/sink/protocol.py; CliObject.last_sdiag cache + collect_sdiag generator added in monitoring/cli/slurm_monitor.py. A second tuple in data_collection_tasks schedules sdiag LOG writes on the same 5-min cycle as the existing METRIC tasks, reusing the per-cycle scrape (no extra slurmctld load). last_sdiag is cleared before each scrape so a raised scrape cannot republish stale data; the dual-task is no-op when last_sdiag is None. collect_sdiag documents the cache-ordering prerequisite (must run after collect_slurm in the same cycle); the loop site at monitoring/utils/monitor.py carries a matching comment about shared-cache patterns being sensitive to task ordering.

3. meta_utils/scribe.py::ScribeErrorWithAcks surfaces raw reject codes. Before: "Failed to write N/N messages". After: "Failed to write N/N messages; reject codes: ['INVALID_CATEGORY']". Three lines, no behavior change on success; applies to every graph_api LOG path.

Ride-along (exporters/otel.py): OTLP handler moved from the "gcm" parent logger to a child "_gcm_otel_emit" with propagate=False, so OTLP-emit validation errors no longer mirror into Scuba LOG rows as body="Missing field_name=...". Standalone hygiene fix.

Docs: website/docs/GCM_Monitoring/collectors/slurm_monitor.md documents the dual-publish (METRIC->ODS + LOG->scribe via DataIdentifier.SDIAG), the last_sdiag single-scrape caching pattern, and the ordering prerequisite. exporters/graph_api.md gains a per-DataIdentifier routing table covering all six *_scribe_category kwargs plus the unset-category assertion behavior, and an "Adding a new scribe category (Meta-internal)" section linking the Scribe wiki for DMV/PrivacyLib/ScribeShell unblock steps.

Reviewed By: yonglimeta

Differential Revision: D95971502

[github-actions]

CI Commands

The following CI workflows run automatically on every push and pull request:

Workflow	What it runs
GPU Cluster Monitoring Python CI	lint, tests, typecheck, format, deb build, pyoxidizer builds
Go packages CI	shelper tests, format, lint

---

The following commands can be used by maintainers to trigger additional tests that require access to secrets:

Command	Description	Requires approval?
/metaci tests	Runs Meta internal integration tests (pytest)	Yes — a maintainer must trigger the command and approve the deployment request
/metaci integration tests	Same as above (alias)	Yes

Note: Only repository maintainers (OWNER association) can trigger /metaci commands. After commenting the command, a maintainer must also navigate to the Actions tab and approve the deployment to the graph-api-access environment before the jobs will run. See the approval guidelines for what to approve or reject.

[meta-codesync]

@Yash0270 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D95971502.

[luccabb]

add some docs: https://facebookresearch.github.io/gcm/docs/GCM_Monitoring/collectors/

if you could add e2e tests too, mainly to verify that the final scuba table is reasonable

[luccabb]

Suggested change

	result = Sdiag(**data)
	if reset:
	self._reset_sdiag_counters()
	return Sdiag(**data)
	return result
	if reset:
	self._reset_sdiag_counters()
	return Sdiag(**data)

[luccabb]

can we be more specific than Any?

[luccabb]

Suggested change

	# fields so any sdiag stat we have not broken into its own column is
	# still queryable from Scuba via JSON_EXTRACT and so future Slurm
	# additions land in the table without a schema change.
	# fields so any sdiag stat we have not broken into its own column stays
	# queryable downstream via JSON path expressions, and new fields land
	# without requiring a schema change.

[luccabb]

maybe expose both as parameters and let upstream callers decide what/when to reset

[Yash0270]

@luccabb — pushed a structural rewrite (force-pushed after rebasing on main, so the previous inline comment anchors are gone; summary below covers each).

Architecture change. Dropped the standalone gcm sdiag CLI + SdiagPayload dataclass + meta_utils/scuba helper. Instead, the existing slurm_monitor sub-command now uses a new ods_scuba_tee exporter that wraps graph_api (ODS, METRIC) and otel (Scuba fair_sdiag, LOG). Single sdiag --all --json scrape per 5-min cycle, two writes. Avoids the read/reset race that would have existed between two independent collectors hitting the same in-controller sdiag counters, and removes the need for a reset parameter on sdiag_structured().

Comments addressed:

Old comment	Status in this revision
client.py data: dict[str, Any]	Narrowed to dict[str, Optional[int]] (legacy text-parse path only emits int/None).
client.py reset parameter suggestion	Architecture moot — slurm_monitor is the only resetter, no second collector.
sdiag_payload.py docstring wording	Obsolete — SdiagPayload removed; replaced by SdiagLog (no payload-specific docstring).
cli/sdiag.py reset on CLI	Obsolete — sub-command removed.
Top-level: add docs + e2e tests	Added website/docs/GCM_Monitoring/exporters/ods_scuba_tee.md (108 lines) + dual-publish note in slurm_monitor.md. Added test_ods_scuba_tee.py with 10 unit tests (routing, projection, filtering, failure isolation, shutdown, logger-isolation regression). E2E validated by sideloading a standalone PAR into the gcm-slurm-monitor container on a FAIR HPC AWS cluster — confirmed rows land in Scuba fair_sdiag with cluster identity preserved.

Bonus fix while in otel.py: the handler was attached to the "gcm" parent logger, so every gcm.* log record (e.g. dataclass_utils.warning("Missing field_name=...")) propagated up and leaked into whatever Scuba dataset the otel sink was targeting. Moved emit to a dedicated leaf logger _gcm_otel_emit with propagate=False. Net positive for all otel-using sub-commands (scontrol, sacct_publish, etc.) — likely also cleans up similar null-row noise in fair_sacct, fair_scontrol_data.

[Yash0270]

v12: fixed a bug I introduced in the previous force-push.

With --heterogeneous-cluster-v1, slurm_monitor yields one SLURMLog per (cluster, partition), but all sdiag fields are identical across those rows (sdiag is a cluster-wide slurmctld stat, not partition-scoped). The previous version of the tee sink wrote one Scuba row per partition with identical sdiag values — 4× storage and misleading derived_cluster tags suggesting per-partition data.

Fix in ods_scuba_tee.py:

• Dedupe by cluster before projecting to SdiagLog
• Set derived_cluster=cluster (not the partition-suffixed value)
• ODS path unchanged — graph_api still receives all N partition rows for per-partition entity suffixes

Tests added:

• test_write_dedupes_partition_split_to_one_row_per_cluster
• test_write_dedupes_multiple_clusters_independently

E2E re-verified on fair-aws-rc-1: one --once invocation now lands exactly 1 row in fair_sdiag (was 4), cluster=fair-aws-rc-1, derived_cluster=fair-aws-rc-1.

[Yash0270]

v13: fixes OSS CI that v12 broke.

• ufmt: ods_scuba_tee.py was not ufmt-clean (missing blank line after module docstring + import order).
• mypy:
  • client.py was narrowed per your dict[str, Any] comment, but the narrow type confused mypy on the Sdiag(**data) splat (per-key check against Sdiag's mixed int|str|None fields). Kept the narrow type for clarity, added cast(dict[str, Any], data) at the splat site with a comment explaining why.
  • test_slurm.py: json.loads(result.rpcs_by_message_type_json) needed an explicit is not None check (the field is Optional[str]).
  • test_ods_scuba_tee.py: refactored the test fixture so MagicMock instances are exposed via a typed _Fixture dataclass instead of accessed through sink._graph_api (which is typed as the Sink protocol).

All 3 OSS checks pass locally: flake8 ✓ | ufmt ✓ | mypy ✓ — All OSS checks passed!
15 pytest tests pass (was 12; +2 partition-dedup tests, +1 logger-isolation regression).

[luccabb]

polluting the target Scuba table with
# null-data log rows.

the row gets published to scuba but with empty fields? do you have a sample?

[Yash0270]

Here: https://fburl.com/scuba/fair_sacct_running/eqt1fnfy

[Yash0270]

Yes — concrete sample. Two tables affected, the second one is the dramatic case.

fair_sdiag sample row (one per dataclass_utils.warning("Missing field_name=...") per slurm_monitor cycle):

time=2026-05-17 16:14:41 PDT  cluster=null  derived_cluster=null
schedule_cycle_last=null  bf_active=null  ...all 70 cols null...
code.file.path=gcm/monitoring/dataclass_utils.py
code.function.name=instantiate_dataclass

~80 such null rows in last 24h. Scuba: https://fburl.com/scuba/fair_sdiag/oj8u9g16

fair_sacct_running is the dramatic case: 637,929 of 713,988 rows over the last 24h (89.3%) are null-data leakage. Most of those (574,000) attribute to gcm/exporters/otel.py:_write_log itself — the handler was recursing on log records emitted by its own emit path (_write_log logs at gcm.exporters.otel → propagates up to "gcm" parent → re-fires the handler → emits another row → which logs again → snowball). sacct_running runs hourly with large batches so each invocation generates 26-38k junk rows.

Full counts after running this query across all otel-targeted Scuba tables:

Dataset	Total 24h	Null-cluster leaked	%
fair_sacct_running	713,988	637,929	89.3%
fair_sdiag	200	80	40%
fair_sacct	10,360,194	0	0%
fair_scontrol_data	74	0	0%
fair_scontrol_config	68	0	0%
fair_sacctmgr_qos	639	0	0%
fair_sacctmgr_user	517,754	0	0%

Only the first two trigger gcm-level logging during their otel write path; the other four don't, so the bug never fires on them. The fix (logger = "_gcm_otel_emit", propagate=False) removes the entire class of self-amplification.

[luccabb]

Here: https://fburl.com/scuba/fair_sacct_running/eqt1fnfy

that query is Cluster vs cluster naming diff. there's a few nulls on both but with other fields populated, I'm not confident this fixes the root of this issue: https://fburl.com/scuba/fair_sacct_running/28owvuw2

fair_sdiag

there's a few rows here where cluster=null and Body gets populated, I think your change fixes this: https://fburl.com/scuba/fair_sdiag/q9a0ou8f

add some tests:

def test_unrelated_gcm_logger_does_not_emit_to_otel():
    sink = OtelSink(...)
    other = logging.getLogger("gcm.dataclass_utils")
    other.warning("Missing field_name=foo")
    assert sink._scuba_writes == []
    
def test_gcm_logger_emits_to_otel():
    sink = OtelSink(...)
    sink.write(<data> ...)
    assert sink._scuba_writes == [<data> ...]

[luccabb]

can we add DerivedCluster and cluster to the base Sdiag class?

[Yash0270]

Done in v25.

Added cluster: Optional[str] = None to the base Sdiag dataclass (populated at the collect-sdiag site, not by the JSON parser — keeps the parser path untouched).

Dropped derived_cluster and deleted SdiagLog entirely. Rationale: sdiag is a cluster-wide slurmctld stat, so tagging it with derived_cluster would generate misleading duplicate rows for the same scrape (fair-sc, fair-sc.h200, fair-sc.h100 would all carry identical sdiag values). One row per cluster per cycle is the correct shape, even with --heterogeneous-cluster-v1. collect_sdiag now yields Sdiag directly via replace(sdiag, cluster=cluster).

[luccabb]

I think just fold this into collect_slurm -> collect_slurm_and_sdiag. the current state creates an implicit requirement that collect_slurm must always run before collect_sdiag

[luccabb]

i think another way is to write a class that does the querying

class CollectionContext:

    def sdiag(): ...

    def sinfo(): ...

then you init that class inside run_data_collection_loop at every round of collection, call its methods, and cache results.

def callable(collection_context):
    return collection_context.sinfo

...

...
while True:
    collection_context = CollectionContext(...)
    for callable, ...:
        callable(collection_context, ...)

either way, this would require changes on run_data_collection_loop, which has a decent blast radius. Wouldn't be opposed to run_data_collection_loop_v2 that handles this

If we need the data asap, maybe just say explicitly in the docstring that collect_slurm must be called before. Also add a comment here that not respecting task ordering may break collection.

[Yash0270]

Took the short path in v25.

Added an explicit Prerequisite block to the collect_sdiag docstring (must run after collect_slurm in the same cycle; no-ops cleanly if last_sdiag is None), plus a shared-cache ordering comment at the top of the data_collection_tasks loop in monitoring/utils/monitor.py.

The CollectionContext + run_data_collection_loop_v2 refactor is the right long-term shape, but the blast radius (every gcm collector) is wider than this diff. Filing as a follow-up once this lands — rc1 burn-in is queued behind it and the docstring + loop comment is enough to prevent the ordering footgun for now.

[Yash0270]

v25 addresses all three review comments.

Comment on sdiag_log.py:11 (cluster/derived_cluster):

Added cluster: Optional[str] = None to the base Sdiag dataclass (populated at the collect-sdiag site, not by the JSON parser — keeps the parser path untouched).

Dropped derived_cluster and deleted SdiagLog entirely. Rationale: sdiag is a cluster-wide slurmctld stat. Tagging it with derived_cluster would generate misleading duplicate rows for the same scrape (fair-sc, fair-sc.h200, fair-sc.h100 would all carry identical sdiag values). One row per cluster per cycle is the correct shape, even with --heterogeneous-cluster-v1. collect_sdiag now yields Sdiag directly via replace(sdiag, cluster=cluster).

Comments on slurm_monitor.py:356 (ordering coupling between collect_slurm and collect_sdiag):

Took the short path you outlined: explicit docstring on collect_sdiag documenting the prerequisite (must run after collect_slurm in the same cycle; no-ops cleanly if last_sdiag is None), plus a shared-cache ordering comment at the top of the data_collection_tasks loop in monitoring/utils/monitor.py.

The longer-term refactor you sketched (CollectionContext class + run_data_collection_loop_v2) is the right shape but the blast radius (every gcm collector) is wider than this diff. Filing as a follow-up once this lands — rc1 burn-in is queued behind it and the docstring + loop comment is enough to prevent the ordering footgun for now.

[luccabb]

Suggested change

	# Task ordering matters when callables share an instance cache (e.g.
	# `collect_slurm` populates `CliObject.last_sdiag`, consumed by
	# `collect_sdiag`). Reordering tasks in `data_collection_tasks` can
	# break collection for shared-cache consumers.
	# Task ordering matters for some collections. If you wish to update this assumption please create a new `run_data_collection_loop` function

[luccabb]

Suggested change

	# Task B: Sdiag -> LOG -> scribe (perfpipe_fair_sdiag_v2) ->
	# Scuba (perfpipe_fair_sdiag_v2) via graph_api._write_log. Reads cached
	# sdiag from Task A. Mirrors the slurm_job_monitor pattern of
	# per-task DataIdentifier-driven scribe routing.

[luccabb]

Suggested change

	# Task A: SLURMLog -> METRIC -> ODS via graph_api._write_metric.
	# Populates obj.last_sdiag for Task B.

[luccabb]

move these to the top

[luccabb]

Suggested change

	**Data Type**: `DataType.METRIC` (`SLURMLog` → ODS) + `DataType.LOG` with `DataIdentifier.SDIAG` (`Sdiag` → scribe → Scuba `perfpipe_fair_sdiag_v2`).
	**Data Type**: `DataType.METRIC` + `DataType.LOG` with `DataIdentifier.SDIAG`.

[Yash0270]

1 failed test seems to be because of fbcode <--> github cyclic dependency. Fbcode test failure is because of github dependency is failing. Github test failures point to fbcode diff failure.

Force pushing to resolve this. Already tried rebasing multiple times.