Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 0 additions & 32 deletions src/srtctl/cli/mixins/benchmark_stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -664,38 +664,6 @@ def _get_aiperf_server_metrics_env(
urls = list(dict.fromkeys(urls)) if logical_workers_only else sorted(set(urls))
return {"AIPERF_SERVER_METRICS_URLS": ",".join(urls)}

def _client_polled_metric_urls(self) -> frozenset[str]:
"""The ``/metrics`` URLs the benchmark client will poll on its own.

Tachometer scrapes the complement of this set (see
``TelemetryStageMixin.start_tachometer``), so it is derived from the
same logic that injects ``AIPERF_SERVER_METRICS_URLS`` — including the
dead-TRT-LLM-worker omission and the explicit recipe override. It is
deliberately NOT a second endpoint list to maintain: when the injected
set changes, the complement moves with it. A serve-only or manual run
has no client, so nothing is polled and Tachometer covers everything.
"""
if bool(getattr(self, "serve_only", False)):
return frozenset()
explicit = self.runtime.environment.get("AIPERF_SERVER_METRICS_URLS")
if explicit is not None:
return frozenset(url for url in explicit.split(",") if url)
from srtctl.benchmarks.base import AIPerfBenchmarkRunner, get_runner

benchmark_type = self.config.benchmark.type
if benchmark_type == "custom":
env = self._get_aiperf_server_metrics_env(logical_workers_only=True)
else:
try:
runner = get_runner(benchmark_type)
except ValueError:
return frozenset()
if not isinstance(runner, AIPerfBenchmarkRunner):
return frozenset()
env = self._get_aiperf_server_metrics_env()
urls = env.get("AIPERF_SERVER_METRICS_URLS", "")
return frozenset(url for url in urls.split(",") if url)

def _get_benchmark_env(self, runner: "BenchmarkRunner") -> dict[str, str]:
"""Get environment variables for the benchmark script."""
from srtctl.benchmarks.base import AIPerfBenchmarkRunner
Expand Down
12 changes: 0 additions & 12 deletions src/srtctl/cli/mixins/telemetry_stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -295,17 +295,6 @@ def start_tachometer(self) -> list[ManagedProcess]:

logger.info("Starting Tachometer")

# Scrape the complement of what the benchmark client already polls;
# the helper lives on BenchmarkStageMixin (same orchestrator object).
client_urls_fn = getattr(self, "_client_polled_metric_urls", None)
exclude_urls = client_urls_fn() if client_urls_fn is not None else frozenset()
if exclude_urls:
logger.info(
"Tachometer excludes %d endpoint(s) the benchmark client polls: %s",
len(exclude_urls),
",".join(sorted(exclude_urls)),
)

power_telemetry = self.config.telemetry
dcgm_exporter = power_telemetry.dcgm_exporter if power_telemetry.enabled else tachometer.dcgm_exporter
topology = self._compute_frontend_topology()
Expand All @@ -318,7 +307,6 @@ def start_tachometer(self) -> list[ManagedProcess]:
tachometer=tachometer,
dcgm_exporter=dcgm_exporter,
frontend_type=self.config.frontend.type,
exclude_urls=exclude_urls,
)
)

Expand Down
15 changes: 4 additions & 11 deletions src/srtctl/core/telemetry.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,13 @@ def generate_tachometer_config(
tachometer: TachometerConfig,
dcgm_exporter: TelemetryExporterConfig | None = None,
frontend_type: str = "dynamo",
exclude_urls: frozenset[str] | set[str] = frozenset(),
) -> str:
"""Generate Tachometer TOML from backend and frontend topology.

``exclude_urls`` is the set of ``/metrics`` URLs the benchmark client
already polls (``AIPERF_SERVER_METRICS_URLS``). Tachometer scrapes the
complement so a worker endpoint is never double-polled — the extra scrape
load has previously made a submission irreproducible. Frontend, DCGM and
node-exporter endpoints are never excluded: the frontend scrape is cheap
and Tachometer is the only whole-window, per-replica capture of it.
Every endpoint is scraped even when the benchmark client polls the same
URL (``AIPERF_SERVER_METRICS_URLS``): double-polling has been validated
as harmless, and unconditional coverage keeps Tachometer the one
whole-window, per-replica capture regardless of what the client does.
"""
dcgm_exporter = dcgm_exporter or tachometer.dcgm_exporter
node_exporter = tachometer.node_exporter
Expand Down Expand Up @@ -117,10 +114,6 @@ def generate_tachometer_config(
else:
port = process.sys_port
url = f"http://{node_ip}:{port}/metrics"
if url in exclude_urls:
# The benchmark client already polls this endpoint on its own
# cadence; scrape the complement instead of double-polling.
continue
node_metadata = {
"hostname": process.node,
"worker_index": str(process.endpoint_index),
Expand Down
11 changes: 6 additions & 5 deletions tests/test_telemetry.py
Original file line number Diff line number Diff line change
Expand Up @@ -382,8 +382,9 @@ def test_storage_leaf_is_never_pre_created(self, _mock_get_hostname_ip, tmp_path
assert not local_dir.is_relative_to(storage_path)

@patch("srtctl.core.telemetry.get_hostname_ip", return_value="10.0.0.1")
def test_client_polled_urls_are_excluded_from_backend_targets(self, _mock_get_hostname_ip):
"""Tachometer scrapes the complement of the client's URL list."""
def test_client_polled_urls_are_still_scraped(self, _mock_get_hostname_ip):
"""Tachometer scrapes every endpoint, including URLs the benchmark
client also polls — double-polling has been validated as harmless."""
tachometer = TachometerConfig(enabled=True)
runtime = MagicMock(job_id="12345", run_name="test_12345", network_interface="eth0")
runtime.log_dir = Path("/runs/12345/logs")
Expand Down Expand Up @@ -414,12 +415,12 @@ def test_client_polled_urls_are_excluded_from_backend_targets(self, _mock_get_ho
frontend_topology=topology,
runtime=runtime,
tachometer=tachometer,
exclude_urls={"http://10.0.0.1:8081/metrics"},
)

assert 'url = "http://10.0.0.1:8081/metrics"' not in config_text
# Both worker endpoints appear even though a benchmark client would
# poll the same sys-port URLs via AIPERF_SERVER_METRICS_URLS.
assert 'url = "http://10.0.0.1:8081/metrics"' in config_text
assert 'url = "http://10.0.0.1:8082/metrics"' in config_text
# The frontend endpoint is never excluded (whole-window coverage).
assert 'url = "http://10.0.0.1:8000/metrics"' in config_text

@patch("srtctl.core.telemetry.get_hostname_ip", return_value="10.0.0.1")
Expand Down
Loading