feat(observability): align the Tachometer capture with the load window - #359
Merged
Merged
Conversation
nv-yna
requested review from
alec-flowers,
csahithi,
ishandhanani and
nlevin-ui
as code owners
August 27, 2026 23:28
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
=======================================
Coverage ? 75.07%
=======================================
Files ? 109
Lines ? 16022
Branches ? 0
=======================================
Hits ? 12029
Misses ? 3993
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Closed
Start Tachometer after benchmark health/config validation and attempt a graceful stop when the benchmark script returns or raises. Launch the scraper without a shell wrapper and pass POLARS_MAX_THREADS through srun's environment export so the scraper receives termination signals. Allow a configurable 120-second benchmark shutdown grace for compaction. Serve-only, manual and eval-only captures retain their session lifecycle; signal and monitor cleanup retain the existing registry timeout. Preserve current default-on capture, full endpoint coverage, millisecond cadence and power finalization. Omit the obsolete health-check test fix already present in main. Adapted from 3afd832. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
nv-yna
force-pushed
the
yna/tachometer-load-window
branch
from
September 11, 2026 23:51
3afd832 to
2fd702c
Compare
ishandhanani
added a commit
that referenced
this pull request
Sep 13, 2026
Nine conflicting files, resolved by hand: - The process exporter (#413) and the wider node-exporter collector set (#415) arrive as tachometer-stage exporter launches; on this branch the exporters are services. process-exporter is now a service kind (`type: process-exporter`, implied while tachometer runs, `placement.node: all`), host-native from the `configs/process-exporter` binary `make setup` installs (no container, no mounts), skipped with a warning when the binary is missing, container launch when a recipe declares a `container`. The group file is written by the kind's `prepare` hook; `ServiceKind` gains `host_native`, `prepare`, and `skip_reason`. node-exporter's built command carries the stat/vmstat/pressure/meminfo_numa/processes collectors and the widened vmstat field filter. The templates, the group YAML, and the host binary resolver move to `srtctl.services.exporters`; their tests follow. - Load-window tachometer (#359): start/stop inside run_benchmark is kept; `stop_tachometer` now terminates through `ManagedProcess.terminate`, which signals the Slurm step (SIGTERM to the srun client would abort the step and SIGKILL the scraper). The scraper's `terminate_timeout` is the recipe's `shutdown_grace_secs`; the 90 s module constant is gone. - Ingest timestamp fallback (#414) is taken from main wholesale (it also offers `--start-ns`); this branch's own fallback is dropped, its dedup module and docstring edits re-applied. - CPU power telemetry (#410, #422): taken as is (power is frozen); the energy report runs before the S3 upload, which returns the URL only. - Makefile: the `examples`/`golden-check` targets plus the cpu-power targets; the `recipes/`-based runner targets stay deleted. SUMMARY: both new pages, `analyzing.md` stays deleted. submit.py: the direct-host renderer import stays deleted; the arch helpers the cpu-power preflight uses are kept. telemetry.py: IPv6-safe host and the SGLang gateway metrics port together. 2355 tests on Python 3.10 and 3.13, lint, schema docs, 21 examples validated, golden 574 identical / 0 mismatched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
On benchmark runs, start Tachometer after health and runner validation, then attempt a graceful stop when the benchmark script returns or raises. The scraper receives SIGTERM directly through a shell-less srun launch, with
POLARS_MAX_THREADSpassed through--export.observability.tachometer.shutdown_grace_secsdefaults to 120 seconds for benchmark shutdown compaction.Serve-only, manual, and eval-only runs retain session-wide capture. Existing default-on collection, full endpoint coverage, millisecond cadence, and power finalization behavior are preserved. The obsolete health-check test fix is already in main and is omitted from this diff.
Signal handlers and the critical-process monitor retain the registry's existing cleanup timeout; the longer benchmark shutdown grace does not override those paths.
Validation
make check: 1,959 passed, 2 skipped, 6 deselected; source Ruff lint and formatting passed.tydiagnostics match unchanged main; the lifecycle call adds none.git diff --checkpassed. No new SLURM run or end-to-end parquet-compaction verification was performed for this conflict resolution.