From 044999a4f139679e85a0ae11185ae0f3ec314df8 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 18:39:56 +0000 Subject: [PATCH 01/71] telemetry: add multiprocess metric-state store (mp_store) First component of the native multi-process Prometheus aggregation (prometheus_mp) plugin. mp_store is a per-process, fixed-slot memory-mapped store holding current metric state (cumulative counters and last-operation gauges) indexed by nixl_telemetry_event_type_t, for scrape-time aggregation by a single exporter process. It is a raw mmap of a fixed POD layout (no serialization): writers update slots in place with lock-free __atomic ops; a reader validates the magic/schema/size header and returns a consistent snapshot. Per-process identity (pid + /proc start_time, agent name, hostname, optional dp_rank) lives in the header for liveness and labeling. Built as a small prometheus-cpp-free static library with CTest coverage. Part of NIX-1614. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/meson.build | 1 + .../telemetry/prometheus_mp/meson.build | 35 +++ .../telemetry/prometheus_mp/mp_store.cpp | 258 ++++++++++++++++++ .../telemetry/prometheus_mp/mp_store.h | 146 ++++++++++ test/gtest/meson.build | 3 +- test/gtest/telemetry_mp_store_test.cpp | 166 +++++++++++ 6 files changed, 608 insertions(+), 1 deletion(-) create mode 100644 src/plugins/telemetry/prometheus_mp/meson.build create mode 100644 src/plugins/telemetry/prometheus_mp/mp_store.cpp create mode 100644 src/plugins/telemetry/prometheus_mp/mp_store.h create mode 100644 test/gtest/telemetry_mp_store_test.cpp diff --git a/src/plugins/telemetry/meson.build b/src/plugins/telemetry/meson.build index 4b05ebc04a..d17586a16f 100644 --- a/src/plugins/telemetry/meson.build +++ b/src/plugins/telemetry/meson.build @@ -14,6 +14,7 @@ # limitations under the License. subdir('prometheus') +subdir('prometheus_mp') # DOCA telemetry exporter: an optional DOCA pkg-config dependency, gated exactly # like the GPUNETIO backend (src/plugins/meson.build). Built by default when the diff --git a/src/plugins/telemetry/prometheus_mp/meson.build b/src/plugins/telemetry/prometheus_mp/meson.build new file mode 100644 index 0000000000..c9f8d1bc6d --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/meson.build @@ -0,0 +1,35 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Multiprocess metric-state store: a small, prometheus-cpp-free component (mmap +# per-process store) shared by the prometheus_mp plugin and its unit tests. Built +# unconditionally so the CTest coverage runs even where prometheus-cpp is absent; +# the plugin shared library (which needs prometheus-cpp) is added in a later step. +prometheus_mp_inc_dirs = include_directories('.') + +telemetry_mp_store_lib = static_library( + 'nixl_telemetry_mp_store', + 'mp_store.cpp', + include_directories: [nixl_inc_dirs, utils_inc_dirs, prometheus_mp_inc_dirs], + dependencies: [nixl_common_dep], + install: false, + pic: true, +) + +nixl_telemetry_mp_store_dep = declare_dependency( + link_with: telemetry_mp_store_lib, + include_directories: prometheus_mp_inc_dirs, + dependencies: [nixl_common_dep], +) diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp new file mode 100644 index 0000000000..5f8eef66a6 --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -0,0 +1,258 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "mp_store.h" + +#include "common/nixl_log.h" + +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace nixl::telemetry::mp { + +namespace { + + // "NIXLMPS1" as a little-endian tag; changing the layout must change either this + // or MP_STORE_SCHEMA_VERSION so stale-format files are rejected. + constexpr uint64_t MP_STORE_MAGIC = 0x3153504d4c58494eULL; + + constexpr std::size_t MP_MAX_AGENT_NAME = 256; + constexpr std::size_t MP_MAX_HOSTNAME = 128; + constexpr std::size_t MP_MAX_DP_RANK = 64; + + // Fixed on-disk layout. Plain trivially-copyable POD operated on with __atomic + // builtins (not std::atomic) so it is safe to memset/reinterpret over an mmap'd + // region shared between processes. Field order keeps every uint64 8-byte aligned. + struct mpStoreLayout { + uint64_t magic; + uint32_t schemaVersion; + uint32_t slotCount; + int64_t pid; + uint64_t startTime; + uint64_t lastUpdateNs; + char agentName[MP_MAX_AGENT_NAME]; + char hostname[MP_MAX_HOSTNAME]; + char dpRank[MP_MAX_DP_RANK]; + uint64_t counters[MP_STORE_SLOT_COUNT]; + uint64_t gauges[MP_STORE_SLOT_COUNT]; + }; + + [[nodiscard]] uint64_t + nowNs() noexcept { + return static_cast(std::chrono::duration_cast( + std::chrono::system_clock::now().time_since_epoch()) + .count()); + } + + void + copyField(char *dst, std::size_t cap, const std::string &src, const char *what) { + if (src.size() >= cap) { + NIXL_WARN << "prometheus_mp: " << what << " '" << src << "' exceeds " << (cap - 1) + << " chars; truncating in telemetry store"; + } + const std::size_t n = std::min(src.size(), cap - 1); + std::memcpy(dst, src.data(), n); + dst[n] = '\0'; + } + + [[nodiscard]] std::string + readField(const char *src, std::size_t cap) { + const std::size_t n = ::strnlen(src, cap); + return std::string(src, n); + } + +} // namespace + +uint64_t +readProcessStartTime(int64_t pid) { + std::ifstream stat("/proc/" + std::to_string(pid) + "/stat"); + if (!stat.is_open()) { + return 0; + } + std::string content((std::istreambuf_iterator(stat)), std::istreambuf_iterator()); + + // comm (field 2) is wrapped in parentheses and may itself contain spaces or + // ')', so split on the LAST ')': everything after it starts at field 3. + const auto close = content.rfind(')'); + if (close == std::string::npos) { + return 0; + } + + std::istringstream rest(content.substr(close + 1)); + std::vector tokens{std::istream_iterator(rest), + std::istream_iterator()}; + // starttime is field 22; tokens[0] is field 3, so index 22 - 3 = 19. + constexpr std::size_t kStartTimeIndex = 19; + if (tokens.size() <= kStartTimeIndex) { + return 0; + } + try { + return static_cast(std::stoull(tokens[kStartTimeIndex])); + } + catch (const std::exception &) { + return 0; + } +} + +mpStoreWriter::mpStoreWriter(std::filesystem::path path, + const std::string &agent_name, + const std::string &hostname, + const std::string &dp_rank) + : path_(std::move(path)), + mappingSize_(sizeof(mpStoreLayout)) { + const int fd = ::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC, 0644); + if (fd < 0) { + throw std::runtime_error("prometheus_mp: cannot open telemetry store '" + path_.string() + + "': " + std::strerror(errno)); + } + + if (::ftruncate(fd, static_cast(mappingSize_)) != 0) { + const std::string reason = std::strerror(errno); + ::close(fd); + throw std::runtime_error("prometheus_mp: cannot size telemetry store '" + path_.string() + + "': " + reason); + } + + mapping_ = ::mmap(nullptr, mappingSize_, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + ::close(fd); + if (mapping_ == MAP_FAILED) { + mapping_ = nullptr; + throw std::runtime_error("prometheus_mp: cannot map telemetry store '" + path_.string() + + "': " + std::strerror(errno)); + } + + auto *layout = static_cast(mapping_); + std::memset(layout, 0, mappingSize_); + layout->schemaVersion = MP_STORE_SCHEMA_VERSION; + layout->slotCount = static_cast(MP_STORE_SLOT_COUNT); + layout->pid = static_cast(::getpid()); + layout->startTime = readProcessStartTime(layout->pid); + copyField(layout->agentName, MP_MAX_AGENT_NAME, agent_name, "agent name"); + copyField(layout->hostname, MP_MAX_HOSTNAME, hostname, "hostname"); + copyField(layout->dpRank, MP_MAX_DP_RANK, dp_rank, "dp_rank"); + __atomic_store_n(&layout->lastUpdateNs, nowNs(), __ATOMIC_RELEASE); + // Publish the magic last so a concurrent reader never validates a + // half-initialized header. + __atomic_store_n(&layout->magic, MP_STORE_MAGIC, __ATOMIC_RELEASE); +} + +mpStoreWriter::~mpStoreWriter() { + if (mapping_ != nullptr) { + ::munmap(mapping_, mappingSize_); + mapping_ = nullptr; + } +} + +void +mpStoreWriter::touch() noexcept { + auto *layout = static_cast(mapping_); + __atomic_store_n(&layout->lastUpdateNs, nowNs(), __ATOMIC_RELEASE); +} + +void +mpStoreWriter::addCounter(nixl_telemetry_event_type_t type, uint64_t delta) noexcept { + const auto idx = static_cast(type); + if (idx >= MP_STORE_SLOT_COUNT) { + return; + } + auto *layout = static_cast(mapping_); + __atomic_fetch_add(&layout->counters[idx], delta, __ATOMIC_RELAXED); + touch(); +} + +void +mpStoreWriter::setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept { + const auto idx = static_cast(type); + if (idx >= MP_STORE_SLOT_COUNT) { + return; + } + auto *layout = static_cast(mapping_); + __atomic_store_n(&layout->gauges[idx], value, __ATOMIC_RELAXED); + touch(); +} + +void +mpStoreWriter::refreshHeartbeat() noexcept { + touch(); +} + +std::optional +readStoreSnapshot(const std::filesystem::path &path) { + const int fd = ::open(path.c_str(), O_RDONLY | O_CLOEXEC); + if (fd < 0) { + // Missing/unreadable file is not an error here (peer may have exited). + return std::nullopt; + } + + struct stat st{}; + if (::fstat(fd, &st) != 0 || static_cast(st.st_size) < sizeof(mpStoreLayout)) { + // Too small: likely a file mid-creation by a peer. Skip quietly. + ::close(fd); + return std::nullopt; + } + + void *mapping = ::mmap(nullptr, sizeof(mpStoreLayout), PROT_READ, MAP_SHARED, fd, 0); + ::close(fd); + if (mapping == MAP_FAILED) { + NIXL_WARN << "prometheus_mp: cannot map telemetry store '" << path.string() + << "': " << std::strerror(errno); + return std::nullopt; + } + + const auto *layout = static_cast(mapping); + + if (__atomic_load_n(&layout->magic, __ATOMIC_ACQUIRE) != MP_STORE_MAGIC) { + NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() + << "' with bad magic"; + ::munmap(mapping, sizeof(mpStoreLayout)); + return std::nullopt; + } + if (layout->schemaVersion != MP_STORE_SCHEMA_VERSION || + layout->slotCount != MP_STORE_SLOT_COUNT) { + NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() + << "' with incompatible schema (version " << layout->schemaVersion << ", slots " + << layout->slotCount << ")"; + ::munmap(mapping, sizeof(mpStoreLayout)); + return std::nullopt; + } + + mpStoreSnapshot snap; + snap.pid = layout->pid; + snap.startTime = layout->startTime; + snap.lastUpdateNs = __atomic_load_n(&layout->lastUpdateNs, __ATOMIC_ACQUIRE); + snap.agentName = readField(layout->agentName, MP_MAX_AGENT_NAME); + snap.hostname = readField(layout->hostname, MP_MAX_HOSTNAME); + snap.dpRank = readField(layout->dpRank, MP_MAX_DP_RANK); + for (std::size_t i = 0; i < MP_STORE_SLOT_COUNT; ++i) { + snap.counters[i] = __atomic_load_n(&layout->counters[i], __ATOMIC_RELAXED); + snap.gauges[i] = __atomic_load_n(&layout->gauges[i], __ATOMIC_RELAXED); + } + + ::munmap(mapping, sizeof(mpStoreLayout)); + return snap; +} + +} // namespace nixl::telemetry::mp diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h new file mode 100644 index 0000000000..f529b48d67 --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -0,0 +1,146 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_MP_STORE_H +#define NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_MP_STORE_H + +#include "telemetry_event.h" + +#include +#include +#include +#include +#include + +namespace nixl::telemetry::mp { + +// On-disk schema version for the per-process metric-state store. Independent of +// the event-buffer TELEMETRY_VERSION: this is a different file format. Bump on +// any layout change so a reader rejects incompatible files. +inline constexpr uint32_t MP_STORE_SCHEMA_VERSION = 1; + +// Number of value slots in each of the counter and gauge arrays. Indexed +// directly by nixl_telemetry_event_type_t, so every event type has a reserved +// counter slot and a reserved gauge slot (unused ones stay zero). Derived from +// the highest enum value (AGENT_TELEMETRY_EVENTS_DROPPED must stay last); keep in +// sync if the enum is extended. +inline constexpr std::size_t MP_STORE_SLOT_COUNT = + static_cast(nixl_telemetry_event_type_t::AGENT_TELEMETRY_EVENTS_DROPPED) + 1; + +/** + * @brief A point-in-time copy of one process's metric-state store file. + * + * Produced by readStoreSnapshot(). Values are plain (already loaded from the + * shared file); @c counters are cumulative running totals and @c gauges hold the + * last-operation value, both indexed by nixl_telemetry_event_type_t. + */ +struct mpStoreSnapshot { + int64_t pid = 0; + // Process start time in clock ticks (/proc//stat field 22); pairs with + // pid to survive PID reuse when checking liveness. + uint64_t startTime = 0; + // Wall-clock nanoseconds of the last writer update; used for TTL staleness. + uint64_t lastUpdateNs = 0; + std::string agentName; + std::string hostname; + // Optional Dynamo-style rank label; empty when no rank env was set. + std::string dpRank; + std::array counters{}; + std::array gauges{}; +}; + +/** + * @class mpStoreWriter + * @brief Owns one process's metric-state mmap file and updates it in place. + * + * Each NIXL process (writer or exporter mode) owns exactly one store. Updates + * are lock-free atomic operations directly on the mapped file, so the exporter + * process can read peers' files concurrently without coordination. The file has + * a fixed size (fixed slot layout), so it never needs to grow. + */ +class mpStoreWriter { +public: + /** + * @brief Creates (or truncates) and maps the store file at @p path. + * @param path Full path to this process's store file. + * @param agent_name Per-process agent name (unique; drives the series label). + * @param hostname Host name label. + * @param dp_rank Optional rank label; pass empty to omit it. + * @throws std::runtime_error on open/ftruncate/mmap failure. + */ + mpStoreWriter(std::filesystem::path path, + const std::string &agent_name, + const std::string &hostname, + const std::string &dp_rank); + ~mpStoreWriter(); + + mpStoreWriter(const mpStoreWriter &) = delete; + mpStoreWriter & + operator=(const mpStoreWriter &) = delete; + mpStoreWriter(mpStoreWriter &&) = delete; + mpStoreWriter & + operator=(mpStoreWriter &&) = delete; + + // Adds @p delta to the cumulative counter slot for @p type and refreshes the + // heartbeat. Out-of-range types are ignored. + void + addCounter(nixl_telemetry_event_type_t type, uint64_t delta) noexcept; + + // Stores @p value as the last-operation gauge for @p type and refreshes the + // heartbeat. Out-of-range types are ignored. + void + setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept; + + // Refreshes the last-update timestamp without changing any metric (keeps the + // process from looking stale during idle periods). + void + refreshHeartbeat() noexcept; + + [[nodiscard]] const std::filesystem::path & + path() const noexcept { + return path_; + } + +private: + void + touch() noexcept; + + std::filesystem::path path_; + void *mapping_ = nullptr; + std::size_t mappingSize_ = 0; +}; + +/** + * @brief Reads a consistent snapshot of a store file written by another process. + * @param path Path to a peer's store file. + * @return The snapshot, or std::nullopt if the file is missing, too small, or + * has a bad magic / incompatible schema version (a WARN is logged for a + * present-but-invalid file). + */ +[[nodiscard]] std::optional +readStoreSnapshot(const std::filesystem::path &path); + +/** + * @brief Reads a process's start time (/proc//stat field 22, clock ticks). + * @param pid Process id. + * @return The start time, or 0 if it could not be read. + */ +[[nodiscard]] uint64_t +readProcessStartTime(int64_t pid); + +} // namespace nixl::telemetry::mp + +#endif // NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_MP_STORE_H diff --git a/test/gtest/meson.build b/test/gtest/meson.build index b2be5f7bb1..14e3728574 100644 --- a/test/gtest/meson.build +++ b/test/gtest/meson.build @@ -89,6 +89,7 @@ gtest_sources = [ 'query_mem.cpp', 'telemetry_test.cpp', 'telemetry_prometheus_test.cpp', + 'telemetry_mp_store_test.cpp', 'telemetry_benchmark.cpp', 'nixl_duration_test.cpp', 'configuration.cpp' @@ -114,7 +115,7 @@ test_exe = executable('gtest', sources : gtest_sources, include_directories: [nixl_inc_dirs, utils_inc_dirs, device_api_inc, include_directories('../doca-telemetry')], cpp_args : cpp_flags, - dependencies : [nixl_dep, nixl_common_dep, cuda_dependencies, device_api_dep, gtest_dep, gmock_dep, absl_strings_dep, absl_time_dep, ucx_hw_warning_dep, file_utils_interface], + dependencies : [nixl_dep, nixl_common_dep, cuda_dependencies, device_api_dep, gtest_dep, gmock_dep, absl_strings_dep, absl_time_dep, ucx_hw_warning_dep, file_utils_interface, nixl_telemetry_mp_store_dep], link_with: [nixl_build_lib], install : true ) diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp new file mode 100644 index 0000000000..ac3f386e25 --- /dev/null +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -0,0 +1,166 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "mp_store.h" + +#include "common.h" + +#include + +#include + +#include +#include +#include +#include + +namespace { + +using nixl::telemetry::mp::mpStoreWriter; +using nixl::telemetry::mp::readProcessStartTime; +using nixl::telemetry::mp::readStoreSnapshot; + +constexpr auto TX_BYTES = nixl_telemetry_event_type_t::AGENT_TX_BYTES; +constexpr auto RX_BYTES = nixl_telemetry_event_type_t::AGENT_RX_BYTES; +constexpr auto ERR_BACKEND = nixl_telemetry_event_type_t::AGENT_ERR_BACKEND; + +[[nodiscard]] std::size_t +idx(nixl_telemetry_event_type_t t) { + return static_cast(t); +} + +class MpStoreTest : public ::testing::Test { +protected: + void + SetUp() override { + const auto *info = ::testing::UnitTest::GetInstance()->current_test_info(); + dir_ = std::filesystem::path(::testing::TempDir()) / + ("nixl_mp_store_" + std::to_string(::getpid()) + "_" + info->name()); + std::filesystem::create_directories(dir_); + } + + void + TearDown() override { + std::error_code ec; + std::filesystem::remove_all(dir_, ec); + } + + [[nodiscard]] std::filesystem::path + storePath(const std::string &name) const { + return dir_ / name; + } + + std::filesystem::path dir_; +}; + +TEST_F(MpStoreTest, WriteReadRoundTrip) { + const auto path = storePath("agent-a"); + { + mpStoreWriter writer(path, "agent-a", "host-1", "3"); + writer.addCounter(TX_BYTES, 1000); + writer.setGauge(TX_BYTES, 1000); + writer.addCounter(ERR_BACKEND, 1); + } + + const auto snap = readStoreSnapshot(path); + ASSERT_TRUE(snap.has_value()); + EXPECT_EQ(snap->agentName, "agent-a"); + EXPECT_EQ(snap->hostname, "host-1"); + EXPECT_EQ(snap->dpRank, "3"); + EXPECT_EQ(snap->pid, static_cast(::getpid())); + EXPECT_GT(snap->startTime, 0u); + EXPECT_GT(snap->lastUpdateNs, 0u); + EXPECT_EQ(snap->counters[idx(TX_BYTES)], 1000u); + EXPECT_EQ(snap->gauges[idx(TX_BYTES)], 1000u); + EXPECT_EQ(snap->counters[idx(ERR_BACKEND)], 1u); + // Untouched slots stay zero. + EXPECT_EQ(snap->counters[idx(RX_BYTES)], 0u); + EXPECT_EQ(snap->gauges[idx(RX_BYTES)], 0u); +} + +TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { + const auto path = storePath("agent-b"); + { + mpStoreWriter writer(path, "agent-b", "host-1", ""); + writer.addCounter(TX_BYTES, 100); + writer.addCounter(TX_BYTES, 250); + writer.addCounter(TX_BYTES, 650); + writer.setGauge(TX_BYTES, 100); + writer.setGauge(TX_BYTES, 650); // last write wins + } + + const auto snap = readStoreSnapshot(path); + ASSERT_TRUE(snap.has_value()); + EXPECT_EQ(snap->counters[idx(TX_BYTES)], 1000u); + EXPECT_EQ(snap->gauges[idx(TX_BYTES)], 650u); +} + +TEST_F(MpStoreTest, EmptyRankIsEmpty) { + const auto path = storePath("agent-c"); + { mpStoreWriter writer(path, "agent-c", "host-1", ""); } + + const auto snap = readStoreSnapshot(path); + ASSERT_TRUE(snap.has_value()); + EXPECT_TRUE(snap->dpRank.empty()); +} + +TEST_F(MpStoreTest, LongAgentNameTruncated) { + const auto path = storePath("agent-long"); + const std::string long_name(1000, 'x'); + const gtest::LogIgnoreGuard lig("exceeds 255 chars"); + { mpStoreWriter writer(path, long_name, "host-1", ""); } + + const auto snap = readStoreSnapshot(path); + ASSERT_TRUE(snap.has_value()); + EXPECT_EQ(snap->agentName.size(), 255u); + EXPECT_EQ(snap->agentName, long_name.substr(0, 255)); + EXPECT_EQ(lig.getIgnoredCount(), 1); +} + +TEST_F(MpStoreTest, MissingFileReturnsNullopt) { + EXPECT_FALSE(readStoreSnapshot(storePath("does-not-exist")).has_value()); +} + +TEST_F(MpStoreTest, TooSmallFileReturnsNullopt) { + const auto path = storePath("tiny"); + { + std::ofstream f(path, std::ios::binary); + const char junk[16] = {0}; + f.write(junk, sizeof(junk)); + } + EXPECT_FALSE(readStoreSnapshot(path).has_value()); +} + +TEST_F(MpStoreTest, BadMagicReturnsNullopt) { + const auto path = storePath("bad-magic"); + { + // Large enough to pass the size check, but all-zero -> magic mismatch. + std::ofstream f(path, std::ios::binary); + const std::string zeros(64 * 1024, '\0'); + f.write(zeros.data(), static_cast(zeros.size())); + } + const gtest::LogIgnoreGuard lig("bad magic"); + EXPECT_FALSE(readStoreSnapshot(path).has_value()); + EXPECT_EQ(lig.getIgnoredCount(), 1); +} + +TEST_F(MpStoreTest, ProcessStartTimeSelfNonZeroBogusZero) { + EXPECT_GT(readProcessStartTime(::getpid()), 0u); + // A pid that will not exist -> 0. + EXPECT_EQ(readProcessStartTime(0x7fffffff), 0u); +} + +} // namespace From f17b3126abd93ab02b42d08e714c51ee88faeee0 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 18:51:31 +0000 Subject: [PATCH 02/71] telemetry: add multiprocess scrape-time collector (mp_collector) Second component of the prometheus_mp plugin. nixlMultiprocessCollector is a prometheus::Collectable that, on each scrape, globs the shared telemetry directory for peer store files, reads a snapshot of each, drops (and optionally reaps) stale ones, and returns per-process metric families. Series are emitted per (metric, process) with no cross-process aggregation: cumulative counters and last-operation gauges keyed by nixl_telemetry_event_type_t, plus agent_errors_total with a status label, labeled by hostname, agent_name and (when present) dp_rank. Staleness combines pid liveness (kill(pid,0) + /proc start_time to guard PID reuse) with a last-update TTL. The family-building and liveness logic are split into pure functions for unit testing; a shared store file-naming helper is added to mp_store. Collector library and tests are gated on prometheus-cpp. Part of NIX-1614. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/meson.build | 20 ++ .../telemetry/prometheus_mp/mp_collector.cpp | 209 +++++++++++++++ .../telemetry/prometheus_mp/mp_collector.h | 99 +++++++ .../telemetry/prometheus_mp/mp_store.cpp | 6 + .../telemetry/prometheus_mp/mp_store.h | 15 ++ test/gtest/meson.build | 11 +- test/gtest/telemetry_mp_collector_test.cpp | 247 ++++++++++++++++++ 7 files changed, 606 insertions(+), 1 deletion(-) create mode 100644 src/plugins/telemetry/prometheus_mp/mp_collector.cpp create mode 100644 src/plugins/telemetry/prometheus_mp/mp_collector.h create mode 100644 test/gtest/telemetry_mp_collector_test.cpp diff --git a/src/plugins/telemetry/prometheus_mp/meson.build b/src/plugins/telemetry/prometheus_mp/meson.build index c9f8d1bc6d..c8fd2a3025 100644 --- a/src/plugins/telemetry/prometheus_mp/meson.build +++ b/src/plugins/telemetry/prometheus_mp/meson.build @@ -33,3 +33,23 @@ nixl_telemetry_mp_store_dep = declare_dependency( include_directories: prometheus_mp_inc_dirs, dependencies: [nixl_common_dep], ) + +# Scrape-time collector: converts peer store snapshots into prometheus-cpp metric +# families. Needs prometheus-cpp (core), so it is gated on it being available +# (same condition as the prometheus plugin, set in the sibling subdir above). +if prometheus_found + telemetry_mp_collector_lib = static_library( + 'nixl_telemetry_mp_collector', + 'mp_collector.cpp', + include_directories: [nixl_inc_dirs, utils_inc_dirs, prometheus_mp_inc_dirs], + dependencies: [nixl_common_dep, prometheus_core_dep, nixl_telemetry_mp_store_dep], + install: false, + pic: true, + ) + + nixl_telemetry_mp_collector_dep = declare_dependency( + link_with: telemetry_mp_collector_lib, + include_directories: prometheus_mp_inc_dirs, + dependencies: [nixl_common_dep, prometheus_core_dep, nixl_telemetry_mp_store_dep], + ) +endif diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp new file mode 100644 index 0000000000..c7ba703750 --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -0,0 +1,209 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "mp_collector.h" + +#include "common/nixl_log.h" +#include "telemetry_event.h" + +#include +#include + +#include +#include + +#include +#include +#include + +namespace nixl::telemetry::mp { + +namespace { + + using prometheus::ClientMetric; + using prometheus::MetricFamily; + using prometheus::MetricType; + + [[nodiscard]] uint64_t + nowNs() noexcept { + return static_cast(std::chrono::duration_cast( + std::chrono::system_clock::now().time_since_epoch()) + .count()); + } + + [[nodiscard]] std::vector + baseLabels(const mpStoreSnapshot &s) { + std::vector labels; + labels.push_back({"hostname", s.hostname}); + labels.push_back({"agent_name", s.agentName}); + if (!s.dpRank.empty()) { + labels.push_back({"dp_rank", s.dpRank}); + } + return labels; + } + + [[nodiscard]] ClientMetric + counterMetric(std::vector labels, uint64_t value) { + ClientMetric m; + m.label = std::move(labels); + m.counter.value = static_cast(value); + return m; + } + + [[nodiscard]] ClientMetric + gaugeMetric(std::vector labels, uint64_t value) { + ClientMetric m; + m.label = std::move(labels); + m.gauge.value = static_cast(value); + return m; + } + + [[nodiscard]] bool + nameMatchesStore(const std::string &name) { + return name.size() > MP_STORE_FILE_PREFIX.size() + MP_STORE_FILE_SUFFIX.size() && + name.compare(0, MP_STORE_FILE_PREFIX.size(), MP_STORE_FILE_PREFIX) == 0 && + name.compare(name.size() - MP_STORE_FILE_SUFFIX.size(), + MP_STORE_FILE_SUFFIX.size(), + MP_STORE_FILE_SUFFIX) == 0; + } + +} // namespace + +bool +isProcessAlive(int64_t pid, uint64_t start_time) { + if (pid <= 0) { + return false; + } + if (::kill(static_cast(pid), 0) != 0 && errno == ESRCH) { + return false; + } + // Process exists (kill succeeded, or failed with EPERM). Guard against PID + // reuse: if we recorded a start time and can read the current one, they must + // match. + if (start_time != 0) { + const uint64_t current = readProcessStartTime(pid); + if (current != 0 && current != start_time) { + return false; + } + } + return true; +} + +bool +isSnapshotLive(const mpStoreSnapshot &snap, std::chrono::nanoseconds ttl) { + if (isProcessAlive(snap.pid, snap.startTime)) { + return true; + } + const uint64_t now = nowNs(); + const auto ttl_ns = static_cast(ttl.count() < 0 ? 0 : ttl.count()); + return now >= snap.lastUpdateNs && (now - snap.lastUpdateNs) <= ttl_ns; +} + +std::vector +buildMetricFamilies(const std::vector &snapshots) { + std::vector families; + if (snapshots.empty()) { + return families; + } + + for (const auto type : telemetry_metric_event_types) { + const auto descriptor = nixlEnumStrings::telemetryMetricDescriptor(type); + if (descriptor.counterName == nullptr) { + continue; + } + MetricFamily family; + family.name = descriptor.counterName; + family.help = descriptor.counterHelp; + family.type = MetricType::Counter; + const auto slot = static_cast(type); + for (const auto &snap : snapshots) { + family.metric.push_back(counterMetric(baseLabels(snap), snap.counters[slot])); + } + families.push_back(std::move(family)); + } + + for (const auto type : telemetry_metric_event_types) { + const auto descriptor = nixlEnumStrings::telemetryMetricDescriptor(type); + if (descriptor.gaugeName == nullptr) { + continue; + } + MetricFamily family; + family.name = descriptor.gaugeName; + family.help = descriptor.gaugeHelp; + family.type = MetricType::Gauge; + const auto slot = static_cast(type); + for (const auto &snap : snapshots) { + family.metric.push_back(gaugeMetric(baseLabels(snap), snap.gauges[slot])); + } + families.push_back(std::move(family)); + } + + MetricFamily errors; + errors.name = "agent_errors_total"; + errors.help = "Cumulative error count by status"; + errors.type = MetricType::Counter; + for (const auto &snap : snapshots) { + for (const auto type : telemetry_error_event_types) { + auto labels = baseLabels(snap); + labels.push_back({"status", nixlEnumStrings::telemetryErrorStatusLabel(type)}); + errors.metric.push_back( + counterMetric(std::move(labels), snap.counters[static_cast(type)])); + } + } + families.push_back(std::move(errors)); + + return families; +} + +nixlMultiprocessCollector::nixlMultiprocessCollector(std::filesystem::path dir, + std::chrono::nanoseconds stale_ttl, + bool reap_stale) + : dir_(std::move(dir)), + staleTtl_(stale_ttl), + reapStale_(reap_stale) {} + +std::vector +nixlMultiprocessCollector::Collect() const { + std::vector live; + + std::error_code ec; + std::filesystem::directory_iterator it(dir_, ec); + if (ec) { + NIXL_DEBUG << "prometheus_mp: cannot scan telemetry dir '" << dir_.string() + << "': " << ec.message(); + return {}; + } + + for (const auto &entry : it) { + if (!entry.is_regular_file(ec) || !nameMatchesStore(entry.path().filename().string())) { + continue; + } + auto snap = readStoreSnapshot(entry.path()); + if (!snap) { + continue; + } + if (isSnapshotLive(*snap, staleTtl_)) { + live.push_back(std::move(*snap)); + } else if (reapStale_) { + std::error_code rm_ec; + std::filesystem::remove(entry.path(), rm_ec); + } + } + + return buildMetricFamilies(live); +} + +} // namespace nixl::telemetry::mp diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.h b/src/plugins/telemetry/prometheus_mp/mp_collector.h new file mode 100644 index 0000000000..a16245805f --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.h @@ -0,0 +1,99 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_MP_COLLECTOR_H +#define NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_MP_COLLECTOR_H + +#include "mp_store.h" + +#include +#include + +#include +#include +#include + +namespace nixl::telemetry::mp { + +// Default staleness window: a store whose owning process is gone and whose last +// update is older than this is dropped (and reaped) by the collector. +inline constexpr std::chrono::seconds MP_DEFAULT_STALE_TTL{30}; + +/** + * @brief Whether a process is still running, guarding against PID reuse. + * @param pid Process id from a store header. + * @param start_time Process start time from the same header (0 = unknown/skip check). + * @return True if the pid exists and (when known) its start time still matches. + */ +[[nodiscard]] bool +isProcessAlive(int64_t pid, uint64_t start_time); + +/** + * @brief Whether a store snapshot should still be published. + * + * Live if the owning process is alive, or (to smooth over a just-exited process) + * if its last update is within @p ttl. + * @param snap Store snapshot. + * @param ttl Freshness window for a process that is no longer alive. + */ +[[nodiscard]] bool +isSnapshotLive(const mpStoreSnapshot &snap, std::chrono::nanoseconds ttl); + +/** + * @brief Converts per-process snapshots into Prometheus metric families. + * + * Emits one series per (metric, process): cumulative counters and last-operation + * gauges keyed by nixl_telemetry_event_type_t, plus the agent_errors_total family + * with a status label. Series are labeled by hostname, agent_name and (when + * present) dp_rank. No cross-process aggregation -- each process is its own + * series. Returns empty when @p snapshots is empty. + * @param snapshots Live per-process snapshots. + */ +[[nodiscard]] std::vector +buildMetricFamilies(const std::vector &snapshots); + +/** + * @class nixlMultiprocessCollector + * @brief prometheus-cpp Collectable that aggregates all peer store files. + * + * Registered with the exporter process's Exposer. On each scrape it globs the + * shared directory for store files, reads a snapshot of each, drops (and + * optionally reaps) stale ones, and returns per-process metric families. + */ +class nixlMultiprocessCollector final : public prometheus::Collectable { +public: + /** + * @param dir Shared telemetry directory holding peer store files. + * @param stale_ttl Freshness window for a process that has exited. + * @param reap_stale When true, unlink store files whose process is gone and + * whose data is older than @p stale_ttl. + */ + explicit nixlMultiprocessCollector(std::filesystem::path dir, + std::chrono::nanoseconds stale_ttl = MP_DEFAULT_STALE_TTL, + bool reap_stale = true); + + [[nodiscard]] std::vector + Collect() const override; + +private: + std::filesystem::path dir_; + std::chrono::nanoseconds staleTtl_; + bool reapStale_; +}; + +} // namespace nixl::telemetry::mp + +#endif // NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_MP_COLLECTOR_H diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 5f8eef66a6..c9fef4bf00 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -86,6 +86,12 @@ namespace { } // namespace +std::string +makeStoreFileName(int64_t pid, uint64_t start_time) { + return std::string(MP_STORE_FILE_PREFIX) + std::to_string(pid) + "." + + std::to_string(start_time) + std::string(MP_STORE_FILE_SUFFIX); +} + uint64_t readProcessStartTime(int64_t pid) { std::ifstream stat("/proc/" + std::to_string(pid) + "/stat"); diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index f529b48d67..b907867b29 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -24,6 +24,7 @@ #include #include #include +#include namespace nixl::telemetry::mp { @@ -32,6 +33,11 @@ namespace nixl::telemetry::mp { // any layout change so a reader rejects incompatible files. inline constexpr uint32_t MP_STORE_SCHEMA_VERSION = 1; +// Store file naming shared by the writer (exporter) and the collector so the +// collector can discover peer files by globbing "*". +inline constexpr std::string_view MP_STORE_FILE_PREFIX = "nixl."; +inline constexpr std::string_view MP_STORE_FILE_SUFFIX = ".mmap"; + // Number of value slots in each of the counter and gauge arrays. Indexed // directly by nixl_telemetry_event_type_t, so every event type has a reserved // counter slot and a reserved gauge slot (unused ones stay zero). Derived from @@ -141,6 +147,15 @@ readStoreSnapshot(const std::filesystem::path &path); [[nodiscard]] uint64_t readProcessStartTime(int64_t pid); +/** + * @brief Builds this process's store file name (MP_STORE_FILE_PREFIX / suffix). + * @param pid Process id. + * @param start_time Process start time (disambiguates PID reuse across restarts). + * @return File name of the form "nixl...mmap". + */ +[[nodiscard]] std::string +makeStoreFileName(int64_t pid, uint64_t start_time); + } // namespace nixl::telemetry::mp #endif // NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_MP_STORE_H diff --git a/test/gtest/meson.build b/test/gtest/meson.build index 14e3728574..328b52c120 100644 --- a/test/gtest/meson.build +++ b/test/gtest/meson.build @@ -111,11 +111,20 @@ else ucx_hw_warning_dep = [] endif +# prometheus_mp collector unit test rides the main gtest binary, but only when +# prometheus-cpp (and thus the collector library) is available. +if is_variable('nixl_telemetry_mp_collector_dep') + gtest_sources += 'telemetry_mp_collector_test.cpp' + telemetry_mp_collector_dep = [nixl_telemetry_mp_collector_dep] +else + telemetry_mp_collector_dep = [] +endif + test_exe = executable('gtest', sources : gtest_sources, include_directories: [nixl_inc_dirs, utils_inc_dirs, device_api_inc, include_directories('../doca-telemetry')], cpp_args : cpp_flags, - dependencies : [nixl_dep, nixl_common_dep, cuda_dependencies, device_api_dep, gtest_dep, gmock_dep, absl_strings_dep, absl_time_dep, ucx_hw_warning_dep, file_utils_interface, nixl_telemetry_mp_store_dep], + dependencies : [nixl_dep, nixl_common_dep, cuda_dependencies, device_api_dep, gtest_dep, gmock_dep, absl_strings_dep, absl_time_dep, ucx_hw_warning_dep, file_utils_interface, nixl_telemetry_mp_store_dep, telemetry_mp_collector_dep], link_with: [nixl_build_lib], install : true ) diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp new file mode 100644 index 0000000000..921a69a579 --- /dev/null +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -0,0 +1,247 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "mp_collector.h" +#include "mp_store.h" + +#include + +#include +#include +#include + +#include + +#include +#include +#include +#include +#include +#include + +namespace { + +using nixl::telemetry::mp::buildMetricFamilies; +using nixl::telemetry::mp::isProcessAlive; +using nixl::telemetry::mp::isSnapshotLive; +using nixl::telemetry::mp::makeStoreFileName; +using nixl::telemetry::mp::mpStoreSnapshot; +using nixl::telemetry::mp::mpStoreWriter; +using nixl::telemetry::mp::nixlMultiprocessCollector; +using nixl::telemetry::mp::readProcessStartTime; + +constexpr auto TX_BYTES = nixl_telemetry_event_type_t::AGENT_TX_BYTES; +constexpr auto ERR_BACKEND = nixl_telemetry_event_type_t::AGENT_ERR_BACKEND; + +[[nodiscard]] std::size_t +idx(nixl_telemetry_event_type_t t) { + return static_cast(t); +} + +[[nodiscard]] uint64_t +nowNs() { + return static_cast(std::chrono::duration_cast( + std::chrono::system_clock::now().time_since_epoch()) + .count()); +} + +[[nodiscard]] mpStoreSnapshot +makeSnap(const std::string &agent, const std::string &rank) { + mpStoreSnapshot s; + s.pid = ::getpid(); + s.startTime = 1; + s.lastUpdateNs = nowNs(); + s.agentName = agent; + s.hostname = "host"; + s.dpRank = rank; + return s; +} + +[[nodiscard]] const prometheus::MetricFamily * +findFamily(const std::vector &fams, const std::string &name) { + for (const auto &f : fams) { + if (f.name == name) { + return &f; + } + } + return nullptr; +} + +[[nodiscard]] const prometheus::ClientMetric * +findByLabel(const prometheus::MetricFamily &fam, const std::string &key, const std::string &value) { + for (const auto &m : fam.metric) { + for (const auto &l : m.label) { + if (l.name == key && l.value == value) { + return &m; + } + } + } + return nullptr; +} + +[[nodiscard]] bool +hasLabel(const prometheus::ClientMetric &m, const std::string &key) { + for (const auto &l : m.label) { + if (l.name == key) { + return true; + } + } + return false; +} + +TEST(MpCollectorTest, EmptySnapshotsYieldNoFamilies) { + EXPECT_TRUE(buildMetricFamilies({}).empty()); +} + +TEST(MpCollectorTest, PerProcessCountersAndGauges) { + auto a = makeSnap("agent-a", "0"); + a.counters[idx(TX_BYTES)] = 1000; + a.gauges[idx(TX_BYTES)] = 200; + auto b = makeSnap("agent-b", "1"); + b.counters[idx(TX_BYTES)] = 50; + b.gauges[idx(TX_BYTES)] = 50; + + const auto fams = buildMetricFamilies({a, b}); + + const auto *tx = findFamily(fams, "agent_tx_bytes_total"); + ASSERT_NE(tx, nullptr); + EXPECT_EQ(tx->type, prometheus::MetricType::Counter); + ASSERT_EQ(tx->metric.size(), 2u); + const auto *tx_a = findByLabel(*tx, "agent_name", "agent-a"); + const auto *tx_b = findByLabel(*tx, "agent_name", "agent-b"); + ASSERT_NE(tx_a, nullptr); + ASSERT_NE(tx_b, nullptr); + EXPECT_DOUBLE_EQ(tx_a->counter.value, 1000.0); + EXPECT_DOUBLE_EQ(tx_b->counter.value, 50.0); + + const auto *gauge = findFamily(fams, "agent_tx_last_bytes"); + ASSERT_NE(gauge, nullptr); + EXPECT_EQ(gauge->type, prometheus::MetricType::Gauge); + const auto *g_a = findByLabel(*gauge, "agent_name", "agent-a"); + ASSERT_NE(g_a, nullptr); + EXPECT_DOUBLE_EQ(g_a->gauge.value, 200.0); +} + +TEST(MpCollectorTest, DpRankLabelOnlyWhenPresent) { + const auto with_rank = makeSnap("agent-a", "3"); + const auto without_rank = makeSnap("agent-b", ""); + + const auto fams = buildMetricFamilies({with_rank, without_rank}); + const auto *tx = findFamily(fams, "agent_tx_bytes_total"); + ASSERT_NE(tx, nullptr); + + const auto *m_with = findByLabel(*tx, "agent_name", "agent-a"); + const auto *m_without = findByLabel(*tx, "agent_name", "agent-b"); + ASSERT_NE(m_with, nullptr); + ASSERT_NE(m_without, nullptr); + EXPECT_TRUE(hasLabel(*m_with, "dp_rank")); + EXPECT_FALSE(hasLabel(*m_without, "dp_rank")); +} + +TEST(MpCollectorTest, ErrorFamilyCarriesStatusLabel) { + auto a = makeSnap("agent-a", "0"); + a.counters[idx(ERR_BACKEND)] = 5; + + const auto fams = buildMetricFamilies({a}); + const auto *errors = findFamily(fams, "agent_errors_total"); + ASSERT_NE(errors, nullptr); + EXPECT_EQ(errors->type, prometheus::MetricType::Counter); + + const auto *backend = findByLabel(*errors, "status", "backend"); + ASSERT_NE(backend, nullptr); + EXPECT_DOUBLE_EQ(backend->counter.value, 5.0); + const auto *canceled = findByLabel(*errors, "status", "canceled"); + ASSERT_NE(canceled, nullptr); + EXPECT_DOUBLE_EQ(canceled->counter.value, 0.0); +} + +TEST(MpCollectorTest, IsProcessAliveGuardsPidReuse) { + EXPECT_TRUE(isProcessAlive(::getpid(), readProcessStartTime(::getpid()))); + EXPECT_TRUE(isProcessAlive(::getpid(), 0)); // unknown start time -> existence only + EXPECT_FALSE(isProcessAlive(0x7fffffff, 0)); // no such process + // Existing pid but wrong start time -> treated as reused -> not our process. + EXPECT_FALSE(isProcessAlive(::getpid(), 0xffffffffffffffffULL)); +} + +TEST(MpCollectorTest, SnapshotLivenessByProcessThenTtl) { + const auto ttl = std::chrono::seconds(30); + + auto alive = makeSnap("a", ""); + alive.startTime = readProcessStartTime(::getpid()); + alive.lastUpdateNs = 0; // even with an old heartbeat, a live process stays live + EXPECT_TRUE(isSnapshotLive(alive, ttl)); + + auto dead_fresh = makeSnap("b", ""); + dead_fresh.pid = 0x7fffffff; + dead_fresh.lastUpdateNs = nowNs(); + EXPECT_TRUE(isSnapshotLive(dead_fresh, ttl)); + + auto dead_stale = makeSnap("c", ""); + dead_stale.pid = 0x7fffffff; + dead_stale.lastUpdateNs = nowNs() - std::chrono::nanoseconds(std::chrono::seconds(60)).count(); + EXPECT_FALSE(isSnapshotLive(dead_stale, ttl)); +} + +class MpCollectorFileTest : public ::testing::Test { +protected: + void + SetUp() override { + const auto *info = ::testing::UnitTest::GetInstance()->current_test_info(); + dir_ = std::filesystem::path(::testing::TempDir()) / + ("nixl_mp_collector_" + std::to_string(::getpid()) + "_" + info->name()); + std::filesystem::create_directories(dir_); + } + + void + TearDown() override { + std::error_code ec; + std::filesystem::remove_all(dir_, ec); + } + + std::filesystem::path dir_; +}; + +TEST_F(MpCollectorFileTest, CollectReadsLiveStoresAndIgnoresOthers) { + // Two distinct store files; both headers stamp this (live) process. + mpStoreWriter w1(dir_ / makeStoreFileName(111, 1), "agent-1", "host", "0"); + w1.addCounter(TX_BYTES, 500); + w1.setGauge(TX_BYTES, 500); + mpStoreWriter w2(dir_ / makeStoreFileName(222, 2), "agent-2", "host", "1"); + w2.addCounter(TX_BYTES, 700); + + // A non-store file must be ignored. + { std::ofstream(dir_ / "unrelated.txt") << "ignore me"; } + + nixlMultiprocessCollector collector(dir_, std::chrono::seconds(30), /*reap_stale=*/false); + const auto fams = collector.Collect(); + + const auto *tx = findFamily(fams, "agent_tx_bytes_total"); + ASSERT_NE(tx, nullptr); + ASSERT_EQ(tx->metric.size(), 2u); + const auto *m1 = findByLabel(*tx, "agent_name", "agent-1"); + const auto *m2 = findByLabel(*tx, "agent_name", "agent-2"); + ASSERT_NE(m1, nullptr); + ASSERT_NE(m2, nullptr); + EXPECT_DOUBLE_EQ(m1->counter.value, 500.0); + EXPECT_DOUBLE_EQ(m2->counter.value, 700.0); +} + +TEST_F(MpCollectorFileTest, CollectOnEmptyDirYieldsNoFamilies) { + nixlMultiprocessCollector collector(dir_, std::chrono::seconds(30), /*reap_stale=*/false); + EXPECT_TRUE(collector.Collect().empty()); +} + +} // namespace From 5f71e4e0d8a4b67093e6da1923f0abf02e3c9346 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 19:00:29 +0000 Subject: [PATCH 03/71] telemetry: label mp series by pid to guarantee uniqueness The collector's per-process series were disambiguated only by agent_name (plus optional dp_rank). If two processes share an agent name and have no dp_rank, they would emit an identical label set -> a duplicate Prometheus series and a rejected scrape. Add an unconditional pid label so every live process is a distinct series regardless of how callers name agents. pid (not the reserved "instance" target label) also avoids the exported_instance renaming trap. Part of NIX-1614. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/mp_collector.cpp | 4 ++++ test/gtest/telemetry_mp_collector_test.cpp | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index c7ba703750..8dbd8d227b 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -49,6 +49,10 @@ namespace { std::vector labels; labels.push_back({"hostname", s.hostname}); labels.push_back({"agent_name", s.agentName}); + // pid guarantees per-process series uniqueness even if agent names are + // not unique across processes; avoids duplicate-series scrape errors. Not + // named "instance" (a reserved Prometheus target label). + labels.push_back({"pid", std::to_string(s.pid)}); if (!s.dpRank.empty()) { labels.push_back({"dp_rank", s.dpRank}); } diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp index 921a69a579..c8db73727c 100644 --- a/test/gtest/telemetry_mp_collector_test.cpp +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -135,6 +135,29 @@ TEST(MpCollectorTest, PerProcessCountersAndGauges) { EXPECT_DOUBLE_EQ(g_a->gauge.value, 200.0); } +TEST(MpCollectorTest, PidLabelDisambiguatesSameAgentName) { + // Two processes that (mis)use the same agent name and no dp_rank must still + // produce distinct series, keyed by pid, rather than a duplicate series. + auto a = makeSnap("dup", ""); + a.pid = 1001; + a.counters[idx(TX_BYTES)] = 10; + auto b = makeSnap("dup", ""); + b.pid = 1002; + b.counters[idx(TX_BYTES)] = 20; + + const auto fams = buildMetricFamilies({a, b}); + const auto *tx = findFamily(fams, "agent_tx_bytes_total"); + ASSERT_NE(tx, nullptr); + ASSERT_EQ(tx->metric.size(), 2u); + const auto *m_a = findByLabel(*tx, "pid", "1001"); + const auto *m_b = findByLabel(*tx, "pid", "1002"); + ASSERT_NE(m_a, nullptr); + ASSERT_NE(m_b, nullptr); + EXPECT_DOUBLE_EQ(m_a->counter.value, 10.0); + EXPECT_DOUBLE_EQ(m_b->counter.value, 20.0); + EXPECT_TRUE(hasLabel(*m_a, "pid")); +} + TEST(MpCollectorTest, DpRankLabelOnlyWhenPresent) { const auto with_rank = makeSnap("agent-a", "3"); const auto without_rank = makeSnap("agent-b", ""); From 000eaa357417f490de9a2085f8ce13c359d4b0ac Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 19:10:23 +0000 Subject: [PATCH 04/71] telemetry: add prometheus_mp exporter, plugin, and bind-race election Third component of the prometheus_mp plugin: the exporter that ties the store and collector together, selectable via NIXL_TELEMETRY_EXPORTER=prometheus_mp. Every process writes its own metric state to a per-process store in NIXL_TELEMETRY_MULTIPROC_DIR (unique file per pid/instance). On construction each process races to bind the scrape port: the winner runs a prometheus-cpp Exposer plus a nixlMultiprocessCollector that aggregates all peers on each scrape; losers fall back to writer-only mode. A bind collision is caught internally and never rethrown as nixlTelemetryBindFailed, so every process -- owner or writer -- gets a valid telemetry sink and all ranks are exported behind the single owner endpoint. Config: NIXL_TELEMETRY_MULTIPROC_DIR (required), NIXL_TELEMETRY_RANK_ENV (optional dp_rank source, default LOCAL_RANK), NIXL_TELEMETRY_MP_STALE_TTL; reuses NIXL_TELEMETRY_PROMETHEUS_PORT / _LOCAL. The exporter is built both as the installable plugin .so and as a static library for direct unit testing (owner mode, writer-mode-on-collision, missing-dir). Gated on prometheus-cpp. Part of NIX-1614. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/meson.build | 32 ++++ .../telemetry/prometheus_mp/mp_store.cpp | 5 +- .../telemetry/prometheus_mp/mp_store.h | 8 +- .../prometheus_mp/prometheus_mp_exporter.cpp | 163 ++++++++++++++++++ .../prometheus_mp/prometheus_mp_exporter.h | 64 +++++++ .../prometheus_mp/prometheus_mp_plugin.cpp | 32 ++++ test/gtest/meson.build | 13 +- test/gtest/telemetry_mp_collector_test.cpp | 4 +- test/gtest/telemetry_mp_exporter_test.cpp | 131 ++++++++++++++ 9 files changed, 442 insertions(+), 10 deletions(-) create mode 100644 src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp create mode 100644 src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h create mode 100644 src/plugins/telemetry/prometheus_mp/prometheus_mp_plugin.cpp create mode 100644 test/gtest/telemetry_mp_exporter_test.cpp diff --git a/src/plugins/telemetry/prometheus_mp/meson.build b/src/plugins/telemetry/prometheus_mp/meson.build index c8fd2a3025..4d08ccda01 100644 --- a/src/plugins/telemetry/prometheus_mp/meson.build +++ b/src/plugins/telemetry/prometheus_mp/meson.build @@ -52,4 +52,36 @@ if prometheus_found include_directories: prometheus_mp_inc_dirs, dependencies: [nixl_common_dep, prometheus_core_dep, nixl_telemetry_mp_store_dep], ) + + # Exporter with bind-race election. Built as a static library too so it can be + # unit-tested directly (in addition to being packaged into the plugin .so). + telemetry_mp_exporter_lib = static_library( + 'nixl_telemetry_mp_exporter', + 'prometheus_mp_exporter.cpp', + include_directories: [nixl_inc_dirs, utils_inc_dirs, prometheus_mp_inc_dirs], + dependencies: [nixl_common_dep, prometheus_core_dep, prometheus_pull_dep, + nixl_telemetry_mp_store_dep, nixl_telemetry_mp_collector_dep], + install: false, + pic: true, + ) + + nixl_telemetry_mp_exporter_dep = declare_dependency( + link_with: telemetry_mp_exporter_lib, + include_directories: prometheus_mp_inc_dirs, + dependencies: [nixl_common_dep, prometheus_core_dep, prometheus_pull_dep, + nixl_telemetry_mp_store_dep, nixl_telemetry_mp_collector_dep], + ) + + # Installable plugin: selected via NIXL_TELEMETRY_EXPORTER=prometheus_mp. + prometheus_mp_exporter_plugin = shared_library( + 'libtelemetry_exporter_prometheus_mp', + 'prometheus_mp_plugin.cpp', + include_directories: [nixl_inc_dirs, utils_inc_dirs, prometheus_mp_inc_dirs], + dependencies: [nixl_infra, nixl_common_dep, absl_log_dep, prometheus_core_dep, + prometheus_pull_dep, nixl_telemetry_mp_exporter_dep], + install: true, + install_dir: plugin_install_dir, + name_prefix: '', + ) + message('Building Prometheus multiprocess telemetry exporter plugin') endif diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index c9fef4bf00..6e0a9c20b9 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -87,9 +87,10 @@ namespace { } // namespace std::string -makeStoreFileName(int64_t pid, uint64_t start_time) { +makeStoreFileName(int64_t pid, uint64_t start_time, uint64_t instance) { return std::string(MP_STORE_FILE_PREFIX) + std::to_string(pid) + "." + - std::to_string(start_time) + std::string(MP_STORE_FILE_SUFFIX); + std::to_string(start_time) + "." + std::to_string(instance) + + std::string(MP_STORE_FILE_SUFFIX); } uint64_t diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index b907867b29..9754a4f0bb 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -148,13 +148,15 @@ readStoreSnapshot(const std::filesystem::path &path); readProcessStartTime(int64_t pid); /** - * @brief Builds this process's store file name (MP_STORE_FILE_PREFIX / suffix). + * @brief Builds a store file name (MP_STORE_FILE_PREFIX / suffix). * @param pid Process id. * @param start_time Process start time (disambiguates PID reuse across restarts). - * @return File name of the form "nixl...mmap". + * @param instance Per-process instance counter (disambiguates multiple agents in + * the same process so their store files never collide). + * @return File name of the form "nixl....mmap". */ [[nodiscard]] std::string -makeStoreFileName(int64_t pid, uint64_t start_time); +makeStoreFileName(int64_t pid, uint64_t start_time, uint64_t instance); } // namespace nixl::telemetry::mp diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp new file mode 100644 index 0000000000..20f6a8db42 --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -0,0 +1,163 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "prometheus_mp_exporter.h" + +#include "common/configuration.h" +#include "common/nixl_log.h" + +#include + +#include +#include +#include +#include +#include +#include + +namespace { + +using nixl::telemetry::mp::makeStoreFileName; +using nixl::telemetry::mp::mpStoreWriter; +using nixl::telemetry::mp::nixlMultiprocessCollector; +using nixl::telemetry::mp::readProcessStartTime; + +constexpr uint16_t defaultPort = 9090; +constexpr uint64_t defaultStaleTtlSeconds = 30; +constexpr char defaultRankEnvName[] = "LOCAL_RANK"; + +constexpr char prometheusPortVar[] = "NIXL_TELEMETRY_PROMETHEUS_PORT"; +constexpr char prometheusLocalVar[] = "NIXL_TELEMETRY_PROMETHEUS_LOCAL"; +constexpr char multiprocDirVar[] = "NIXL_TELEMETRY_MULTIPROC_DIR"; +constexpr char rankEnvVar[] = "NIXL_TELEMETRY_RANK_ENV"; +constexpr char staleTtlVar[] = "NIXL_TELEMETRY_MP_STALE_TTL"; + +const std::string localAddress = "127.0.0.1"; +const std::string publicAddress = "0.0.0.0"; + +// civetweb reports a failed port bind with this exact text (as used by the +// single-process prometheus exporter). Only this case is treated as a benign +// bind collision; any other Exposer failure is a genuine error. +constexpr char bindFailureMarker[] = "Failed to setup server ports"; + +[[nodiscard]] std::string +getHostname() { + char hostname[HOST_NAME_MAX + 1]; + if (gethostname(hostname, sizeof(hostname)) == 0) { + hostname[HOST_NAME_MAX] = '\0'; + return std::string(hostname); + } + return "unknown"; +} + +// Resolves the optional dp_rank label value: NIXL_TELEMETRY_RANK_ENV names which +// env var holds the rank (default LOCAL_RANK); the value of that env var is the +// rank. Empty when the named env var is unset -- rank is a best-effort label only. +[[nodiscard]] std::string +resolveDpRank() { + const std::string rank_source = + nixl::config::getValueDefaulted(rankEnvVar, defaultRankEnvName); + if (rank_source.empty()) { + return {}; + } + const char *value = std::getenv(rank_source.c_str()); + return value != nullptr ? std::string(value) : std::string(); +} + +[[nodiscard]] std::chrono::nanoseconds +resolveStaleTtl() { + const uint64_t seconds = + nixl::config::getValueDefaulted(staleTtlVar, defaultStaleTtlSeconds); + return std::chrono::duration_cast(std::chrono::seconds(seconds)); +} + +[[nodiscard]] std::filesystem::path +resolveMultiprocDir() { + const auto dir = nixl::config::getValueOptional(multiprocDirVar); + if (!dir || dir->empty()) { + throw std::runtime_error( + "prometheus_mp exporter requires NIXL_TELEMETRY_MULTIPROC_DIR to be set"); + } + std::filesystem::path path(*dir); + std::error_code ec; + std::filesystem::create_directories(path, ec); + if (ec) { + throw std::runtime_error("prometheus_mp exporter: cannot create telemetry dir '" + + path.string() + "': " + ec.message()); + } + return path; +} + +// Per-process instance counter so multiple agents in one process get distinct +// store files. +std::atomic s_instanceSeq{0}; + +} // namespace + +nixlTelemetryPrometheusMpExporter::nixlTelemetryPrometheusMpExporter( + const nixlTelemetryExporterInitParams &init_params) + : nixlTelemetryExporter(init_params) { + const std::filesystem::path dir = resolveMultiprocDir(); + + const int64_t pid = static_cast(::getpid()); + const uint64_t start_time = readProcessStartTime(pid); + const uint64_t instance = s_instanceSeq.fetch_add(1, std::memory_order_relaxed); + const std::filesystem::path store_path = dir / makeStoreFileName(pid, start_time, instance); + + store_ = std::make_unique( + store_path, init_params.agentName, getHostname(), resolveDpRank()); + + const bool local = nixl::config::getValueDefaulted(prometheusLocalVar, false); + const uint16_t port = nixl::config::getValueDefaulted(prometheusPortVar, defaultPort); + const std::string bind_address = + (local ? localAddress : publicAddress) + ":" + std::to_string(port); + + try { + auto exposer = std::make_shared(bind_address); + collector_ = std::make_shared(dir, resolveStaleTtl()); + exposer->RegisterCollectable(collector_); + exposer_ = std::move(exposer); + owner_ = true; + NIXL_INFO << "prometheus_mp exporter (owner) serving " << bind_address + << ", aggregating telemetry dir " << dir.string(); + } + catch (const std::exception &e) { + if (std::string(e.what()).find(bindFailureMarker) == std::string::npos) { + throw; + } + owner_ = false; + NIXL_INFO << "prometheus_mp exporter (writer): endpoint " << bind_address + << " owned by another process; agent '" << init_params.agentName + << "' writing to " << store_path.string(); + } +} + +nixlTelemetryPrometheusMpExporter::~nixlTelemetryPrometheusMpExporter() = default; + +nixl_status_t +nixlTelemetryPrometheusMpExporter::exportEvent(const nixlTelemetryEvent &event) { + const auto type = event.eventType_; + const auto descriptor = nixlEnumStrings::telemetryMetricDescriptor(type); + const bool is_error = nixlEnumStrings::telemetryErrorStatusLabel(type) != nullptr; + + if (descriptor.counterName != nullptr || is_error) { + store_->addCounter(type, event.value_); + } + if (descriptor.gaugeName != nullptr) { + store_->setGauge(type, event.value_); + } + return NIXL_SUCCESS; +} diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h new file mode 100644 index 0000000000..919040befe --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h @@ -0,0 +1,64 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_PROMETHEUS_MP_EXPORTER_H +#define NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_PROMETHEUS_MP_EXPORTER_H + +#include "telemetry/telemetry_exporter.h" +#include "telemetry_event.h" +#include "mp_collector.h" +#include "mp_store.h" + +#include + +#include + +/** + * @class nixlTelemetryPrometheusMpExporter + * @brief Multi-process Prometheus exporter with bind-race owner election. + * + * Every process writes its own metric state to a per-process store in the shared + * NIXL_TELEMETRY_MULTIPROC_DIR. On construction each process races to bind the + * scrape port: the winner ("owner") runs a prometheus-cpp Exposer plus a + * nixlMultiprocessCollector that aggregates all peers' stores on each scrape; the + * losers run in writer-only mode (no HTTP server). A bind collision is therefore + * benign -- it never throws nixlTelemetryBindFailed -- so every process gets a + * valid telemetry sink and all ranks are exported behind the single owner port. + */ +class nixlTelemetryPrometheusMpExporter final : public nixlTelemetryExporter { +public: + explicit nixlTelemetryPrometheusMpExporter(const nixlTelemetryExporterInitParams &init_params); + ~nixlTelemetryPrometheusMpExporter() override; + + nixl_status_t + exportEvent(const nixlTelemetryEvent &event) override; + + // True if this process won the bind race and serves the scrape endpoint. + [[nodiscard]] bool + isExporter() const noexcept { + return owner_; + } + +private: + // Declared so destruction is exposer_ -> collector_ -> store_: stop serving + // before dropping the collector it weak-references, then unmap the store. + std::unique_ptr store_; + std::shared_ptr collector_; + std::shared_ptr exposer_; + bool owner_ = false; +}; + +#endif // NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_PROMETHEUS_MP_EXPORTER_H diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_plugin.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_plugin.cpp new file mode 100644 index 0000000000..b6ee1672be --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_plugin.cpp @@ -0,0 +1,32 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "prometheus_mp_exporter.h" +#include "telemetry/telemetry_plugin.h" +#include "telemetry/telemetry_exporter.h" + +using prometheus_mp_exporter_plugin_t = + nixlTelemetryPluginCreator; + +extern "C" NIXL_TELEMETRY_PLUGIN_EXPORT nixlTelemetryPlugin * +nixl_telemetry_plugin_init() { + return prometheus_mp_exporter_plugin_t::create( + nixl_telemetry_plugin_api_version::V2, "prometheus_mp", "1.0.0"); +} + +extern "C" NIXL_TELEMETRY_PLUGIN_EXPORT void +nixl_telemetry_plugin_fini() {} diff --git a/test/gtest/meson.build b/test/gtest/meson.build index 328b52c120..2be51dc507 100644 --- a/test/gtest/meson.build +++ b/test/gtest/meson.build @@ -111,8 +111,8 @@ else ucx_hw_warning_dep = [] endif -# prometheus_mp collector unit test rides the main gtest binary, but only when -# prometheus-cpp (and thus the collector library) is available. +# prometheus_mp collector/exporter unit tests ride the main gtest binary, but +# only when prometheus-cpp (and thus those libraries) is available. if is_variable('nixl_telemetry_mp_collector_dep') gtest_sources += 'telemetry_mp_collector_test.cpp' telemetry_mp_collector_dep = [nixl_telemetry_mp_collector_dep] @@ -120,11 +120,18 @@ else telemetry_mp_collector_dep = [] endif +if is_variable('nixl_telemetry_mp_exporter_dep') + gtest_sources += 'telemetry_mp_exporter_test.cpp' + telemetry_mp_exporter_dep = [nixl_telemetry_mp_exporter_dep] +else + telemetry_mp_exporter_dep = [] +endif + test_exe = executable('gtest', sources : gtest_sources, include_directories: [nixl_inc_dirs, utils_inc_dirs, device_api_inc, include_directories('../doca-telemetry')], cpp_args : cpp_flags, - dependencies : [nixl_dep, nixl_common_dep, cuda_dependencies, device_api_dep, gtest_dep, gmock_dep, absl_strings_dep, absl_time_dep, ucx_hw_warning_dep, file_utils_interface, nixl_telemetry_mp_store_dep, telemetry_mp_collector_dep], + dependencies : [nixl_dep, nixl_common_dep, cuda_dependencies, device_api_dep, gtest_dep, gmock_dep, absl_strings_dep, absl_time_dep, ucx_hw_warning_dep, file_utils_interface, nixl_telemetry_mp_store_dep, telemetry_mp_collector_dep, telemetry_mp_exporter_dep], link_with: [nixl_build_lib], install : true ) diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp index c8db73727c..394a4fcd49 100644 --- a/test/gtest/telemetry_mp_collector_test.cpp +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -239,10 +239,10 @@ class MpCollectorFileTest : public ::testing::Test { TEST_F(MpCollectorFileTest, CollectReadsLiveStoresAndIgnoresOthers) { // Two distinct store files; both headers stamp this (live) process. - mpStoreWriter w1(dir_ / makeStoreFileName(111, 1), "agent-1", "host", "0"); + mpStoreWriter w1(dir_ / makeStoreFileName(111, 1, 0), "agent-1", "host", "0"); w1.addCounter(TX_BYTES, 500); w1.setGauge(TX_BYTES, 500); - mpStoreWriter w2(dir_ / makeStoreFileName(222, 2), "agent-2", "host", "1"); + mpStoreWriter w2(dir_ / makeStoreFileName(222, 2, 0), "agent-2", "host", "1"); w2.addCounter(TX_BYTES, 700); // A non-store file must be ignored. diff --git a/test/gtest/telemetry_mp_exporter_test.cpp b/test/gtest/telemetry_mp_exporter_test.cpp new file mode 100644 index 0000000000..0fc83479b2 --- /dev/null +++ b/test/gtest/telemetry_mp_exporter_test.cpp @@ -0,0 +1,131 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "prometheus_mp_exporter.h" +#include "mp_store.h" + +#include "common.h" + +#include + +#include + +#include + +#include +#include +#include + +namespace { + +using nixl::telemetry::mp::readStoreSnapshot; + +constexpr auto TX_BYTES = nixl_telemetry_event_type_t::AGENT_TX_BYTES; +constexpr auto RX_BYTES = nixl_telemetry_event_type_t::AGENT_RX_BYTES; + +[[nodiscard]] std::size_t +idx(nixl_telemetry_event_type_t t) { + return static_cast(t); +} + +[[nodiscard]] nixlTelemetryExporterInitParams +initParams(const std::string &agent) { + return nixlTelemetryExporterInitParams{agent, 4096}; +} + +class MpExporterTest : public ::testing::Test { +protected: + void + SetUp() override { + const auto *info = ::testing::UnitTest::GetInstance()->current_test_info(); + dir_ = std::filesystem::path(::testing::TempDir()) / + ("nixl_mp_exporter_" + std::to_string(::getpid()) + "_" + info->name()); + std::filesystem::create_directories(dir_); + port_ = gtest::PortAllocator::next_tcp_port(); + env_.addVar("NIXL_TELEMETRY_PROMETHEUS_LOCAL", "y"); + env_.addVar("NIXL_TELEMETRY_PROMETHEUS_PORT", std::to_string(port_)); + env_.addVar("NIXL_TELEMETRY_MULTIPROC_DIR", dir_.string()); + } + + void + TearDown() override { + env_.popVar(); + env_.popVar(); + env_.popVar(); + std::error_code ec; + std::filesystem::remove_all(dir_, ec); + } + + [[nodiscard]] std::filesystem::path + singleStoreFile() const { + std::error_code ec; + for (const auto &entry : std::filesystem::directory_iterator(dir_, ec)) { + const auto name = entry.path().filename().string(); + if (name.rfind("nixl.", 0) == 0 && name.size() > 5 && + name.substr(name.size() - 5) == ".mmap") { + return entry.path(); + } + } + return {}; + } + + gtest::ScopedEnv env_; + uint16_t port_ = 0; + std::filesystem::path dir_; +}; + +TEST_F(MpExporterTest, OwnerBindsAndRecordsToStore) { + nixlTelemetryPrometheusMpExporter exporter(initParams("agent-owner")); + EXPECT_TRUE(exporter.isExporter()); + + const auto file = singleStoreFile(); + ASSERT_FALSE(file.empty()); + + exporter.exportEvent({TX_BYTES, 1234}); + + const auto snap = readStoreSnapshot(file); + ASSERT_TRUE(snap.has_value()); + EXPECT_EQ(snap->agentName, "agent-owner"); + EXPECT_EQ(snap->counters[idx(TX_BYTES)], 1234u); + EXPECT_EQ(snap->gauges[idx(TX_BYTES)], 1234u); +} + +TEST_F(MpExporterTest, WriterModeWhenPortTaken) { + // Occupy the port first so the exporter loses the bind race. + prometheus::Exposer blocker("127.0.0.1:" + std::to_string(port_)); + + nixlTelemetryPrometheusMpExporter exporter(initParams("agent-writer")); + EXPECT_FALSE(exporter.isExporter()); + + const auto file = singleStoreFile(); + ASSERT_FALSE(file.empty()); + + exporter.exportEvent({RX_BYTES, 77}); + + const auto snap = readStoreSnapshot(file); + ASSERT_TRUE(snap.has_value()); + EXPECT_EQ(snap->agentName, "agent-writer"); + EXPECT_EQ(snap->counters[idx(RX_BYTES)], 77u); +} + +TEST(MpExporterStandaloneTest, MissingMultiprocDirThrows) { + ::unsetenv("NIXL_TELEMETRY_MULTIPROC_DIR"); + EXPECT_THROW( + { nixlTelemetryPrometheusMpExporter exporter(nixlTelemetryExporterInitParams{"a", 4096}); }, + std::runtime_error); +} + +} // namespace From 30f12dd3326cf885f773a2619ef1009b415b9eda Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 19:16:24 +0000 Subject: [PATCH 05/71] telemetry: add prometheus_mp multiprocess e2e forking test Fourth component of the prometheus_mp plugin: an end-to-end test that proves cross-process aggregation with real processes. The parent becomes the bind-race owner and serves the endpoint; N children fork (while the parent is still single-threaded) and run as writers against the shared dir. After a single HTTP scrape of the owner port, all N+1 processes appear as distinct per-process series. One child is then killed and reaped, and a second scrape confirms its series is dropped and its store file removed (stale TTL 0). Part of NIX-1614. Signed-off-by: Efraim Eygin --- test/gtest/meson.build | 1 + test/gtest/telemetry_mp_e2e_test.cpp | 255 +++++++++++++++++++++++++++ 2 files changed, 256 insertions(+) create mode 100644 test/gtest/telemetry_mp_e2e_test.cpp diff --git a/test/gtest/meson.build b/test/gtest/meson.build index 2be51dc507..b8d33b6780 100644 --- a/test/gtest/meson.build +++ b/test/gtest/meson.build @@ -122,6 +122,7 @@ endif if is_variable('nixl_telemetry_mp_exporter_dep') gtest_sources += 'telemetry_mp_exporter_test.cpp' + gtest_sources += 'telemetry_mp_e2e_test.cpp' telemetry_mp_exporter_dep = [nixl_telemetry_mp_exporter_dep] else telemetry_mp_exporter_dep = [] diff --git a/test/gtest/telemetry_mp_e2e_test.cpp b/test/gtest/telemetry_mp_e2e_test.cpp new file mode 100644 index 0000000000..90bf48e0b9 --- /dev/null +++ b/test/gtest/telemetry_mp_e2e_test.cpp @@ -0,0 +1,255 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "prometheus_mp_exporter.h" + +#include "common.h" + +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +constexpr auto TX_BYTES = nixl_telemetry_event_type_t::AGENT_TX_BYTES; + +[[nodiscard]] nixlTelemetryExporterInitParams +initParams(const std::string &agent) { + return nixlTelemetryExporterInitParams{agent, 4096}; +} + +// Minimal HTTP/1.1 GET over 127.0.0.1:; returns the response body (empty on +// failure). Self-contained to keep the test free of an HTTP client dependency. +[[nodiscard]] std::string +httpGet(uint16_t port, const std::string &path) { + const int fd = ::socket(AF_INET, SOCK_STREAM, 0); + if (fd < 0) { + return {}; + } + const struct timeval tv{3, 0}; + ::setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); + ::setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)); + + sockaddr_in addr{}; + addr.sin_family = AF_INET; + addr.sin_port = htons(port); + addr.sin_addr.s_addr = ::inet_addr("127.0.0.1"); + if (::connect(fd, reinterpret_cast(&addr), sizeof(addr)) < 0) { + ::close(fd); + return {}; + } + + const std::string req = + "GET " + path + " HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n"; + ::send(fd, req.data(), req.size(), 0); + + std::string response; + char buf[4096]; + while (true) { + const ssize_t n = ::recv(fd, buf, sizeof(buf), 0); + if (n <= 0) { + break; + } + response.append(buf, static_cast(n)); + } + ::close(fd); + + const auto pos = response.find("\r\n\r\n"); + return pos == std::string::npos ? std::string{} : response.substr(pos + 4); +} + +[[nodiscard]] std::string +scrapeMetrics(uint16_t port) { + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(3); + do { + const std::string body = httpGet(port, "/metrics"); + if (!body.empty()) { + return body; + } + std::this_thread::sleep_for(std::chrono::milliseconds(25)); + } while (std::chrono::steady_clock::now() < deadline); + return {}; +} + +// Parses "{...agent_name="X"...} " lines into agent_name -> value. +[[nodiscard]] std::map +parseSeriesByAgent(const std::string &body, const std::string &metric) { + std::map out; + const std::string prefix = metric + "{"; + const std::string key = "agent_name=\""; + std::istringstream iss(body); + std::string line; + while (std::getline(iss, line)) { + if (line.rfind(prefix, 0) != 0) { + continue; + } + const auto ap = line.find(key); + const auto brace = line.rfind('}'); + if (ap == std::string::npos || brace == std::string::npos) { + continue; + } + const auto vstart = ap + key.size(); + const auto vend = line.find('"', vstart); + if (vend == std::string::npos) { + continue; + } + try { + out[line.substr(vstart, vend - vstart)] = std::stod(line.substr(brace + 1)); + } + catch (const std::exception &) { + } + } + return out; +} + +void +runWriterChild(int go_fd, int ready_fd, int quit_fd, const std::string &agent, uint64_t tx_value) { + char c = 0; + while (::read(go_fd, &c, 1) > 0) {} + + int rc = 0; + try { + nixlTelemetryPrometheusMpExporter exporter(initParams(agent)); + exporter.exportEvent({TX_BYTES, tx_value}); + const char ok = 1; + ::write(ready_fd, &ok, 1); + // Block until the parent closes the quit pipe. + while (::read(quit_fd, &c, 1) > 0) {} + } + catch (...) { + rc = 3; + } + ::_exit(rc); +} + +class MpE2ETest : public ::testing::Test { +protected: + void + SetUp() override { + const auto *info = ::testing::UnitTest::GetInstance()->current_test_info(); + dir_ = std::filesystem::path(::testing::TempDir()) / + ("nixl_mp_e2e_" + std::to_string(::getpid()) + "_" + info->name()); + std::filesystem::create_directories(dir_); + port_ = gtest::PortAllocator::next_tcp_port(); + env_.addVar("NIXL_TELEMETRY_PROMETHEUS_LOCAL", "y"); + env_.addVar("NIXL_TELEMETRY_PROMETHEUS_PORT", std::to_string(port_)); + env_.addVar("NIXL_TELEMETRY_MULTIPROC_DIR", dir_.string()); + // Dead processes become stale immediately so the reaping check is prompt. + env_.addVar("NIXL_TELEMETRY_MP_STALE_TTL", "0"); + } + + void + TearDown() override { + for (int i = 0; i < 4; ++i) { + env_.popVar(); + } + std::error_code ec; + std::filesystem::remove_all(dir_, ec); + } + + gtest::ScopedEnv env_; + uint16_t port_ = 0; + std::filesystem::path dir_; +}; + +TEST_F(MpE2ETest, AllRankProcessesAggregateBehindOneEndpointAndStaleAreDropped) { + constexpr int kChildren = 3; + + int go_pipe[2]; + int ready_pipe[2]; + int quit_pipe[2]; + ASSERT_EQ(::pipe(go_pipe), 0); + ASSERT_EQ(::pipe(ready_pipe), 0); + ASSERT_EQ(::pipe(quit_pipe), 0); + + // Fork children while the parent is still single-threaded (before it builds + // the owner exporter, which starts civetweb threads). + std::vector children; + for (int i = 0; i < kChildren; ++i) { + const pid_t pid = ::fork(); + ASSERT_GE(pid, 0); + if (pid == 0) { + ::close(go_pipe[1]); + ::close(ready_pipe[0]); + ::close(quit_pipe[1]); + runWriterChild(go_pipe[0], + ready_pipe[1], + quit_pipe[0], + "agent-" + std::to_string(i), + static_cast((i + 1) * 100)); + } + children.push_back(pid); + } + + ::close(go_pipe[0]); + ::close(ready_pipe[1]); + ::close(quit_pipe[0]); + + // Parent becomes the bind-race owner and serves the endpoint. + nixlTelemetryPrometheusMpExporter owner(initParams("agent-parent")); + ASSERT_TRUE(owner.isExporter()); + owner.exportEvent({TX_BYTES, 999}); + + // Release the children (they now become writers) and wait for readiness. + ::close(go_pipe[1]); + for (int i = 0; i < kChildren; ++i) { + char c = 0; + ASSERT_EQ(::read(ready_pipe[0], &c, 1), 1); + } + + // Phase 1: every process must appear behind the single owner endpoint. + const auto phase1 = parseSeriesByAgent(scrapeMetrics(port_), "agent_tx_bytes_total"); + EXPECT_EQ(phase1.size(), static_cast(kChildren + 1)); + EXPECT_DOUBLE_EQ(phase1.at("agent-parent"), 999.0); + EXPECT_DOUBLE_EQ(phase1.at("agent-0"), 100.0); + EXPECT_DOUBLE_EQ(phase1.at("agent-1"), 200.0); + EXPECT_DOUBLE_EQ(phase1.at("agent-2"), 300.0); + + // Kill one child and reap it so its pid is truly gone before the next scrape. + ASSERT_EQ(::kill(children[0], SIGKILL), 0); + int status = 0; + ASSERT_EQ(::waitpid(children[0], &status, 0), children[0]); + + // Phase 2: the dead child's series is dropped (and its store reaped). + const auto phase2 = parseSeriesByAgent(scrapeMetrics(port_), "agent_tx_bytes_total"); + EXPECT_EQ(phase2.count("agent-0"), 0u); + EXPECT_EQ(phase2.count("agent-1"), 1u); + EXPECT_EQ(phase2.count("agent-2"), 1u); + EXPECT_EQ(phase2.count("agent-parent"), 1u); + + // Release the remaining children and reap them. + ::close(quit_pipe[1]); + for (int i = 1; i < kChildren; ++i) { + ::waitpid(children[i], &status, 0); + } + ::close(ready_pipe[0]); +} + +} // namespace From 38b5093c4f625c1e434e0696524864b0469f4b51 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 19:27:28 +0000 Subject: [PATCH 06/71] telemetry: harden mp collector against mid-scrape churn and orphans Handle processes that appear or die mid-scrape more robustly: - Reader treats a zero-magic store (a process still initializing its file, or an orphan from a process killed mid-creation) as a quiet skip instead of a "bad magic" WARN; a genuinely wrong non-zero magic still warns. - Collector now reaps unparseable store files (zero/bad magic, wrong schema, truncated) once they are older than max(stale TTL, 2s). The floor protects a store a live process is actively creating from being deleted out from under it, even when the TTL is 0. This closes a slow file leak: a process killed during store creation left a zero-magic file that was correctly skipped every scrape but never cleaned up. Cleanup is performed by the bind-race owner during Collect(), since a killed process cannot clean up after itself. Part of NIX-1614. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/mp_collector.cpp | 30 +++++++++++++++++++ .../telemetry/prometheus_mp/mp_store.cpp | 10 ++++++- test/gtest/telemetry_mp_collector_test.cpp | 25 ++++++++++++++++ test/gtest/telemetry_mp_store_test.cpp | 17 +++++++++-- 4 files changed, 79 insertions(+), 3 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index 8dbd8d227b..2859037874 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -23,10 +23,13 @@ #include #include +#include #include +#include #include #include +#include #include namespace nixl::telemetry::mp { @@ -75,6 +78,27 @@ namespace { return m; } + // Minimum age before an unparseable file is reaped, even when the TTL is 0. + // Protects a store a live process is actively creating (a sub-second window) + // from being deleted out from under it. + constexpr long kInvalidFileFloorSeconds = 2; + + // Whether an unparseable store file (bad/zero magic, wrong schema, truncated) + // is old enough to be an orphan worth removing, rather than a live process's + // store mid-creation. + [[nodiscard]] bool + invalidFileReapable(const std::filesystem::path &path, std::chrono::nanoseconds ttl) { + struct stat st{}; + if (::stat(path.c_str(), &st) != 0) { + return false; + } + const long ttl_seconds = + static_cast(std::chrono::duration_cast(ttl).count()); + const long grace = std::max(ttl_seconds, kInvalidFileFloorSeconds); + const long age = static_cast(::time(nullptr) - st.st_mtime); + return age > grace; + } + [[nodiscard]] bool nameMatchesStore(const std::string &name) { return name.size() > MP_STORE_FILE_PREFIX.size() + MP_STORE_FILE_SUFFIX.size() && @@ -197,6 +221,12 @@ nixlMultiprocessCollector::Collect() const { } auto snap = readStoreSnapshot(entry.path()); if (!snap) { + // Unparseable (mid-init, orphaned, or incompatible): reap only if it is + // old enough to not be a store a live process is currently creating. + if (reapStale_ && invalidFileReapable(entry.path(), staleTtl_)) { + std::error_code rm_ec; + std::filesystem::remove(entry.path(), rm_ec); + } continue; } if (isSnapshotLive(*snap, staleTtl_)) { diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 6e0a9c20b9..98c8bca538 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -231,7 +231,15 @@ readStoreSnapshot(const std::filesystem::path &path) { const auto *layout = static_cast(mapping); - if (__atomic_load_n(&layout->magic, __ATOMIC_ACQUIRE) != MP_STORE_MAGIC) { + const uint64_t magic = __atomic_load_n(&layout->magic, __ATOMIC_ACQUIRE); + if (magic == 0) { + // Zeroed header: either a store still being initialized by a live process, + // or an orphan left by a process that died mid-creation. Skip quietly (no + // WARN); the collector reaps stale orphans by file age. + ::munmap(mapping, sizeof(mpStoreLayout)); + return std::nullopt; + } + if (magic != MP_STORE_MAGIC) { NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() << "' with bad magic"; ::munmap(mapping, sizeof(mpStoreLayout)); diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp index 394a4fcd49..f38e20b172 100644 --- a/test/gtest/telemetry_mp_collector_test.cpp +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -262,6 +262,31 @@ TEST_F(MpCollectorFileTest, CollectReadsLiveStoresAndIgnoresOthers) { EXPECT_DOUBLE_EQ(m2->counter.value, 700.0); } +TEST_F(MpCollectorFileTest, ReapsOldOrphanFilesButKeepsFreshMidInitFiles) { + const auto writeZeroFile = [](const std::filesystem::path &p) { + std::ofstream f(p, std::ios::binary); + const std::string zeros(64 * 1024, '\0'); + f.write(zeros.data(), static_cast(zeros.size())); + }; + + // Orphan: zero-magic store backdated well past the reap grace -> removed. + const auto orphan = dir_ / makeStoreFileName(999, 1, 0); + writeZeroFile(orphan); + std::filesystem::last_write_time( + orphan, std::filesystem::file_time_type::clock::now() - std::chrono::hours(1)); + + // Fresh mid-init file (a live process just created it) -> must be kept. + const auto fresh = dir_ / makeStoreFileName(998, 1, 0); + writeZeroFile(fresh); + + nixlMultiprocessCollector collector(dir_, std::chrono::seconds(0), /*reap_stale=*/true); + const auto fams = collector.Collect(); + + EXPECT_TRUE(fams.empty()); + EXPECT_FALSE(std::filesystem::exists(orphan)); + EXPECT_TRUE(std::filesystem::exists(fresh)); +} + TEST_F(MpCollectorFileTest, CollectOnEmptyDirYieldsNoFamilies) { nixlMultiprocessCollector collector(dir_, std::chrono::seconds(30), /*reap_stale=*/false); EXPECT_TRUE(collector.Collect().empty()); diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index ac3f386e25..2c9fcab584 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -144,11 +144,24 @@ TEST_F(MpStoreTest, TooSmallFileReturnsNullopt) { EXPECT_FALSE(readStoreSnapshot(path).has_value()); } -TEST_F(MpStoreTest, BadMagicReturnsNullopt) { +TEST_F(MpStoreTest, ZeroMagicReturnsNulloptQuietly) { + const auto path = storePath("zero-magic"); + { + // Large enough to pass the size check, but all-zero: a store mid-creation + // or an orphan. Must be skipped WITHOUT a warning (no LogIgnoreGuard). + std::ofstream f(path, std::ios::binary); + const std::string zeros(64 * 1024, '\0'); + f.write(zeros.data(), static_cast(zeros.size())); + } + EXPECT_FALSE(readStoreSnapshot(path).has_value()); +} + +TEST_F(MpStoreTest, BadMagicWarnsAndReturnsNullopt) { const auto path = storePath("bad-magic"); { - // Large enough to pass the size check, but all-zero -> magic mismatch. std::ofstream f(path, std::ios::binary); + const uint64_t bad_magic = 0xDEADBEEFULL; // non-zero, not our magic + f.write(reinterpret_cast(&bad_magic), sizeof(bad_magic)); const std::string zeros(64 * 1024, '\0'); f.write(zeros.data(), static_cast(zeros.size())); } From 2a624a5e95e04cf3de16c9e9bed1743cbd68faef Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 19:37:17 +0000 Subject: [PATCH 07/71] docs: document the prometheus_mp multi-process telemetry exporter Describe the native single-endpoint multi-process aggregation exporter in docs/telemetry.md: add it to the architecture components, update the multi-process scrape note to point ranks needing full aggregation at it, and add a "Multi-process aggregation" section covering its model, configuration (NIXL_TELEMETRY_MULTIPROC_DIR/RANK_ENV/MP_STALE_TTL), labels, and the explicit limitation that it is hardcoded to NIXL's fixed metric model and cannot carry dynamic per-observation labels. Part of NIX-1614. Signed-off-by: Efraim Eygin --- docs/telemetry.md | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/docs/telemetry.md b/docs/telemetry.md index 2207f83184..e50b4038bb 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -14,6 +14,7 @@ Custom telemetry exporter plug-ins can be created according to [src/plugins/tele 2. **Shared Memory Buffer**: Statically-linked built in implementation of telemetry exporter. Uses shared memory cyclic buffer for efficient event storage and export. 3. **Telemetry Readers**: C++ and Python applications to read and display telemetry data from the cyclic buffer. 4. **Prometheus exporter**: EXPERIMENTAL (beta) Prometheus compatible telemetry exporter, see [src/plugins/telemetry/prometheus/README.md](../src/plugins/telemetry/prometheus/README.md). +5. **Multi-process Prometheus exporter (`prometheus_mp`)**: EXPERIMENTAL native aggregation of all processes of a multi-process run behind one scrape endpoint, without an external service. See [src/plugins/telemetry/prometheus_mp/README.md](../src/plugins/telemetry/prometheus_mp/README.md) and "Multi-process aggregation" below. ### Event Structure @@ -132,7 +133,45 @@ Telemetry is configured by environment variables: - When telemetry is requested but no output sink is configured (neither `NIXL_TELEMETRY_EXPORTER` nor `NIXL_TELEMETRY_DIR`), it falls back to the collect-only NOP exporter: events are collected in-process so `getXferTelemetry()` / `get_xfer_telemetry()` works, but nothing is written out. - If telemetry is enabled but no exporter is set, or the exporter name is empty, then the sink depends on `NIXL_TELEMETRY_DIR` as explained below (falling back to NOP when it is unset). - Set `NIXL_TELEMETRY_EXPORTER=NOP` to explicitly keep telemetry active (events are collected and `getXferTelemetry()` works) while discarding all output. It needs no sink and writes nothing, so it can be used to measure the overhead of the telemetry collection path in isolation. -- Exporters that expose a scrape endpoint (e.g. Prometheus) bind one port per process. Under multi-process runs (e.g. tensor/data parallelism) every rank tries to bind the same port; only one wins. Losing that race is benign and non-fatal: the affected process logs a single warning and runs without a telemetry sink instead of failing agent construction. See [src/plugins/telemetry/prometheus/README.md](../src/plugins/telemetry/prometheus/README.md). +- Exporters that expose a scrape endpoint (e.g. Prometheus) bind one port per process. Under multi-process runs (e.g. tensor/data parallelism) every rank tries to bind the same port; only one wins. With the single-process `prometheus` exporter, losing that race is benign and non-fatal: the affected process logs a single warning and runs without a telemetry sink instead of failing agent construction (see [src/plugins/telemetry/prometheus/README.md](../src/plugins/telemetry/prometheus/README.md)). To instead export **every** rank behind one endpoint, use the `prometheus_mp` exporter (see "Multi-process aggregation" below). + +## Multi-process aggregation (`prometheus_mp`) + +The `prometheus_mp` exporter aggregates the telemetry of all processes of a +multi-process NIXL run behind a **single** Prometheus scrape endpoint, natively +(no DOCA/DTS). Every process writes its own metric state to a per-process +memory-mapped file in a shared directory; the processes race to bind the scrape +port, the winner serves `/metrics` by reading and republishing all live processes' +files on each scrape (labeled per process), and the losers run as writers only. A +bind collision is benign, so no rank is dropped. Full details: +[src/plugins/telemetry/prometheus_mp/README.md](../src/plugins/telemetry/prometheus_mp/README.md). + +### Configuration + +| Variable | Description | Default | +| -------- | ----------- | ------- | +| `NIXL_TELEMETRY_EXPORTER` | Set to `prometheus_mp` to select this exporter | - | +| `NIXL_TELEMETRY_MULTIPROC_DIR` | Shared directory for per-process store files (all ranks to be aggregated must use the same path). **Required.** | - | +| `NIXL_TELEMETRY_PROMETHEUS_PORT` | Scrape port (shared with the `prometheus` exporter) | `9090` | +| `NIXL_TELEMETRY_PROMETHEUS_LOCAL` | Bind `127.0.0.1` instead of `0.0.0.0` | `false` | +| `NIXL_TELEMETRY_RANK_ENV` | Name of the env var holding the rank for the optional `dp_rank` label; no label if that env var is unset | `LOCAL_RANK` | +| `NIXL_TELEMETRY_MP_STALE_TTL` | Seconds after which a dead process's store is stale and reaped | `30` | + +Series are labeled by `hostname`, `agent_name`, `pid` (guarantees uniqueness), and +optionally `dp_rank`. Metric names, types, and semantics are identical to the +single-process `prometheus` exporter. + +### Scope & limitations + +`prometheus_mp` is purpose-built for NIXL's telemetry model, **not** a generic +Prometheus multiprocess store, and it does not reuse Python `prometheus_client`'s +multiprocess format. The metric set is fixed at compile time (positional slots; +names are not stored) and per-process label values are captured once at startup and +never change -- events carry only a numeric value, with no per-observation labels. +It therefore **cannot represent a metric with a dynamic / high-cardinality label** +that varies per observation. No NIXL metric needs that today; if one is ever added, +a different (keyed) store would be required. For aggregation via an external +service, use the DOCA/CollectX exporter instead. ## Cyclic Buffer From 0fa45a18c0850eb1a4c2f5e3a4322a6c8f15916e Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 19:40:27 +0000 Subject: [PATCH 08/71] docs: add prometheus_mp plugin README Document the multi-process Prometheus exporter plug-in alongside the sibling prometheus and doca plugin READMEs: how bind-race owner election and the per-process mmap stores work, configuration (NIXL_TELEMETRY_MULTIPROC_DIR and the optional RANK_ENV / MP_STALE_TTL / port vars), the per-process labels (hostname, agent_name, pid, optional dp_rank), and the explicit scope note that it is purpose-built for NIXL's fixed metric model (no dynamic per-observation labels) rather than a generic Prometheus multiprocess store. Part of NIX-1614. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/prometheus_mp/README.md | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 src/plugins/telemetry/prometheus_mp/README.md diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md new file mode 100644 index 0000000000..6b344099b2 --- /dev/null +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -0,0 +1,114 @@ + + +# NIXL multi-process Prometheus Telemetry exporter plug-in (`prometheus_mp`) + +`prometheus_mp` exposes the telemetry of **all** processes of a multi-process +NIXL run (e.g. tensor/data parallelism) behind a **single** Prometheus scrape +endpoint, without any DOCA/DTS dependency. + +It complements the single-process [`prometheus`](../prometheus/README.md) exporter +(which binds one port per process, so only one rank's metrics are scraped) and the +DOCA/CollectX exporter (which aggregates via an external DTS service). Use +`prometheus_mp` when you want all ranks aggregated natively with no extra +infrastructure. General NIXL telemetry background: [docs/telemetry.md](../../../../docs/telemetry.md). + +## Dependencies + +Same as the `prometheus` plug-in: the bundled prometheus-cpp subproject and +`libcurl` (`libcurl4-openssl-dev` / `libcurl-devel`). + +## How it works + +- **Every process writes its own metric state** to a per-process memory-mapped + file in a shared directory (`NIXL_TELEMETRY_MULTIPROC_DIR`). Updates are + lock-free; there is no serialization. +- **Bind-race owner election.** On startup each process tries to bind the scrape + port. The one that wins ("owner") runs the HTTP endpoint plus a collector that, + on each scrape, reads every live process's file and republishes them as one + exposition. The processes that lose the race run in **writer-only** mode (no + HTTP server). A bind collision is therefore benign -- every process gets a valid + telemetry sink; no rank is dropped and no scary error is logged. +- **Per-process series.** Each process is exported as its own series (cumulative + counters and last-operation gauges), never summed across processes, so + per-process values stay correct and monotonic. +- **Stale handling.** When a process exits, the owner drops its series once the + process is gone (verified by pid + `/proc` start time) or its data ages past the + TTL, and reaps the file. Cleanup is performed by the owner, since a killed + process cannot clean up after itself. + +## Configuration + +```bash +export NIXL_TELEMETRY_ENABLE="y" +export NIXL_TELEMETRY_EXPORTER="prometheus_mp" # selects libtelemetry_exporter_prometheus_mp.so +export NIXL_TELEMETRY_MULTIPROC_DIR="/tmp/nixl_metrics" # REQUIRED: shared by all ranks in the pod +``` + +All ranks that should be aggregated together must point `NIXL_TELEMETRY_MULTIPROC_DIR` +at the **same** directory (e.g. a per-pod `emptyDir` under Kubernetes). + +### Optional configuration + +```bash +# Scrape port (default 9090) and bind scope -- shared with the prometheus plug-in. +export NIXL_TELEMETRY_PROMETHEUS_PORT="" +export NIXL_TELEMETRY_PROMETHEUS_LOCAL="y" # bind 127.0.0.1 instead of 0.0.0.0 + +# Optional dp_rank label: names the env var that holds the rank (default LOCAL_RANK). +# If that env var is unset, no dp_rank label is emitted (series stay unique via pid). +export NIXL_TELEMETRY_RANK_ENV="LOCAL_RANK" + +# Seconds after which a dead process's store is considered stale and reaped (default 30). +export NIXL_TELEMETRY_MP_STALE_TTL="30" +``` + +## Metric labels + +Every series is labeled by: + +- `hostname` -- host where the agent runs. +- `agent_name` -- the agent name given at initialization. +- `pid` -- the producing process id. This guarantees each process is a distinct + series even if agent names collide; it is deliberately **not** named `instance` + (a reserved Prometheus target label). +- `dp_rank` -- **optional**, present only when a rank env var (see + `NIXL_TELEMETRY_RANK_ENV`) is set. +- `status` -- only on `agent_errors_total`, bounded by the fixed `AGENT_ERR_*` set. + +The metric names, types, counter/gauge semantics, and events are identical to the +single-process [`prometheus`](../prometheus/README.md) exporter (same shared +descriptor). + +## Design scope & limitations + +This exporter is **purpose-built for NIXL's telemetry model, not a generic +Prometheus multiprocess store** (in particular it is not compatible with, and does +not reuse, Python `prometheus_client`'s multiprocess format): + +- The metric set is fixed at compile time; slots are positional, so metric names + are never stored in the files. +- Per-process label values (`hostname`, `agent_name`, `pid`, `dp_rank`) are + captured once at startup and never change. Events carry only a numeric value -- + there are **no per-observation labels**. +- Consequently the store **cannot represent a metric with a dynamic / + high-cardinality label** whose value varies per observation. No NIXL metric has + such a label today; if one is ever added, this exporter would need a different + (keyed) store. + +This is the native, dependency-free path. For aggregation through an external +telemetry service, use the DOCA/CollectX exporter (IPC to DTS) instead. From 93f95ae59b4069229e5008070ea3ad2341bc5d94 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 19:48:09 +0000 Subject: [PATCH 09/71] docs: align prometheus_mp shared-dir guidance with Dynamo Clarify that NIXL_TELEMETRY_MULTIPROC_DIR follows Dynamo's PROMETHEUS_MULTIPROC_DIR convention: a shared local folder (not NFS), one per pod/process-family, treated as ephemeral. Note the key difference from Dynamo: NIXL is loaded independently per rank with no parent to propagate the path, so the launcher/operator must set the same directory for every rank (hence it is required, not auto-defaulted). tmpfs is optional, not required. Part of NIX-1614. Signed-off-by: Efraim Eygin --- docs/telemetry.md | 7 +++++++ src/plugins/telemetry/prometheus_mp/README.md | 19 +++++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/docs/telemetry.md b/docs/telemetry.md index e50b4038bb..39e43b00df 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -161,6 +161,13 @@ Series are labeled by `hostname`, `agent_name`, `pid` (guarantees uniqueness), a optionally `dp_rank`. Metric names, types, and semantics are identical to the single-process `prometheus` exporter. +`NIXL_TELEMETRY_MULTIPROC_DIR` follows Dynamo's `PROMETHEUS_MULTIPROC_DIR` +convention: a shared **local** folder (not NFS), one per pod / process-family, +treated as ephemeral (e.g. a per-pod Kubernetes `emptyDir`). Because NIXL is a +library loaded independently per rank -- with no parent to propagate the path as in +Dynamo -- the launcher/operator must set the **same** directory for every rank, so +it is required rather than auto-defaulted. + ### Scope & limitations `prometheus_mp` is purpose-built for NIXL's telemetry model, **not** a generic diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index 6b344099b2..ae81667381 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -59,8 +59,23 @@ export NIXL_TELEMETRY_EXPORTER="prometheus_mp" # selects libtelemetry_exporter_p export NIXL_TELEMETRY_MULTIPROC_DIR="/tmp/nixl_metrics" # REQUIRED: shared by all ranks in the pod ``` -All ranks that should be aggregated together must point `NIXL_TELEMETRY_MULTIPROC_DIR` -at the **same** directory (e.g. a per-pod `emptyDir` under Kubernetes). +This mirrors Dynamo's `PROMETHEUS_MULTIPROC_DIR` convention (a shared folder that +every related process writes into, one leader exports): all ranks that should be +aggregated together must point `NIXL_TELEMETRY_MULTIPROC_DIR` at the **same** +directory. Unlike Dynamo -- which auto-creates a temp dir in the parent and lets +child engine processes inherit it -- NIXL is a library loaded independently in each +rank, so there is no parent to propagate the path; the launcher/operator must set +the same directory for every rank (hence it is required, not auto-defaulted, so a +per-process temp dir can never silently break aggregation). + +Recommended, following Dynamo's model: a shared **local** folder, one per pod / +process-family, treated as ephemeral (e.g. a per-pod Kubernetes `emptyDir`, or a +temp dir cleaned between runs). It must be a local filesystem -- **not** a network +filesystem (NFS/CIFS), where mmap `MAP_SHARED` cross-process visibility is not +guaranteed (the same restriction Dynamo's multiprocess dir has). tmpfs (e.g. a +Memory-medium `emptyDir` or `/dev/shm`) works and avoids any disk writeback, but is +optional -- a plain local dir is fine, since updates hit the page cache and the +per-process store files are ~one page each. ### Optional configuration From b4093af4999d3cb2532dca893a30a7a67279d498 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 20:18:11 +0000 Subject: [PATCH 10/71] telemetry: fix codespell (reuse nixlTime::getNs, unparsable spelling) CI pre-commit (codespell) flagged the local nowNs() helper (read as "knowns/nouns") and "unparseable". - Replace the duplicated per-file nowNs() helpers with the existing nixlTime::getNs() (CLOCK_MONOTONIC, host-wide so comparable across processes; also skew-free vs wall clock for the staleness delta). The last-update timestamp is only ever compared against another getNs() reading, so switching from system_clock to steady_clock is safe; the orphan-file mtime path is independent and still uses wall-clock time(). - Spell "unparseable" as "unparsable" in comments. Part of NIX-1614. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/mp_collector.cpp | 16 +++++----------- src/plugins/telemetry/prometheus_mp/mp_store.cpp | 13 +++---------- src/plugins/telemetry/prometheus_mp/mp_store.h | 3 ++- test/gtest/telemetry_mp_collector_test.cpp | 16 ++++++---------- 4 files changed, 16 insertions(+), 32 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index 2859037874..06b3780400 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -17,6 +17,7 @@ #include "mp_collector.h" #include "common/nixl_log.h" +#include "common/nixl_time.h" #include "telemetry_event.h" #include @@ -40,13 +41,6 @@ namespace { using prometheus::MetricFamily; using prometheus::MetricType; - [[nodiscard]] uint64_t - nowNs() noexcept { - return static_cast(std::chrono::duration_cast( - std::chrono::system_clock::now().time_since_epoch()) - .count()); - } - [[nodiscard]] std::vector baseLabels(const mpStoreSnapshot &s) { std::vector labels; @@ -78,12 +72,12 @@ namespace { return m; } - // Minimum age before an unparseable file is reaped, even when the TTL is 0. + // Minimum age before an unparsable file is reaped, even when the TTL is 0. // Protects a store a live process is actively creating (a sub-second window) // from being deleted out from under it. constexpr long kInvalidFileFloorSeconds = 2; - // Whether an unparseable store file (bad/zero magic, wrong schema, truncated) + // Whether an unparsable store file (bad/zero magic, wrong schema, truncated) // is old enough to be an orphan worth removing, rather than a live process's // store mid-creation. [[nodiscard]] bool @@ -135,7 +129,7 @@ isSnapshotLive(const mpStoreSnapshot &snap, std::chrono::nanoseconds ttl) { if (isProcessAlive(snap.pid, snap.startTime)) { return true; } - const uint64_t now = nowNs(); + const uint64_t now = nixlTime::getNs(); const auto ttl_ns = static_cast(ttl.count() < 0 ? 0 : ttl.count()); return now >= snap.lastUpdateNs && (now - snap.lastUpdateNs) <= ttl_ns; } @@ -221,7 +215,7 @@ nixlMultiprocessCollector::Collect() const { } auto snap = readStoreSnapshot(entry.path()); if (!snap) { - // Unparseable (mid-init, orphaned, or incompatible): reap only if it is + // Unparsable (mid-init, orphaned, or incompatible): reap only if it is // old enough to not be a store a live process is currently creating. if (reapStale_ && invalidFileReapable(entry.path(), staleTtl_)) { std::error_code rm_ec; diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 98c8bca538..e7f6bec14c 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -17,6 +17,7 @@ #include "mp_store.h" #include "common/nixl_log.h" +#include "common/nixl_time.h" #include #include @@ -24,7 +25,6 @@ #include #include -#include #include #include #include @@ -60,13 +60,6 @@ namespace { uint64_t gauges[MP_STORE_SLOT_COUNT]; }; - [[nodiscard]] uint64_t - nowNs() noexcept { - return static_cast(std::chrono::duration_cast( - std::chrono::system_clock::now().time_since_epoch()) - .count()); - } - void copyField(char *dst, std::size_t cap, const std::string &src, const char *what) { if (src.size() >= cap) { @@ -160,7 +153,7 @@ mpStoreWriter::mpStoreWriter(std::filesystem::path path, copyField(layout->agentName, MP_MAX_AGENT_NAME, agent_name, "agent name"); copyField(layout->hostname, MP_MAX_HOSTNAME, hostname, "hostname"); copyField(layout->dpRank, MP_MAX_DP_RANK, dp_rank, "dp_rank"); - __atomic_store_n(&layout->lastUpdateNs, nowNs(), __ATOMIC_RELEASE); + __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELEASE); // Publish the magic last so a concurrent reader never validates a // half-initialized header. __atomic_store_n(&layout->magic, MP_STORE_MAGIC, __ATOMIC_RELEASE); @@ -176,7 +169,7 @@ mpStoreWriter::~mpStoreWriter() { void mpStoreWriter::touch() noexcept { auto *layout = static_cast(mapping_); - __atomic_store_n(&layout->lastUpdateNs, nowNs(), __ATOMIC_RELEASE); + __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELEASE); } void diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 9754a4f0bb..a018b57b70 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -58,7 +58,8 @@ struct mpStoreSnapshot { // Process start time in clock ticks (/proc//stat field 22); pairs with // pid to survive PID reuse when checking liveness. uint64_t startTime = 0; - // Wall-clock nanoseconds of the last writer update; used for TTL staleness. + // Monotonic nanoseconds (nixlTime::getNs, CLOCK_MONOTONIC -- host-wide, so + // comparable across processes) of the last writer update; used for TTL staleness. uint64_t lastUpdateNs = 0; std::string agentName; std::string hostname; diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp index f38e20b172..642d9f2c04 100644 --- a/test/gtest/telemetry_mp_collector_test.cpp +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -17,6 +17,8 @@ #include "mp_collector.h" #include "mp_store.h" +#include "common/nixl_time.h" + #include #include @@ -51,19 +53,12 @@ idx(nixl_telemetry_event_type_t t) { return static_cast(t); } -[[nodiscard]] uint64_t -nowNs() { - return static_cast(std::chrono::duration_cast( - std::chrono::system_clock::now().time_since_epoch()) - .count()); -} - [[nodiscard]] mpStoreSnapshot makeSnap(const std::string &agent, const std::string &rank) { mpStoreSnapshot s; s.pid = ::getpid(); s.startTime = 1; - s.lastUpdateNs = nowNs(); + s.lastUpdateNs = nixlTime::getNs(); s.agentName = agent; s.hostname = "host"; s.dpRank = rank; @@ -209,12 +204,13 @@ TEST(MpCollectorTest, SnapshotLivenessByProcessThenTtl) { auto dead_fresh = makeSnap("b", ""); dead_fresh.pid = 0x7fffffff; - dead_fresh.lastUpdateNs = nowNs(); + dead_fresh.lastUpdateNs = nixlTime::getNs(); EXPECT_TRUE(isSnapshotLive(dead_fresh, ttl)); auto dead_stale = makeSnap("c", ""); dead_stale.pid = 0x7fffffff; - dead_stale.lastUpdateNs = nowNs() - std::chrono::nanoseconds(std::chrono::seconds(60)).count(); + dead_stale.lastUpdateNs = + nixlTime::getNs() - std::chrono::nanoseconds(std::chrono::seconds(60)).count(); EXPECT_FALSE(isSnapshotLive(dead_stale, ttl)); } From 3bf21390afb382701375ad624fa21187cadb7b15 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Thu, 9 Jul 2026 21:16:36 +0000 Subject: [PATCH 11/71] telemetry: address CodeRabbit review (mp bounds guard, dir iteration, includes) - mp_store.h: add a compile-time static_assert that MP_STORE_SLOT_COUNT covers every telemetry event type the collector indexes, so extending the enum past AGENT_TELEMETRY_EVENTS_DROPPED fails the build instead of causing an out-of-bounds counter/gauge access. - mp_collector.cpp: iterate the shared directory with the non-throwing increment(ec) instead of the range-for's throwing operator++, since peer writers and this collector's own reaping mutate the directory concurrently and a mid-iteration filesystem error would otherwise escape Collect(). - mp_store.cpp: add explicit and includes (std::min, std::istream[buf]_iterator) rather than relying on transitive includes. Part of NIX-1614. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/mp_collector.cpp | 10 +++++++- .../telemetry/prometheus_mp/mp_store.cpp | 2 ++ .../telemetry/prometheus_mp/mp_store.h | 25 +++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index 06b3780400..7b1b9b8545 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -209,7 +209,15 @@ nixlMultiprocessCollector::Collect() const { return {}; } - for (const auto &entry : it) { + // Iterate with the non-throwing increment: peer writers and this collector's + // own reaping mutate the directory concurrently, and the range-for's + // operator++ would throw on a mid-iteration filesystem error. + for (const std::filesystem::directory_iterator end; it != end; it.increment(ec)) { + if (ec) { + NIXL_DEBUG << "prometheus_mp: telemetry dir iteration stopped early: " << ec.message(); + break; + } + const auto &entry = *it; if (!entry.is_regular_file(ec) || !nameMatchesStore(entry.path().filename().string())) { continue; } diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index e7f6bec14c..e4ac7a7984 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -24,9 +24,11 @@ #include #include +#include #include #include #include +#include #include #include #include diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index a018b57b70..3bbe79be2e 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -19,6 +19,7 @@ #include "telemetry_event.h" +#include #include #include #include @@ -46,6 +47,30 @@ inline constexpr std::string_view MP_STORE_FILE_SUFFIX = ".mmap"; inline constexpr std::size_t MP_STORE_SLOT_COUNT = static_cast(nixl_telemetry_event_type_t::AGENT_TELEMETRY_EVENTS_DROPPED) + 1; +namespace detail { + // Highest slot index the collector will index into the counter/gauge arrays, + // across every event type it publishes. + [[nodiscard]] constexpr std::size_t + maxTelemetrySlot() { + std::size_t max_slot = 0; + for (const auto type : telemetry_metric_event_types) { + max_slot = std::max(max_slot, static_cast(type)); + } + for (const auto type : telemetry_error_event_types) { + max_slot = std::max(max_slot, static_cast(type)); + } + return max_slot; + } +} // namespace detail + +// Compile-time guard: if the enum is extended past AGENT_TELEMETRY_EVENTS_DROPPED +// (so it is no longer last), the fixed-slot store would be indexed out of bounds +// by the collector. Fail the build instead, forcing MP_STORE_SLOT_COUNT to be +// updated. +static_assert(detail::maxTelemetrySlot() < MP_STORE_SLOT_COUNT, + "MP_STORE_SLOT_COUNT must cover every telemetry event type the collector indexes; " + "keep AGENT_TELEMETRY_EVENTS_DROPPED last or update MP_STORE_SLOT_COUNT"); + /** * @brief A point-in-time copy of one process's metric-state store file. * From cd22887d692955847ba55f2589dace32fc595380 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Fri, 10 Jul 2026 01:37:30 +0000 Subject: [PATCH 12/71] telemetry: rename mp rank label dp_rank -> local_rank The optional per-process rank label was named dp_rank, but its value is sourced from the local/per-GPU (tensor-parallel) rank env (default LOCAL_RANK), not the data-parallel rank. That misdescribed the value and collided with Dynamo's own data-parallel dp_rank series. Rename the emitted label and the internal identifiers (store field, writer param, exporter helper) to local_rank. The NIXL_TELEMETRY_RANK_ENV var and its LOCAL_RANK default are unchanged; the label is still optional and emitted only when the env is set. Docs and tests updated. Part of NIX-1614. Signed-off-by: Efraim Eygin --- docs/telemetry.md | 5 +++-- src/plugins/telemetry/prometheus_mp/README.md | 11 ++++++----- src/plugins/telemetry/prometheus_mp/mp_collector.cpp | 4 ++-- src/plugins/telemetry/prometheus_mp/mp_collector.h | 2 +- src/plugins/telemetry/prometheus_mp/mp_store.cpp | 10 +++++----- src/plugins/telemetry/prometheus_mp/mp_store.h | 8 ++++---- .../prometheus_mp/prometheus_mp_exporter.cpp | 11 ++++++----- test/gtest/telemetry_mp_collector_test.cpp | 10 +++++----- test/gtest/telemetry_mp_store_test.cpp | 4 ++-- 9 files changed, 34 insertions(+), 31 deletions(-) diff --git a/docs/telemetry.md b/docs/telemetry.md index 39e43b00df..4b462ab725 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -154,11 +154,12 @@ bind collision is benign, so no rank is dropped. Full details: | `NIXL_TELEMETRY_MULTIPROC_DIR` | Shared directory for per-process store files (all ranks to be aggregated must use the same path). **Required.** | - | | `NIXL_TELEMETRY_PROMETHEUS_PORT` | Scrape port (shared with the `prometheus` exporter) | `9090` | | `NIXL_TELEMETRY_PROMETHEUS_LOCAL` | Bind `127.0.0.1` instead of `0.0.0.0` | `false` | -| `NIXL_TELEMETRY_RANK_ENV` | Name of the env var holding the rank for the optional `dp_rank` label; no label if that env var is unset | `LOCAL_RANK` | +| `NIXL_TELEMETRY_RANK_ENV` | Name of the env var holding the rank for the optional `local_rank` label; no label if that env var is unset | `LOCAL_RANK` | | `NIXL_TELEMETRY_MP_STALE_TTL` | Seconds after which a dead process's store is stale and reaped | `30` | Series are labeled by `hostname`, `agent_name`, `pid` (guarantees uniqueness), and -optionally `dp_rank`. Metric names, types, and semantics are identical to the +optionally `local_rank` (the local/per-GPU rank, not Dynamo's data-parallel +`dp_rank`). Metric names, types, and semantics are identical to the single-process `prometheus` exporter. `NIXL_TELEMETRY_MULTIPROC_DIR` follows Dynamo's `PROMETHEUS_MULTIPROC_DIR` diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index ae81667381..85787f13b8 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -84,8 +84,8 @@ per-process store files are ~one page each. export NIXL_TELEMETRY_PROMETHEUS_PORT="" export NIXL_TELEMETRY_PROMETHEUS_LOCAL="y" # bind 127.0.0.1 instead of 0.0.0.0 -# Optional dp_rank label: names the env var that holds the rank (default LOCAL_RANK). -# If that env var is unset, no dp_rank label is emitted (series stay unique via pid). +# Optional local_rank label: names the env var that holds the rank (default LOCAL_RANK). +# If that env var is unset, no local_rank label is emitted (series stay unique via pid). export NIXL_TELEMETRY_RANK_ENV="LOCAL_RANK" # Seconds after which a dead process's store is considered stale and reaped (default 30). @@ -101,8 +101,9 @@ Every series is labeled by: - `pid` -- the producing process id. This guarantees each process is a distinct series even if agent names collide; it is deliberately **not** named `instance` (a reserved Prometheus target label). -- `dp_rank` -- **optional**, present only when a rank env var (see - `NIXL_TELEMETRY_RANK_ENV`) is set. +- `local_rank` -- **optional**, present only when a rank env var (see + `NIXL_TELEMETRY_RANK_ENV`) is set. This is the local/per-GPU (TP) rank, distinct + from Dynamo's data-parallel `dp_rank`. - `status` -- only on `agent_errors_total`, bounded by the fixed `AGENT_ERR_*` set. The metric names, types, counter/gauge semantics, and events are identical to the @@ -117,7 +118,7 @@ not reuse, Python `prometheus_client`'s multiprocess format): - The metric set is fixed at compile time; slots are positional, so metric names are never stored in the files. -- Per-process label values (`hostname`, `agent_name`, `pid`, `dp_rank`) are +- Per-process label values (`hostname`, `agent_name`, `pid`, `local_rank`) are captured once at startup and never change. Events carry only a numeric value -- there are **no per-observation labels**. - Consequently the store **cannot represent a metric with a dynamic / diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index 7b1b9b8545..b973ded63f 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -50,8 +50,8 @@ namespace { // not unique across processes; avoids duplicate-series scrape errors. Not // named "instance" (a reserved Prometheus target label). labels.push_back({"pid", std::to_string(s.pid)}); - if (!s.dpRank.empty()) { - labels.push_back({"dp_rank", s.dpRank}); + if (!s.localRank.empty()) { + labels.push_back({"local_rank", s.localRank}); } return labels; } diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.h b/src/plugins/telemetry/prometheus_mp/mp_collector.h index a16245805f..b58e3b7859 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.h +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.h @@ -58,7 +58,7 @@ isSnapshotLive(const mpStoreSnapshot &snap, std::chrono::nanoseconds ttl); * Emits one series per (metric, process): cumulative counters and last-operation * gauges keyed by nixl_telemetry_event_type_t, plus the agent_errors_total family * with a status label. Series are labeled by hostname, agent_name and (when - * present) dp_rank. No cross-process aggregation -- each process is its own + * present) local_rank. No cross-process aggregation -- each process is its own * series. Returns empty when @p snapshots is empty. * @param snapshots Live per-process snapshots. */ diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index e4ac7a7984..6796e9c8f2 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -43,7 +43,7 @@ namespace { constexpr std::size_t MP_MAX_AGENT_NAME = 256; constexpr std::size_t MP_MAX_HOSTNAME = 128; - constexpr std::size_t MP_MAX_DP_RANK = 64; + constexpr std::size_t MP_MAX_LOCAL_RANK = 64; // Fixed on-disk layout. Plain trivially-copyable POD operated on with __atomic // builtins (not std::atomic) so it is safe to memset/reinterpret over an mmap'd @@ -57,7 +57,7 @@ namespace { uint64_t lastUpdateNs; char agentName[MP_MAX_AGENT_NAME]; char hostname[MP_MAX_HOSTNAME]; - char dpRank[MP_MAX_DP_RANK]; + char localRank[MP_MAX_LOCAL_RANK]; uint64_t counters[MP_STORE_SLOT_COUNT]; uint64_t gauges[MP_STORE_SLOT_COUNT]; }; @@ -122,7 +122,7 @@ readProcessStartTime(int64_t pid) { mpStoreWriter::mpStoreWriter(std::filesystem::path path, const std::string &agent_name, const std::string &hostname, - const std::string &dp_rank) + const std::string &local_rank) : path_(std::move(path)), mappingSize_(sizeof(mpStoreLayout)) { const int fd = ::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC, 0644); @@ -154,7 +154,7 @@ mpStoreWriter::mpStoreWriter(std::filesystem::path path, layout->startTime = readProcessStartTime(layout->pid); copyField(layout->agentName, MP_MAX_AGENT_NAME, agent_name, "agent name"); copyField(layout->hostname, MP_MAX_HOSTNAME, hostname, "hostname"); - copyField(layout->dpRank, MP_MAX_DP_RANK, dp_rank, "dp_rank"); + copyField(layout->localRank, MP_MAX_LOCAL_RANK, local_rank, "local_rank"); __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELEASE); // Publish the magic last so a concurrent reader never validates a // half-initialized header. @@ -255,7 +255,7 @@ readStoreSnapshot(const std::filesystem::path &path) { snap.lastUpdateNs = __atomic_load_n(&layout->lastUpdateNs, __ATOMIC_ACQUIRE); snap.agentName = readField(layout->agentName, MP_MAX_AGENT_NAME); snap.hostname = readField(layout->hostname, MP_MAX_HOSTNAME); - snap.dpRank = readField(layout->dpRank, MP_MAX_DP_RANK); + snap.localRank = readField(layout->localRank, MP_MAX_LOCAL_RANK); for (std::size_t i = 0; i < MP_STORE_SLOT_COUNT; ++i) { snap.counters[i] = __atomic_load_n(&layout->counters[i], __ATOMIC_RELAXED); snap.gauges[i] = __atomic_load_n(&layout->gauges[i], __ATOMIC_RELAXED); diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 3bbe79be2e..929f217a5e 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -88,8 +88,8 @@ struct mpStoreSnapshot { uint64_t lastUpdateNs = 0; std::string agentName; std::string hostname; - // Optional Dynamo-style rank label; empty when no rank env was set. - std::string dpRank; + // Optional local (per-GPU/TP) rank label; empty when no rank env was set. + std::string localRank; std::array counters{}; std::array gauges{}; }; @@ -110,13 +110,13 @@ class mpStoreWriter { * @param path Full path to this process's store file. * @param agent_name Per-process agent name (unique; drives the series label). * @param hostname Host name label. - * @param dp_rank Optional rank label; pass empty to omit it. + * @param local_rank Optional rank label; pass empty to omit it. * @throws std::runtime_error on open/ftruncate/mmap failure. */ mpStoreWriter(std::filesystem::path path, const std::string &agent_name, const std::string &hostname, - const std::string &dp_rank); + const std::string &local_rank); ~mpStoreWriter(); mpStoreWriter(const mpStoreWriter &) = delete; diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp index 20f6a8db42..90273d4e69 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -63,11 +63,12 @@ getHostname() { return "unknown"; } -// Resolves the optional dp_rank label value: NIXL_TELEMETRY_RANK_ENV names which -// env var holds the rank (default LOCAL_RANK); the value of that env var is the -// rank. Empty when the named env var is unset -- rank is a best-effort label only. +// Resolves the optional local_rank label value: NIXL_TELEMETRY_RANK_ENV names +// which env var holds the rank (default LOCAL_RANK); the value of that env var is +// the rank. Empty when the named env var is unset -- rank is a best-effort label +// only. This is the local/per-GPU (TP) rank, distinct from Dynamo's dp_rank. [[nodiscard]] std::string -resolveDpRank() { +resolveLocalRank() { const std::string rank_source = nixl::config::getValueDefaulted(rankEnvVar, defaultRankEnvName); if (rank_source.empty()) { @@ -118,7 +119,7 @@ nixlTelemetryPrometheusMpExporter::nixlTelemetryPrometheusMpExporter( const std::filesystem::path store_path = dir / makeStoreFileName(pid, start_time, instance); store_ = std::make_unique( - store_path, init_params.agentName, getHostname(), resolveDpRank()); + store_path, init_params.agentName, getHostname(), resolveLocalRank()); const bool local = nixl::config::getValueDefaulted(prometheusLocalVar, false); const uint16_t port = nixl::config::getValueDefaulted(prometheusPortVar, defaultPort); diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp index 642d9f2c04..aaa8726237 100644 --- a/test/gtest/telemetry_mp_collector_test.cpp +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -61,7 +61,7 @@ makeSnap(const std::string &agent, const std::string &rank) { s.lastUpdateNs = nixlTime::getNs(); s.agentName = agent; s.hostname = "host"; - s.dpRank = rank; + s.localRank = rank; return s; } @@ -131,7 +131,7 @@ TEST(MpCollectorTest, PerProcessCountersAndGauges) { } TEST(MpCollectorTest, PidLabelDisambiguatesSameAgentName) { - // Two processes that (mis)use the same agent name and no dp_rank must still + // Two processes that (mis)use the same agent name and no local_rank must still // produce distinct series, keyed by pid, rather than a duplicate series. auto a = makeSnap("dup", ""); a.pid = 1001; @@ -153,7 +153,7 @@ TEST(MpCollectorTest, PidLabelDisambiguatesSameAgentName) { EXPECT_TRUE(hasLabel(*m_a, "pid")); } -TEST(MpCollectorTest, DpRankLabelOnlyWhenPresent) { +TEST(MpCollectorTest, LocalRankLabelOnlyWhenPresent) { const auto with_rank = makeSnap("agent-a", "3"); const auto without_rank = makeSnap("agent-b", ""); @@ -165,8 +165,8 @@ TEST(MpCollectorTest, DpRankLabelOnlyWhenPresent) { const auto *m_without = findByLabel(*tx, "agent_name", "agent-b"); ASSERT_NE(m_with, nullptr); ASSERT_NE(m_without, nullptr); - EXPECT_TRUE(hasLabel(*m_with, "dp_rank")); - EXPECT_FALSE(hasLabel(*m_without, "dp_rank")); + EXPECT_TRUE(hasLabel(*m_with, "local_rank")); + EXPECT_FALSE(hasLabel(*m_without, "local_rank")); } TEST(MpCollectorTest, ErrorFamilyCarriesStatusLabel) { diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index 2c9fcab584..fa19e74eed 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -79,7 +79,7 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { ASSERT_TRUE(snap.has_value()); EXPECT_EQ(snap->agentName, "agent-a"); EXPECT_EQ(snap->hostname, "host-1"); - EXPECT_EQ(snap->dpRank, "3"); + EXPECT_EQ(snap->localRank, "3"); EXPECT_EQ(snap->pid, static_cast(::getpid())); EXPECT_GT(snap->startTime, 0u); EXPECT_GT(snap->lastUpdateNs, 0u); @@ -114,7 +114,7 @@ TEST_F(MpStoreTest, EmptyRankIsEmpty) { const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); - EXPECT_TRUE(snap->dpRank.empty()); + EXPECT_TRUE(snap->localRank.empty()); } TEST_F(MpStoreTest, LongAgentNameTruncated) { From 4e69307fb6f012a0b3a2de1d16221b0980367609 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Fri, 10 Jul 2026 15:31:14 +0000 Subject: [PATCH 13/71] telemetry: remove mp store file on clean writer destruction The prometheus_mp store writer only unmapped its file on destruction, so an agent destroyed while its process keeps running left a store behind. Because liveness is pid-based, the collector kept publishing that dead agent's frozen series indefinitely. Have ~mpStoreWriter() best-effort remove its own file: clean shutdown is the deterministic "producer gone" signal, while crash/kill (destructor never runs) still falls back to the owner's liveness/TTL reaping. Adjust the store round-trip tests to read while the writer is alive (the real cross-process pattern) and add a test for the new cleanup. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/prometheus_mp/README.md | 8 ++-- .../telemetry/prometheus_mp/mp_store.cpp | 3 ++ test/gtest/telemetry_mp_store_test.cpp | 37 +++++++++++-------- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index 85787f13b8..4de58a7676 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -46,10 +46,10 @@ Same as the `prometheus` plug-in: the bundled prometheus-cpp subproject and - **Per-process series.** Each process is exported as its own series (cumulative counters and last-operation gauges), never summed across processes, so per-process values stay correct and monotonic. -- **Stale handling.** When a process exits, the owner drops its series once the - process is gone (verified by pid + `/proc` start time) or its data ages past the - TTL, and reaps the file. Cleanup is performed by the owner, since a killed - process cannot clean up after itself. +- **Stale handling.** On clean shutdown a process removes its own store file. If + it instead crashes or is killed -- and so cannot clean up after itself -- the + owner drops its series once the process is gone (verified by pid + `/proc` start + time) or its data ages past the TTL, and reaps the file. ## Configuration diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 6796e9c8f2..9ce317b10c 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include namespace nixl::telemetry::mp { @@ -166,6 +167,8 @@ mpStoreWriter::~mpStoreWriter() { ::munmap(mapping_, mappingSize_); mapping_ = nullptr; } + std::error_code ec; + std::filesystem::remove(path_, ec); } void diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index fa19e74eed..15a1297937 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -68,12 +68,10 @@ class MpStoreTest : public ::testing::Test { TEST_F(MpStoreTest, WriteReadRoundTrip) { const auto path = storePath("agent-a"); - { - mpStoreWriter writer(path, "agent-a", "host-1", "3"); - writer.addCounter(TX_BYTES, 1000); - writer.setGauge(TX_BYTES, 1000); - writer.addCounter(ERR_BACKEND, 1); - } + mpStoreWriter writer(path, "agent-a", "host-1", "3"); + writer.addCounter(TX_BYTES, 1000); + writer.setGauge(TX_BYTES, 1000); + writer.addCounter(ERR_BACKEND, 1); const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); @@ -93,14 +91,12 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { const auto path = storePath("agent-b"); - { - mpStoreWriter writer(path, "agent-b", "host-1", ""); - writer.addCounter(TX_BYTES, 100); - writer.addCounter(TX_BYTES, 250); - writer.addCounter(TX_BYTES, 650); - writer.setGauge(TX_BYTES, 100); - writer.setGauge(TX_BYTES, 650); // last write wins - } + mpStoreWriter writer(path, "agent-b", "host-1", ""); + writer.addCounter(TX_BYTES, 100); + writer.addCounter(TX_BYTES, 250); + writer.addCounter(TX_BYTES, 650); + writer.setGauge(TX_BYTES, 100); + writer.setGauge(TX_BYTES, 650); // last write wins const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); @@ -110,18 +106,27 @@ TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { TEST_F(MpStoreTest, EmptyRankIsEmpty) { const auto path = storePath("agent-c"); - { mpStoreWriter writer(path, "agent-c", "host-1", ""); } + mpStoreWriter writer(path, "agent-c", "host-1", ""); const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); EXPECT_TRUE(snap->localRank.empty()); } +TEST_F(MpStoreTest, DestructorRemovesStoreFile) { + const auto path = storePath("agent-cleanup"); + { + mpStoreWriter writer(path, "agent-cleanup", "host-1", ""); + EXPECT_TRUE(std::filesystem::exists(path)); + } + EXPECT_FALSE(std::filesystem::exists(path)); +} + TEST_F(MpStoreTest, LongAgentNameTruncated) { const auto path = storePath("agent-long"); const std::string long_name(1000, 'x'); const gtest::LogIgnoreGuard lig("exceeds 255 chars"); - { mpStoreWriter writer(path, long_name, "host-1", ""); } + mpStoreWriter writer(path, long_name, "host-1", ""); const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); From f5de93f403034e775ddc74c2d553dd0a0e295f59 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Fri, 10 Jul 2026 16:26:37 +0000 Subject: [PATCH 14/71] telemetry: add agent_instance label so same-name mp agents stay distinct Multiple agents created in one process with the same agent name shared every series label (hostname, agent_name, pid, local_rank), producing duplicate Prometheus series that make the whole scrape fail. The store filename already carried a per-process instance counter, but it never reached the exposed labels. Persist the instance in the store header and emit it as an agent_instance label, so same-name same-process agents get distinct series (agent_instance=0 for the common single-agent case). Schema version is unchanged: prometheus_mp has not shipped, so no on-disk format compatibility is at stake. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/prometheus_mp/README.md | 3 ++ .../telemetry/prometheus_mp/mp_collector.cpp | 1 + .../telemetry/prometheus_mp/mp_store.cpp | 6 +++- .../telemetry/prometheus_mp/mp_store.h | 8 +++++- .../prometheus_mp/prometheus_mp_exporter.cpp | 2 +- test/gtest/telemetry_mp_collector_test.cpp | 28 +++++++++++++++++-- test/gtest/telemetry_mp_store_test.cpp | 11 ++++---- 7 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index 4de58a7676..474e8d6b0a 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -101,6 +101,9 @@ Every series is labeled by: - `pid` -- the producing process id. This guarantees each process is a distinct series even if agent names collide; it is deliberately **not** named `instance` (a reserved Prometheus target label). +- `agent_instance` -- a per-process counter distinguishing multiple agents created + in the same process (which share `pid`, `hostname`, and `agent_name`), so their + series never collide. `0` for the common single-agent-per-process case. - `local_rank` -- **optional**, present only when a rank env var (see `NIXL_TELEMETRY_RANK_ENV`) is set. This is the local/per-GPU (TP) rank, distinct from Dynamo's data-parallel `dp_rank`. diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index b973ded63f..d7c73aab3d 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -50,6 +50,7 @@ namespace { // not unique across processes; avoids duplicate-series scrape errors. Not // named "instance" (a reserved Prometheus target label). labels.push_back({"pid", std::to_string(s.pid)}); + labels.push_back({"agent_instance", std::to_string(s.instance)}); if (!s.localRank.empty()) { labels.push_back({"local_rank", s.localRank}); } diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 9ce317b10c..ac9edda590 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -56,6 +56,7 @@ namespace { int64_t pid; uint64_t startTime; uint64_t lastUpdateNs; + uint64_t instance; char agentName[MP_MAX_AGENT_NAME]; char hostname[MP_MAX_HOSTNAME]; char localRank[MP_MAX_LOCAL_RANK]; @@ -123,7 +124,8 @@ readProcessStartTime(int64_t pid) { mpStoreWriter::mpStoreWriter(std::filesystem::path path, const std::string &agent_name, const std::string &hostname, - const std::string &local_rank) + const std::string &local_rank, + uint64_t instance) : path_(std::move(path)), mappingSize_(sizeof(mpStoreLayout)) { const int fd = ::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC, 0644); @@ -153,6 +155,7 @@ mpStoreWriter::mpStoreWriter(std::filesystem::path path, layout->slotCount = static_cast(MP_STORE_SLOT_COUNT); layout->pid = static_cast(::getpid()); layout->startTime = readProcessStartTime(layout->pid); + layout->instance = instance; copyField(layout->agentName, MP_MAX_AGENT_NAME, agent_name, "agent name"); copyField(layout->hostname, MP_MAX_HOSTNAME, hostname, "hostname"); copyField(layout->localRank, MP_MAX_LOCAL_RANK, local_rank, "local_rank"); @@ -255,6 +258,7 @@ readStoreSnapshot(const std::filesystem::path &path) { mpStoreSnapshot snap; snap.pid = layout->pid; snap.startTime = layout->startTime; + snap.instance = layout->instance; snap.lastUpdateNs = __atomic_load_n(&layout->lastUpdateNs, __ATOMIC_ACQUIRE); snap.agentName = readField(layout->agentName, MP_MAX_AGENT_NAME); snap.hostname = readField(layout->hostname, MP_MAX_HOSTNAME); diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 929f217a5e..6c88a1580e 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -86,6 +86,9 @@ struct mpStoreSnapshot { // Monotonic nanoseconds (nixlTime::getNs, CLOCK_MONOTONIC -- host-wide, so // comparable across processes) of the last writer update; used for TTL staleness. uint64_t lastUpdateNs = 0; + // Per-process instance counter, distinguishing multiple agents that share the + // same name (and thus pid/hostname) within one process so their series differ. + uint64_t instance = 0; std::string agentName; std::string hostname; // Optional local (per-GPU/TP) rank label; empty when no rank env was set. @@ -111,12 +114,15 @@ class mpStoreWriter { * @param agent_name Per-process agent name (unique; drives the series label). * @param hostname Host name label. * @param local_rank Optional rank label; pass empty to omit it. + * @param instance Per-process instance counter; disambiguates multiple + * same-named agents in one process so their series stay distinct. * @throws std::runtime_error on open/ftruncate/mmap failure. */ mpStoreWriter(std::filesystem::path path, const std::string &agent_name, const std::string &hostname, - const std::string &local_rank); + const std::string &local_rank, + uint64_t instance); ~mpStoreWriter(); mpStoreWriter(const mpStoreWriter &) = delete; diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp index 90273d4e69..e109e13507 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -119,7 +119,7 @@ nixlTelemetryPrometheusMpExporter::nixlTelemetryPrometheusMpExporter( const std::filesystem::path store_path = dir / makeStoreFileName(pid, start_time, instance); store_ = std::make_unique( - store_path, init_params.agentName, getHostname(), resolveLocalRank()); + store_path, init_params.agentName, getHostname(), resolveLocalRank(), instance); const bool local = nixl::config::getValueDefaulted(prometheusLocalVar, false); const uint16_t port = nixl::config::getValueDefaulted(prometheusPortVar, defaultPort); diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp index aaa8726237..108c170750 100644 --- a/test/gtest/telemetry_mp_collector_test.cpp +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -153,6 +153,30 @@ TEST(MpCollectorTest, PidLabelDisambiguatesSameAgentName) { EXPECT_TRUE(hasLabel(*m_a, "pid")); } +TEST(MpCollectorTest, AgentInstanceLabelDisambiguatesSameProcessSameName) { + // Two agents in the SAME process (same pid) with the same name must still + // produce distinct series, keyed by agent_instance, rather than colliding. + auto a = makeSnap("dup", ""); + a.pid = 1001; + a.instance = 0; + a.counters[idx(TX_BYTES)] = 10; + auto b = makeSnap("dup", ""); + b.pid = 1001; + b.instance = 1; + b.counters[idx(TX_BYTES)] = 20; + + const auto fams = buildMetricFamilies({a, b}); + const auto *tx = findFamily(fams, "agent_tx_bytes_total"); + ASSERT_NE(tx, nullptr); + ASSERT_EQ(tx->metric.size(), 2u); + const auto *m_a = findByLabel(*tx, "agent_instance", "0"); + const auto *m_b = findByLabel(*tx, "agent_instance", "1"); + ASSERT_NE(m_a, nullptr); + ASSERT_NE(m_b, nullptr); + EXPECT_DOUBLE_EQ(m_a->counter.value, 10.0); + EXPECT_DOUBLE_EQ(m_b->counter.value, 20.0); +} + TEST(MpCollectorTest, LocalRankLabelOnlyWhenPresent) { const auto with_rank = makeSnap("agent-a", "3"); const auto without_rank = makeSnap("agent-b", ""); @@ -235,10 +259,10 @@ class MpCollectorFileTest : public ::testing::Test { TEST_F(MpCollectorFileTest, CollectReadsLiveStoresAndIgnoresOthers) { // Two distinct store files; both headers stamp this (live) process. - mpStoreWriter w1(dir_ / makeStoreFileName(111, 1, 0), "agent-1", "host", "0"); + mpStoreWriter w1(dir_ / makeStoreFileName(111, 1, 0), "agent-1", "host", "0", 0); w1.addCounter(TX_BYTES, 500); w1.setGauge(TX_BYTES, 500); - mpStoreWriter w2(dir_ / makeStoreFileName(222, 2, 0), "agent-2", "host", "1"); + mpStoreWriter w2(dir_ / makeStoreFileName(222, 2, 0), "agent-2", "host", "1", 0); w2.addCounter(TX_BYTES, 700); // A non-store file must be ignored. diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index 15a1297937..a798b5a85f 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -68,7 +68,7 @@ class MpStoreTest : public ::testing::Test { TEST_F(MpStoreTest, WriteReadRoundTrip) { const auto path = storePath("agent-a"); - mpStoreWriter writer(path, "agent-a", "host-1", "3"); + mpStoreWriter writer(path, "agent-a", "host-1", "3", 7); writer.addCounter(TX_BYTES, 1000); writer.setGauge(TX_BYTES, 1000); writer.addCounter(ERR_BACKEND, 1); @@ -78,6 +78,7 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { EXPECT_EQ(snap->agentName, "agent-a"); EXPECT_EQ(snap->hostname, "host-1"); EXPECT_EQ(snap->localRank, "3"); + EXPECT_EQ(snap->instance, 7u); EXPECT_EQ(snap->pid, static_cast(::getpid())); EXPECT_GT(snap->startTime, 0u); EXPECT_GT(snap->lastUpdateNs, 0u); @@ -91,7 +92,7 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { const auto path = storePath("agent-b"); - mpStoreWriter writer(path, "agent-b", "host-1", ""); + mpStoreWriter writer(path, "agent-b", "host-1", "", 0); writer.addCounter(TX_BYTES, 100); writer.addCounter(TX_BYTES, 250); writer.addCounter(TX_BYTES, 650); @@ -106,7 +107,7 @@ TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { TEST_F(MpStoreTest, EmptyRankIsEmpty) { const auto path = storePath("agent-c"); - mpStoreWriter writer(path, "agent-c", "host-1", ""); + mpStoreWriter writer(path, "agent-c", "host-1", "", 0); const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); @@ -116,7 +117,7 @@ TEST_F(MpStoreTest, EmptyRankIsEmpty) { TEST_F(MpStoreTest, DestructorRemovesStoreFile) { const auto path = storePath("agent-cleanup"); { - mpStoreWriter writer(path, "agent-cleanup", "host-1", ""); + mpStoreWriter writer(path, "agent-cleanup", "host-1", "", 0); EXPECT_TRUE(std::filesystem::exists(path)); } EXPECT_FALSE(std::filesystem::exists(path)); @@ -126,7 +127,7 @@ TEST_F(MpStoreTest, LongAgentNameTruncated) { const auto path = storePath("agent-long"); const std::string long_name(1000, 'x'); const gtest::LogIgnoreGuard lig("exceeds 255 chars"); - mpStoreWriter writer(path, long_name, "host-1", ""); + mpStoreWriter writer(path, long_name, "host-1", "", 0); const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); From 4c7483474d62668c8607fa0a70d2fea6bf8bbb4f Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Fri, 10 Jul 2026 16:29:48 +0000 Subject: [PATCH 15/71] docs: clarify mp stale TTL is from last update and document agent_instance NIXL_TELEMETRY_MP_STALE_TTL is measured from a dead process's last update, not from the moment of death; a live process is always published regardless of age. Reword both docs to say so. Also add the agent_instance label to the telemetry.md label list to match the prometheus_mp exporter. Signed-off-by: Efraim Eygin --- docs/telemetry.md | 9 +++++---- src/plugins/telemetry/prometheus_mp/README.md | 3 ++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/telemetry.md b/docs/telemetry.md index 4b462ab725..07d82777da 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -155,11 +155,12 @@ bind collision is benign, so no rank is dropped. Full details: | `NIXL_TELEMETRY_PROMETHEUS_PORT` | Scrape port (shared with the `prometheus` exporter) | `9090` | | `NIXL_TELEMETRY_PROMETHEUS_LOCAL` | Bind `127.0.0.1` instead of `0.0.0.0` | `false` | | `NIXL_TELEMETRY_RANK_ENV` | Name of the env var holding the rank for the optional `local_rank` label; no label if that env var is unset | `LOCAL_RANK` | -| `NIXL_TELEMETRY_MP_STALE_TTL` | Seconds after which a dead process's store is stale and reaped | `30` | +| `NIXL_TELEMETRY_MP_STALE_TTL` | Seconds after a dead process's last update before its store is stale and reaped | `30` | -Series are labeled by `hostname`, `agent_name`, `pid` (guarantees uniqueness), and -optionally `local_rank` (the local/per-GPU rank, not Dynamo's data-parallel -`dp_rank`). Metric names, types, and semantics are identical to the +Series are labeled by `hostname`, `agent_name`, `pid` (guarantees cross-process +uniqueness), `agent_instance` (distinguishes multiple same-name agents within one +process), and optionally `local_rank` (the local/per-GPU rank, not Dynamo's +data-parallel `dp_rank`). Metric names, types, and semantics are identical to the single-process `prometheus` exporter. `NIXL_TELEMETRY_MULTIPROC_DIR` follows Dynamo's `PROMETHEUS_MULTIPROC_DIR` diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index 474e8d6b0a..e0142b4649 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -88,7 +88,8 @@ export NIXL_TELEMETRY_PROMETHEUS_LOCAL="y" # bind 127.0.0.1 instead of 0.0.0.0 # If that env var is unset, no local_rank label is emitted (series stay unique via pid). export NIXL_TELEMETRY_RANK_ENV="LOCAL_RANK" -# Seconds after which a dead process's store is considered stale and reaped (default 30). +# Seconds after a dead process's last update before its store is considered stale +# and reaped (default 30). A live process is always published regardless of age. export NIXL_TELEMETRY_MP_STALE_TTL="30" ``` From ad437a8a64789e57ff1daf6623521c2afb7a5c7f Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Fri, 10 Jul 2026 16:52:12 +0000 Subject: [PATCH 16/71] docs: note the status label on agent_errors_total in telemetry.md The prometheus_mp label list omitted that agent_errors_total additionally carries the bounded status label, unlike the plugin README. Align the two. Signed-off-by: Efraim Eygin --- docs/telemetry.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/telemetry.md b/docs/telemetry.md index 07d82777da..dd5d140182 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -160,7 +160,8 @@ bind collision is benign, so no rank is dropped. Full details: Series are labeled by `hostname`, `agent_name`, `pid` (guarantees cross-process uniqueness), `agent_instance` (distinguishes multiple same-name agents within one process), and optionally `local_rank` (the local/per-GPU rank, not Dynamo's -data-parallel `dp_rank`). Metric names, types, and semantics are identical to the +data-parallel `dp_rank`). `agent_errors_total` additionally includes the bounded +`status` label. Metric names, types, and semantics are identical to the single-process `prometheus` exporter. `NIXL_TELEMETRY_MULTIPROC_DIR` follows Dynamo's `PROMETHEUS_MULTIPROC_DIR` From 4f7036f2983ee848f0b2d673c9dbe8029aeaded2 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Wed, 22 Jul 2026 20:31:01 +0000 Subject: [PATCH 17/71] telemetry: address prometheus_mp review (ordering, RAII, const, owner_) Review feedback on the prometheus_mp plugin: - mp_store: drop the redundant RELEASE on lastUpdateNs (the magic store is the single publish barrier; the heartbeat is best-effort RELAXED like the counters). - mp_store: RAII the read-side mmap in readStoreSnapshot with a unique_ptr guard instead of an explicit munmap at every early return. - mp_collector: make the collector's members const. - exporter: drop the redundant owner_ flag; isExporter() returns bool(exposer_). - exporter: resolve the local_rank value through nixl::config instead of a raw getenv, so env/config resolution stays homogeneous. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/prometheus_mp/mp_collector.h | 6 +++--- src/plugins/telemetry/prometheus_mp/mp_store.cpp | 12 ++++++------ .../prometheus_mp/prometheus_mp_exporter.cpp | 5 +---- .../telemetry/prometheus_mp/prometheus_mp_exporter.h | 3 +-- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.h b/src/plugins/telemetry/prometheus_mp/mp_collector.h index b58e3b7859..633bf820fd 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.h +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.h @@ -89,9 +89,9 @@ class nixlMultiprocessCollector final : public prometheus::Collectable { Collect() const override; private: - std::filesystem::path dir_; - std::chrono::nanoseconds staleTtl_; - bool reapStale_; + const std::filesystem::path dir_; + const std::chrono::nanoseconds staleTtl_; + const bool reapStale_; }; } // namespace nixl::telemetry::mp diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index ac9edda590..66ea2fc775 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -159,7 +160,7 @@ mpStoreWriter::mpStoreWriter(std::filesystem::path path, copyField(layout->agentName, MP_MAX_AGENT_NAME, agent_name, "agent name"); copyField(layout->hostname, MP_MAX_HOSTNAME, hostname, "hostname"); copyField(layout->localRank, MP_MAX_LOCAL_RANK, local_rank, "local_rank"); - __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELEASE); + __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELAXED); // Publish the magic last so a concurrent reader never validates a // half-initialized header. __atomic_store_n(&layout->magic, MP_STORE_MAGIC, __ATOMIC_RELEASE); @@ -177,7 +178,7 @@ mpStoreWriter::~mpStoreWriter() { void mpStoreWriter::touch() noexcept { auto *layout = static_cast(mapping_); - __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELEASE); + __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELAXED); } void @@ -230,6 +231,9 @@ readStoreSnapshot(const std::filesystem::path &path) { return std::nullopt; } + const std::unique_ptr guard( + mapping, [](void *p) noexcept { ::munmap(p, sizeof(mpStoreLayout)); }); + const auto *layout = static_cast(mapping); const uint64_t magic = __atomic_load_n(&layout->magic, __ATOMIC_ACQUIRE); @@ -237,13 +241,11 @@ readStoreSnapshot(const std::filesystem::path &path) { // Zeroed header: either a store still being initialized by a live process, // or an orphan left by a process that died mid-creation. Skip quietly (no // WARN); the collector reaps stale orphans by file age. - ::munmap(mapping, sizeof(mpStoreLayout)); return std::nullopt; } if (magic != MP_STORE_MAGIC) { NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() << "' with bad magic"; - ::munmap(mapping, sizeof(mpStoreLayout)); return std::nullopt; } if (layout->schemaVersion != MP_STORE_SCHEMA_VERSION || @@ -251,7 +253,6 @@ readStoreSnapshot(const std::filesystem::path &path) { NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() << "' with incompatible schema (version " << layout->schemaVersion << ", slots " << layout->slotCount << ")"; - ::munmap(mapping, sizeof(mpStoreLayout)); return std::nullopt; } @@ -268,7 +269,6 @@ readStoreSnapshot(const std::filesystem::path &path) { snap.gauges[i] = __atomic_load_n(&layout->gauges[i], __ATOMIC_RELAXED); } - ::munmap(mapping, sizeof(mpStoreLayout)); return snap; } diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp index e109e13507..91cc7051dd 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -74,8 +74,7 @@ resolveLocalRank() { if (rank_source.empty()) { return {}; } - const char *value = std::getenv(rank_source.c_str()); - return value != nullptr ? std::string(value) : std::string(); + return nixl::config::getValueOptional(rank_source).value_or(std::string()); } [[nodiscard]] std::chrono::nanoseconds @@ -131,7 +130,6 @@ nixlTelemetryPrometheusMpExporter::nixlTelemetryPrometheusMpExporter( collector_ = std::make_shared(dir, resolveStaleTtl()); exposer->RegisterCollectable(collector_); exposer_ = std::move(exposer); - owner_ = true; NIXL_INFO << "prometheus_mp exporter (owner) serving " << bind_address << ", aggregating telemetry dir " << dir.string(); } @@ -139,7 +137,6 @@ nixlTelemetryPrometheusMpExporter::nixlTelemetryPrometheusMpExporter( if (std::string(e.what()).find(bindFailureMarker) == std::string::npos) { throw; } - owner_ = false; NIXL_INFO << "prometheus_mp exporter (writer): endpoint " << bind_address << " owned by another process; agent '" << init_params.agentName << "' writing to " << store_path.string(); diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h index 919040befe..afeca8abf5 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h @@ -49,7 +49,7 @@ class nixlTelemetryPrometheusMpExporter final : public nixlTelemetryExporter { // True if this process won the bind race and serves the scrape endpoint. [[nodiscard]] bool isExporter() const noexcept { - return owner_; + return static_cast(exposer_); } private: @@ -58,7 +58,6 @@ class nixlTelemetryPrometheusMpExporter final : public nixlTelemetryExporter { std::unique_ptr store_; std::shared_ptr collector_; std::shared_ptr exposer_; - bool owner_ = false; }; #endif // NIXL_SRC_PLUGINS_TELEMETRY_PROMETHEUS_MP_PROMETHEUS_MP_EXPORTER_H From e982ed39207a2063353fc085a2df5e18f908f792 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Wed, 22 Jul 2026 20:47:47 +0000 Subject: [PATCH 18/71] telemetry: centralize getHostname() into common/hostname.h The prometheus, prometheus_mp, and DOCA exporters each carried their own getHostname() copy. Replace them with one shared nixl::getHostname() in common/hostname.h that returns std::optional; each call site keeps its "unknown" fallback via value_or. Behavior unchanged. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/doca/doca_exporter.cpp | 13 +----- .../prometheus/prometheus_exporter.cpp | 13 +----- .../prometheus_mp/prometheus_mp_exporter.cpp | 18 +++----- src/utils/common/hostname.h | 42 +++++++++++++++++++ 4 files changed, 52 insertions(+), 34 deletions(-) create mode 100644 src/utils/common/hostname.h diff --git a/src/plugins/telemetry/doca/doca_exporter.cpp b/src/plugins/telemetry/doca/doca_exporter.cpp index 0e55e7f2da..0c1c1c2f08 100644 --- a/src/plugins/telemetry/doca/doca_exporter.cpp +++ b/src/plugins/telemetry/doca/doca_exporter.cpp @@ -17,6 +17,7 @@ #include "doca_exporter.h" #include "common/configuration.h" #include "common/exception.h" +#include "common/hostname.h" #include "common/nixl_log.h" #include "common/str_util.h" #include "histogram_buckets.h" @@ -107,16 +108,6 @@ parseExporterConfig() { return config; } -[[nodiscard]] std::string -getHostname() { - std::array hostname{}; - if (gethostname(hostname.data(), hostname.size()) == 0) { - hostname.back() = '\0'; - return std::string(hostname.data()); - } - return "unknown"; -} - [[nodiscard]] uint64_t docaTimestamp() noexcept { uint64_t ts = 0; @@ -178,7 +169,7 @@ struct DocaSharedContext { DocaSharedContext::DocaSharedContext(const DocaExporterConfig &config) : scrape_enabled(config.scrape), ipc_enabled(config.ipc) { - const std::string hostname = getHostname(); + const std::string hostname = nixl::getHostname().value_or("unknown"); const std::string &bind_address = config.bind_address; const std::string &ipc_sockets_dir = config.ipc_sockets_dir; diff --git a/src/plugins/telemetry/prometheus/prometheus_exporter.cpp b/src/plugins/telemetry/prometheus/prometheus_exporter.cpp index 94e81ac918..27c4ce8f4d 100644 --- a/src/plugins/telemetry/prometheus/prometheus_exporter.cpp +++ b/src/plugins/telemetry/prometheus/prometheus_exporter.cpp @@ -16,6 +16,7 @@ */ #include "prometheus_exporter.h" #include "common/configuration.h" +#include "common/hostname.h" #include "common/nixl_log.h" #include "histogram_buckets.h" @@ -38,16 +39,6 @@ const char prometheusLocalVar[] = "NIXL_TELEMETRY_PROMETHEUS_LOCAL"; const std::string prometheusExporterLocalAddress = "127.0.0.1"; const std::string prometheusExporterPublicAddress = "0.0.0.0"; -std::string -getHostname() { - char hostname[HOST_NAME_MAX + 1]; - if (gethostname(hostname, sizeof(hostname)) == 0) { - hostname[HOST_NAME_MAX] = '\0'; // Ensure null-termination - return std::string(hostname); - } - return "unknown"; -} - std::mutex s_mutex; std::weak_ptr s_exposer_weak; std::weak_ptr s_registry_weak; @@ -58,7 +49,7 @@ nixlTelemetryPrometheusExporter::nixlTelemetryPrometheusExporter( const nixlTelemetryExporterInitParams &init_params) : nixlTelemetryExporter(init_params), agent_name_(init_params.agentName), - hostname_(getHostname()) { + hostname_(nixl::getHostname().value_or("unknown")) { const bool local = nixl::config::getValueDefaulted(prometheusLocalVar, false); const uint16_t port = nixl::config::getValueDefaulted(prometheusPortVar, prometheusExporterDefaultPort); diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp index 91cc7051dd..de5bd0507e 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -17,6 +17,7 @@ #include "prometheus_mp_exporter.h" #include "common/configuration.h" +#include "common/hostname.h" #include "common/nixl_log.h" #include @@ -53,16 +54,6 @@ const std::string publicAddress = "0.0.0.0"; // bind collision; any other Exposer failure is a genuine error. constexpr char bindFailureMarker[] = "Failed to setup server ports"; -[[nodiscard]] std::string -getHostname() { - char hostname[HOST_NAME_MAX + 1]; - if (gethostname(hostname, sizeof(hostname)) == 0) { - hostname[HOST_NAME_MAX] = '\0'; - return std::string(hostname); - } - return "unknown"; -} - // Resolves the optional local_rank label value: NIXL_TELEMETRY_RANK_ENV names // which env var holds the rank (default LOCAL_RANK); the value of that env var is // the rank. Empty when the named env var is unset -- rank is a best-effort label @@ -117,8 +108,11 @@ nixlTelemetryPrometheusMpExporter::nixlTelemetryPrometheusMpExporter( const uint64_t instance = s_instanceSeq.fetch_add(1, std::memory_order_relaxed); const std::filesystem::path store_path = dir / makeStoreFileName(pid, start_time, instance); - store_ = std::make_unique( - store_path, init_params.agentName, getHostname(), resolveLocalRank(), instance); + store_ = std::make_unique(store_path, + init_params.agentName, + nixl::getHostname().value_or("unknown"), + resolveLocalRank(), + instance); const bool local = nixl::config::getValueDefaulted(prometheusLocalVar, false); const uint16_t port = nixl::config::getValueDefaulted(prometheusPortVar, defaultPort); diff --git a/src/utils/common/hostname.h b/src/utils/common/hostname.h new file mode 100644 index 0000000000..807df813e8 --- /dev/null +++ b/src/utils/common/hostname.h @@ -0,0 +1,42 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef NIXL_SRC_UTILS_COMMON_HOSTNAME_H +#define NIXL_SRC_UTILS_COMMON_HOSTNAME_H + +#include +#include +#include + +#include +#include + +namespace nixl { + +// Returns the host name, or std::nullopt if it could not be read; callers pick +// their own fallback (e.g. .value_or("unknown")). +[[nodiscard]] inline std::optional +getHostname() { + std::array buf{}; + if (::gethostname(buf.data(), buf.size() - 1) == 0) { + return std::string(buf.data()); + } + return std::nullopt; +} + +} // namespace nixl + +#endif // NIXL_SRC_UTILS_COMMON_HOSTNAME_H From 6452ac5ec249ddb5ef64f645acd80f2e91c710e8 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Wed, 22 Jul 2026 20:54:02 +0000 Subject: [PATCH 19/71] telemetry: drop redundant mp prefix from prometheus_mp store types Rename mpStoreSnapshot/mpStoreWriter/mpStoreLayout to storeSnapshot/storeWriter/storeLayout -- the mp prefix is redundant inside the nixl::telemetry::mp namespace (review feedback). No behavior change. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/mp_collector.cpp | 8 ++-- .../telemetry/prometheus_mp/mp_collector.h | 4 +- .../telemetry/prometheus_mp/mp_store.cpp | 44 +++++++++---------- .../telemetry/prometheus_mp/mp_store.h | 34 +++++++------- .../prometheus_mp/prometheus_mp_exporter.cpp | 12 ++--- .../prometheus_mp/prometheus_mp_exporter.h | 2 +- test/gtest/telemetry_mp_collector_test.cpp | 12 ++--- test/gtest/telemetry_mp_store_test.cpp | 12 ++--- 8 files changed, 64 insertions(+), 64 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index d7c73aab3d..aa3f9b1b75 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -42,7 +42,7 @@ namespace { using prometheus::MetricType; [[nodiscard]] std::vector - baseLabels(const mpStoreSnapshot &s) { + baseLabels(const storeSnapshot &s) { std::vector labels; labels.push_back({"hostname", s.hostname}); labels.push_back({"agent_name", s.agentName}); @@ -126,7 +126,7 @@ isProcessAlive(int64_t pid, uint64_t start_time) { } bool -isSnapshotLive(const mpStoreSnapshot &snap, std::chrono::nanoseconds ttl) { +isSnapshotLive(const storeSnapshot &snap, std::chrono::nanoseconds ttl) { if (isProcessAlive(snap.pid, snap.startTime)) { return true; } @@ -136,7 +136,7 @@ isSnapshotLive(const mpStoreSnapshot &snap, std::chrono::nanoseconds ttl) { } std::vector -buildMetricFamilies(const std::vector &snapshots) { +buildMetricFamilies(const std::vector &snapshots) { std::vector families; if (snapshots.empty()) { return families; @@ -200,7 +200,7 @@ nixlMultiprocessCollector::nixlMultiprocessCollector(std::filesystem::path dir, std::vector nixlMultiprocessCollector::Collect() const { - std::vector live; + std::vector live; std::error_code ec; std::filesystem::directory_iterator it(dir_, ec); diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.h b/src/plugins/telemetry/prometheus_mp/mp_collector.h index 633bf820fd..43e7dac40f 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.h +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.h @@ -50,7 +50,7 @@ isProcessAlive(int64_t pid, uint64_t start_time); * @param ttl Freshness window for a process that is no longer alive. */ [[nodiscard]] bool -isSnapshotLive(const mpStoreSnapshot &snap, std::chrono::nanoseconds ttl); +isSnapshotLive(const storeSnapshot &snap, std::chrono::nanoseconds ttl); /** * @brief Converts per-process snapshots into Prometheus metric families. @@ -63,7 +63,7 @@ isSnapshotLive(const mpStoreSnapshot &snap, std::chrono::nanoseconds ttl); * @param snapshots Live per-process snapshots. */ [[nodiscard]] std::vector -buildMetricFamilies(const std::vector &snapshots); +buildMetricFamilies(const std::vector &snapshots); /** * @class nixlMultiprocessCollector diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 66ea2fc775..5d53bc20ec 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -50,7 +50,7 @@ namespace { // Fixed on-disk layout. Plain trivially-copyable POD operated on with __atomic // builtins (not std::atomic) so it is safe to memset/reinterpret over an mmap'd // region shared between processes. Field order keeps every uint64 8-byte aligned. - struct mpStoreLayout { + struct storeLayout { uint64_t magic; uint32_t schemaVersion; uint32_t slotCount; @@ -122,13 +122,13 @@ readProcessStartTime(int64_t pid) { } } -mpStoreWriter::mpStoreWriter(std::filesystem::path path, - const std::string &agent_name, - const std::string &hostname, - const std::string &local_rank, - uint64_t instance) +storeWriter::storeWriter(std::filesystem::path path, + const std::string &agent_name, + const std::string &hostname, + const std::string &local_rank, + uint64_t instance) : path_(std::move(path)), - mappingSize_(sizeof(mpStoreLayout)) { + mappingSize_(sizeof(storeLayout)) { const int fd = ::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC, 0644); if (fd < 0) { throw std::runtime_error("prometheus_mp: cannot open telemetry store '" + path_.string() + @@ -150,7 +150,7 @@ mpStoreWriter::mpStoreWriter(std::filesystem::path path, "': " + std::strerror(errno)); } - auto *layout = static_cast(mapping_); + auto *layout = static_cast(mapping_); std::memset(layout, 0, mappingSize_); layout->schemaVersion = MP_STORE_SCHEMA_VERSION; layout->slotCount = static_cast(MP_STORE_SLOT_COUNT); @@ -166,7 +166,7 @@ mpStoreWriter::mpStoreWriter(std::filesystem::path path, __atomic_store_n(&layout->magic, MP_STORE_MAGIC, __ATOMIC_RELEASE); } -mpStoreWriter::~mpStoreWriter() { +storeWriter::~storeWriter() { if (mapping_ != nullptr) { ::munmap(mapping_, mappingSize_); mapping_ = nullptr; @@ -176,39 +176,39 @@ mpStoreWriter::~mpStoreWriter() { } void -mpStoreWriter::touch() noexcept { - auto *layout = static_cast(mapping_); +storeWriter::touch() noexcept { + auto *layout = static_cast(mapping_); __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELAXED); } void -mpStoreWriter::addCounter(nixl_telemetry_event_type_t type, uint64_t delta) noexcept { +storeWriter::addCounter(nixl_telemetry_event_type_t type, uint64_t delta) noexcept { const auto idx = static_cast(type); if (idx >= MP_STORE_SLOT_COUNT) { return; } - auto *layout = static_cast(mapping_); + auto *layout = static_cast(mapping_); __atomic_fetch_add(&layout->counters[idx], delta, __ATOMIC_RELAXED); touch(); } void -mpStoreWriter::setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept { +storeWriter::setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept { const auto idx = static_cast(type); if (idx >= MP_STORE_SLOT_COUNT) { return; } - auto *layout = static_cast(mapping_); + auto *layout = static_cast(mapping_); __atomic_store_n(&layout->gauges[idx], value, __ATOMIC_RELAXED); touch(); } void -mpStoreWriter::refreshHeartbeat() noexcept { +storeWriter::refreshHeartbeat() noexcept { touch(); } -std::optional +std::optional readStoreSnapshot(const std::filesystem::path &path) { const int fd = ::open(path.c_str(), O_RDONLY | O_CLOEXEC); if (fd < 0) { @@ -217,13 +217,13 @@ readStoreSnapshot(const std::filesystem::path &path) { } struct stat st{}; - if (::fstat(fd, &st) != 0 || static_cast(st.st_size) < sizeof(mpStoreLayout)) { + if (::fstat(fd, &st) != 0 || static_cast(st.st_size) < sizeof(storeLayout)) { // Too small: likely a file mid-creation by a peer. Skip quietly. ::close(fd); return std::nullopt; } - void *mapping = ::mmap(nullptr, sizeof(mpStoreLayout), PROT_READ, MAP_SHARED, fd, 0); + void *mapping = ::mmap(nullptr, sizeof(storeLayout), PROT_READ, MAP_SHARED, fd, 0); ::close(fd); if (mapping == MAP_FAILED) { NIXL_WARN << "prometheus_mp: cannot map telemetry store '" << path.string() @@ -232,9 +232,9 @@ readStoreSnapshot(const std::filesystem::path &path) { } const std::unique_ptr guard( - mapping, [](void *p) noexcept { ::munmap(p, sizeof(mpStoreLayout)); }); + mapping, [](void *p) noexcept { ::munmap(p, sizeof(storeLayout)); }); - const auto *layout = static_cast(mapping); + const auto *layout = static_cast(mapping); const uint64_t magic = __atomic_load_n(&layout->magic, __ATOMIC_ACQUIRE); if (magic == 0) { @@ -256,7 +256,7 @@ readStoreSnapshot(const std::filesystem::path &path) { return std::nullopt; } - mpStoreSnapshot snap; + storeSnapshot snap; snap.pid = layout->pid; snap.startTime = layout->startTime; snap.instance = layout->instance; diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 6c88a1580e..615a947faa 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -78,7 +78,7 @@ static_assert(detail::maxTelemetrySlot() < MP_STORE_SLOT_COUNT, * shared file); @c counters are cumulative running totals and @c gauges hold the * last-operation value, both indexed by nixl_telemetry_event_type_t. */ -struct mpStoreSnapshot { +struct storeSnapshot { int64_t pid = 0; // Process start time in clock ticks (/proc//stat field 22); pairs with // pid to survive PID reuse when checking liveness. @@ -98,7 +98,7 @@ struct mpStoreSnapshot { }; /** - * @class mpStoreWriter + * @class storeWriter * @brief Owns one process's metric-state mmap file and updates it in place. * * Each NIXL process (writer or exporter mode) owns exactly one store. Updates @@ -106,7 +106,7 @@ struct mpStoreSnapshot { * process can read peers' files concurrently without coordination. The file has * a fixed size (fixed slot layout), so it never needs to grow. */ -class mpStoreWriter { +class storeWriter { public: /** * @brief Creates (or truncates) and maps the store file at @p path. @@ -118,19 +118,19 @@ class mpStoreWriter { * same-named agents in one process so their series stay distinct. * @throws std::runtime_error on open/ftruncate/mmap failure. */ - mpStoreWriter(std::filesystem::path path, - const std::string &agent_name, - const std::string &hostname, - const std::string &local_rank, - uint64_t instance); - ~mpStoreWriter(); - - mpStoreWriter(const mpStoreWriter &) = delete; - mpStoreWriter & - operator=(const mpStoreWriter &) = delete; - mpStoreWriter(mpStoreWriter &&) = delete; - mpStoreWriter & - operator=(mpStoreWriter &&) = delete; + storeWriter(std::filesystem::path path, + const std::string &agent_name, + const std::string &hostname, + const std::string &local_rank, + uint64_t instance); + ~storeWriter(); + + storeWriter(const storeWriter &) = delete; + storeWriter & + operator=(const storeWriter &) = delete; + storeWriter(storeWriter &&) = delete; + storeWriter & + operator=(storeWriter &&) = delete; // Adds @p delta to the cumulative counter slot for @p type and refreshes the // heartbeat. Out-of-range types are ignored. @@ -168,7 +168,7 @@ class mpStoreWriter { * has a bad magic / incompatible schema version (a WARN is logged for a * present-but-invalid file). */ -[[nodiscard]] std::optional +[[nodiscard]] std::optional readStoreSnapshot(const std::filesystem::path &path); /** diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp index de5bd0507e..692854fc60 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -32,7 +32,7 @@ namespace { using nixl::telemetry::mp::makeStoreFileName; -using nixl::telemetry::mp::mpStoreWriter; +using nixl::telemetry::mp::storeWriter; using nixl::telemetry::mp::nixlMultiprocessCollector; using nixl::telemetry::mp::readProcessStartTime; @@ -108,11 +108,11 @@ nixlTelemetryPrometheusMpExporter::nixlTelemetryPrometheusMpExporter( const uint64_t instance = s_instanceSeq.fetch_add(1, std::memory_order_relaxed); const std::filesystem::path store_path = dir / makeStoreFileName(pid, start_time, instance); - store_ = std::make_unique(store_path, - init_params.agentName, - nixl::getHostname().value_or("unknown"), - resolveLocalRank(), - instance); + store_ = std::make_unique(store_path, + init_params.agentName, + nixl::getHostname().value_or("unknown"), + resolveLocalRank(), + instance); const bool local = nixl::config::getValueDefaulted(prometheusLocalVar, false); const uint16_t port = nixl::config::getValueDefaulted(prometheusPortVar, defaultPort); diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h index afeca8abf5..e001faca41 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.h @@ -55,7 +55,7 @@ class nixlTelemetryPrometheusMpExporter final : public nixlTelemetryExporter { private: // Declared so destruction is exposer_ -> collector_ -> store_: stop serving // before dropping the collector it weak-references, then unmap the store. - std::unique_ptr store_; + std::unique_ptr store_; std::shared_ptr collector_; std::shared_ptr exposer_; }; diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp index 108c170750..cf806ce8ee 100644 --- a/test/gtest/telemetry_mp_collector_test.cpp +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -40,8 +40,8 @@ using nixl::telemetry::mp::buildMetricFamilies; using nixl::telemetry::mp::isProcessAlive; using nixl::telemetry::mp::isSnapshotLive; using nixl::telemetry::mp::makeStoreFileName; -using nixl::telemetry::mp::mpStoreSnapshot; -using nixl::telemetry::mp::mpStoreWriter; +using nixl::telemetry::mp::storeSnapshot; +using nixl::telemetry::mp::storeWriter; using nixl::telemetry::mp::nixlMultiprocessCollector; using nixl::telemetry::mp::readProcessStartTime; @@ -53,9 +53,9 @@ idx(nixl_telemetry_event_type_t t) { return static_cast(t); } -[[nodiscard]] mpStoreSnapshot +[[nodiscard]] storeSnapshot makeSnap(const std::string &agent, const std::string &rank) { - mpStoreSnapshot s; + storeSnapshot s; s.pid = ::getpid(); s.startTime = 1; s.lastUpdateNs = nixlTime::getNs(); @@ -259,10 +259,10 @@ class MpCollectorFileTest : public ::testing::Test { TEST_F(MpCollectorFileTest, CollectReadsLiveStoresAndIgnoresOthers) { // Two distinct store files; both headers stamp this (live) process. - mpStoreWriter w1(dir_ / makeStoreFileName(111, 1, 0), "agent-1", "host", "0", 0); + storeWriter w1(dir_ / makeStoreFileName(111, 1, 0), "agent-1", "host", "0", 0); w1.addCounter(TX_BYTES, 500); w1.setGauge(TX_BYTES, 500); - mpStoreWriter w2(dir_ / makeStoreFileName(222, 2, 0), "agent-2", "host", "1", 0); + storeWriter w2(dir_ / makeStoreFileName(222, 2, 0), "agent-2", "host", "1", 0); w2.addCounter(TX_BYTES, 700); // A non-store file must be ignored. diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index a798b5a85f..e1078a53c9 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -29,7 +29,7 @@ namespace { -using nixl::telemetry::mp::mpStoreWriter; +using nixl::telemetry::mp::storeWriter; using nixl::telemetry::mp::readProcessStartTime; using nixl::telemetry::mp::readStoreSnapshot; @@ -68,7 +68,7 @@ class MpStoreTest : public ::testing::Test { TEST_F(MpStoreTest, WriteReadRoundTrip) { const auto path = storePath("agent-a"); - mpStoreWriter writer(path, "agent-a", "host-1", "3", 7); + storeWriter writer(path, "agent-a", "host-1", "3", 7); writer.addCounter(TX_BYTES, 1000); writer.setGauge(TX_BYTES, 1000); writer.addCounter(ERR_BACKEND, 1); @@ -92,7 +92,7 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { const auto path = storePath("agent-b"); - mpStoreWriter writer(path, "agent-b", "host-1", "", 0); + storeWriter writer(path, "agent-b", "host-1", "", 0); writer.addCounter(TX_BYTES, 100); writer.addCounter(TX_BYTES, 250); writer.addCounter(TX_BYTES, 650); @@ -107,7 +107,7 @@ TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { TEST_F(MpStoreTest, EmptyRankIsEmpty) { const auto path = storePath("agent-c"); - mpStoreWriter writer(path, "agent-c", "host-1", "", 0); + storeWriter writer(path, "agent-c", "host-1", "", 0); const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); @@ -117,7 +117,7 @@ TEST_F(MpStoreTest, EmptyRankIsEmpty) { TEST_F(MpStoreTest, DestructorRemovesStoreFile) { const auto path = storePath("agent-cleanup"); { - mpStoreWriter writer(path, "agent-cleanup", "host-1", "", 0); + storeWriter writer(path, "agent-cleanup", "host-1", "", 0); EXPECT_TRUE(std::filesystem::exists(path)); } EXPECT_FALSE(std::filesystem::exists(path)); @@ -127,7 +127,7 @@ TEST_F(MpStoreTest, LongAgentNameTruncated) { const auto path = storePath("agent-long"); const std::string long_name(1000, 'x'); const gtest::LogIgnoreGuard lig("exceeds 255 chars"); - mpStoreWriter writer(path, long_name, "host-1", "", 0); + storeWriter writer(path, long_name, "host-1", "", 0); const auto snap = readStoreSnapshot(path); ASSERT_TRUE(snap.has_value()); From 1cc0fdcf968dec2e42fa9883f4e9c87916ab0c0e Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 14:21:26 +0000 Subject: [PATCH 20/71] telemetry: check ::write result in prometheus_mp e2e test glibc marks write() warn_unused_result under _FORTIFY_SOURCE (Ubuntu GCC at -O2 / debugoptimized), and the gtest target builds -Werror, so the ignored return broke the build on that config (CI configures --buildtype=debug, which skips it). Check the write and _exit on short write. Signed-off-by: Efraim Eygin --- test/gtest/telemetry_mp_e2e_test.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/gtest/telemetry_mp_e2e_test.cpp b/test/gtest/telemetry_mp_e2e_test.cpp index 90bf48e0b9..488adc5ddb 100644 --- a/test/gtest/telemetry_mp_e2e_test.cpp +++ b/test/gtest/telemetry_mp_e2e_test.cpp @@ -139,7 +139,9 @@ runWriterChild(int go_fd, int ready_fd, int quit_fd, const std::string &agent, u nixlTelemetryPrometheusMpExporter exporter(initParams(agent)); exporter.exportEvent({TX_BYTES, tx_value}); const char ok = 1; - ::write(ready_fd, &ok, 1); + if (::write(ready_fd, &ok, 1) != 1) { + ::_exit(4); + } // Block until the parent closes the quit pipe. while (::read(quit_fd, &c, 1) > 0) {} } From 0d3ecacb3f9b42e634a1590a7bd2355a3bfd5b66 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 14:28:20 +0000 Subject: [PATCH 21/71] telemetry: prometheus_mp review cleanups (dead API, TTL default, store opens) - Remove storeWriter::refreshHeartbeat(): declared/defined/documented but never called (the periodic-heartbeat idea was dropped), so it was dead API surface. - Dedupe the stale-TTL default: the exporter now derives it from MP_DEFAULT_STALE_TTL instead of a second hardcoded 30 that could diverge. - Harden the store files: open with O_NOFOLLOW and create 0600 so a pre-planted symlink in a shared directory can't redirect the truncate/unlink, and other users can't read a peer's store. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/prometheus_mp/mp_store.cpp | 9 ++------- src/plugins/telemetry/prometheus_mp/mp_store.h | 5 ----- .../telemetry/prometheus_mp/prometheus_mp_exporter.cpp | 6 +++--- 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 5d53bc20ec..79b7defb45 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -129,7 +129,7 @@ storeWriter::storeWriter(std::filesystem::path path, uint64_t instance) : path_(std::move(path)), mappingSize_(sizeof(storeLayout)) { - const int fd = ::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC, 0644); + const int fd = ::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC | O_NOFOLLOW, 0600); if (fd < 0) { throw std::runtime_error("prometheus_mp: cannot open telemetry store '" + path_.string() + "': " + std::strerror(errno)); @@ -203,14 +203,9 @@ storeWriter::setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept touch(); } -void -storeWriter::refreshHeartbeat() noexcept { - touch(); -} - std::optional readStoreSnapshot(const std::filesystem::path &path) { - const int fd = ::open(path.c_str(), O_RDONLY | O_CLOEXEC); + const int fd = ::open(path.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW); if (fd < 0) { // Missing/unreadable file is not an error here (peer may have exited). return std::nullopt; diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 615a947faa..9a94bbb72e 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -142,11 +142,6 @@ class storeWriter { void setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept; - // Refreshes the last-update timestamp without changing any metric (keeps the - // process from looking stale during idle periods). - void - refreshHeartbeat() noexcept; - [[nodiscard]] const std::filesystem::path & path() const noexcept { return path_; diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp index 692854fc60..7d158b975b 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -32,12 +32,12 @@ namespace { using nixl::telemetry::mp::makeStoreFileName; +using nixl::telemetry::mp::MP_DEFAULT_STALE_TTL; using nixl::telemetry::mp::storeWriter; using nixl::telemetry::mp::nixlMultiprocessCollector; using nixl::telemetry::mp::readProcessStartTime; constexpr uint16_t defaultPort = 9090; -constexpr uint64_t defaultStaleTtlSeconds = 30; constexpr char defaultRankEnvName[] = "LOCAL_RANK"; constexpr char prometheusPortVar[] = "NIXL_TELEMETRY_PROMETHEUS_PORT"; @@ -70,8 +70,8 @@ resolveLocalRank() { [[nodiscard]] std::chrono::nanoseconds resolveStaleTtl() { - const uint64_t seconds = - nixl::config::getValueDefaulted(staleTtlVar, defaultStaleTtlSeconds); + const uint64_t seconds = nixl::config::getValueDefaulted( + staleTtlVar, static_cast(MP_DEFAULT_STALE_TTL.count())); return std::chrono::duration_cast(std::chrono::seconds(seconds)); } From 87b1b942dbd32620c5e13c765cc04ab1c6681db0 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 14:32:43 +0000 Subject: [PATCH 22/71] docs: prometheus_mp private-dir and shared-PID-namespace requirements Document two deployment requirements the code assumes: use a private 0700 directory (not world-writable /tmp; the files are O_NOFOLLOW/0600 but the dir is the operator's responsibility), and run all aggregated ranks in one PID/time namespace (liveness uses kill(pid,0) + /proc; a host-wide CLOCK_MONOTONIC), i.e. one container/process family or shareProcessNamespace. Also change the example dir from /tmp to /run. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/prometheus_mp/README.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index e0142b4649..a03cfd7df5 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -56,7 +56,7 @@ Same as the `prometheus` plug-in: the bundled prometheus-cpp subproject and ```bash export NIXL_TELEMETRY_ENABLE="y" export NIXL_TELEMETRY_EXPORTER="prometheus_mp" # selects libtelemetry_exporter_prometheus_mp.so -export NIXL_TELEMETRY_MULTIPROC_DIR="/tmp/nixl_metrics" # REQUIRED: shared by all ranks in the pod +export NIXL_TELEMETRY_MULTIPROC_DIR="/run/nixl_metrics" # REQUIRED: shared by all ranks in the pod ``` This mirrors Dynamo's `PROMETHEUS_MULTIPROC_DIR` convention (a shared folder that @@ -77,6 +77,19 @@ Memory-medium `emptyDir` or `/dev/shm`) works and avoids any disk writeback, but optional -- a plain local dir is fine, since updates hit the page cache and the per-process store files are ~one page each. +Use a **private** directory (mode `0700`, owned by the run's user) rather than a +world-writable location like `/tmp`. On a shared host a world-writable directory +lets another user pre-plant paths the owner would truncate or unlink. The plugin +already hardens the files themselves (opened with `O_NOFOLLOW`, created `0600`), +but the directory's permissions are the deployment's responsibility. + +All aggregated ranks must also share a **PID namespace** (and time namespace): +staleness/liveness uses `kill(pid, 0)` + `/proc//stat` and a host-wide +`CLOCK_MONOTONIC`, so ranks must run in one process family / container (or a pod +with `shareProcessNamespace: true`). Ranks in separate PID namespaces sharing only +the directory would misidentify each other's liveness -- keeping dead series or +reaping live ones. + ### Optional configuration ```bash From 8162099bf8a8236cb8b9bba0c3bd17d684c84255 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 14:40:31 +0000 Subject: [PATCH 23/71] telemetry: prometheus_mp test hygiene + plugin-loader coverage - Add MpExporterTest.LoadsThroughPluginManager, which constructs nixlTelemetry(agent, "prometheus_mp") so the plugin loader path (nixl_telemetry_plugin_init, name, API version) is exercised, not just direct exporter construction. - MissingMultiprocDirThrows: use ScopedEnv (empty dir, treated as unset) instead of a raw ::unsetenv that leaked global state into later tests. - Drop the redundant popVar() loops in both TearDown()s: ScopedEnv's members restore on destruction, and the hardcoded loop count could silently drift. Signed-off-by: Efraim Eygin --- test/gtest/telemetry_mp_e2e_test.cpp | 3 --- test/gtest/telemetry_mp_exporter_test.cpp | 12 ++++++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/test/gtest/telemetry_mp_e2e_test.cpp b/test/gtest/telemetry_mp_e2e_test.cpp index 488adc5ddb..eadeb35bd8 100644 --- a/test/gtest/telemetry_mp_e2e_test.cpp +++ b/test/gtest/telemetry_mp_e2e_test.cpp @@ -169,9 +169,6 @@ class MpE2ETest : public ::testing::Test { void TearDown() override { - for (int i = 0; i < 4; ++i) { - env_.popVar(); - } std::error_code ec; std::filesystem::remove_all(dir_, ec); } diff --git a/test/gtest/telemetry_mp_exporter_test.cpp b/test/gtest/telemetry_mp_exporter_test.cpp index 0fc83479b2..c35d33b028 100644 --- a/test/gtest/telemetry_mp_exporter_test.cpp +++ b/test/gtest/telemetry_mp_exporter_test.cpp @@ -16,6 +16,7 @@ */ #include "prometheus_mp_exporter.h" #include "mp_store.h" +#include "telemetry.h" #include "common.h" @@ -62,9 +63,6 @@ class MpExporterTest : public ::testing::Test { void TearDown() override { - env_.popVar(); - env_.popVar(); - env_.popVar(); std::error_code ec; std::filesystem::remove_all(dir_, ec); } @@ -121,8 +119,14 @@ TEST_F(MpExporterTest, WriterModeWhenPortTaken) { EXPECT_EQ(snap->counters[idx(RX_BYTES)], 77u); } +TEST_F(MpExporterTest, LoadsThroughPluginManager) { + nixlTelemetry telemetry("agent-loader", "prometheus_mp"); + EXPECT_FALSE(singleStoreFile().empty()); +} + TEST(MpExporterStandaloneTest, MissingMultiprocDirThrows) { - ::unsetenv("NIXL_TELEMETRY_MULTIPROC_DIR"); + gtest::ScopedEnv env; + env.addVar("NIXL_TELEMETRY_MULTIPROC_DIR", ""); EXPECT_THROW( { nixlTelemetryPrometheusMpExporter exporter(nixlTelemetryExporterInitParams{"a", 4096}); }, std::runtime_error); From b7c6380eb2af0bfe9f56a08d27c298b57a5e0874 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 15:20:37 +0000 Subject: [PATCH 24/71] telemetry: don't reap a live prometheus_mp peer on a transient read error readStoreSnapshot returned std::nullopt indistinguishably for "file gone", "transient open/mmap failure" (EMFILE/EACCES/ENOMEM), and "content invalid" (too small / bad or zero magic / wrong schema). The collector treated every nullopt as an orphan and reaped it purely by file mtime -- which never advances for a live MAP_SHARED writer -- so under fd exhaustion the owner could unlink a healthy peer's store, and that peer's series would vanish for the rest of its life. Add a content_invalid out-param that is set only when the file was actually read and found unusable; the collector now reaps an unparsable file only when that is true. Transient failures (and missing files) leave it false and are skipped. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/mp_collector.cpp | 11 +++++--- .../telemetry/prometheus_mp/mp_store.cpp | 23 ++++++++++++++--- .../telemetry/prometheus_mp/mp_store.h | 11 +++++--- test/gtest/telemetry_mp_store_test.cpp | 25 ++++++++++++++----- 4 files changed, 53 insertions(+), 17 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index aa3f9b1b75..80be8b1ab1 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -222,11 +222,14 @@ nixlMultiprocessCollector::Collect() const { if (!entry.is_regular_file(ec) || !nameMatchesStore(entry.path().filename().string())) { continue; } - auto snap = readStoreSnapshot(entry.path()); + bool content_invalid = false; + auto snap = readStoreSnapshot(entry.path(), &content_invalid); if (!snap) { - // Unparsable (mid-init, orphaned, or incompatible): reap only if it is - // old enough to not be a store a live process is currently creating. - if (reapStale_ && invalidFileReapable(entry.path(), staleTtl_)) { + // Reap only genuinely bad content (bad/zero magic, wrong schema, + // truncated) that is old enough to not be mid-creation. A transient + // open/mmap failure leaves content_invalid false, so a healthy peer we + // simply failed to read is never unlinked. + if (reapStale_ && content_invalid && invalidFileReapable(entry.path(), staleTtl_)) { std::error_code rm_ec; std::filesystem::remove(entry.path(), rm_ec); } diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 79b7defb45..576bf2df9a 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -204,23 +204,37 @@ storeWriter::setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept } std::optional -readStoreSnapshot(const std::filesystem::path &path) { +readStoreSnapshot(const std::filesystem::path &path, bool *content_invalid) { + const auto mark_invalid = [content_invalid] { + if (content_invalid != nullptr) { + *content_invalid = true; + } + }; + if (content_invalid != nullptr) { + *content_invalid = false; + } + const int fd = ::open(path.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW); if (fd < 0) { - // Missing/unreadable file is not an error here (peer may have exited). + // Missing or transiently unreadable (e.g. EMFILE): not necessarily an + // orphan, so leave content_invalid false so the collector never reaps a + // live peer we simply failed to open. return std::nullopt; } struct stat st{}; if (::fstat(fd, &st) != 0 || static_cast(st.st_size) < sizeof(storeLayout)) { - // Too small: likely a file mid-creation by a peer. Skip quietly. + // Too small: a truncated/mid-creation store -- unusable content. ::close(fd); + mark_invalid(); return std::nullopt; } void *mapping = ::mmap(nullptr, sizeof(storeLayout), PROT_READ, MAP_SHARED, fd, 0); ::close(fd); if (mapping == MAP_FAILED) { + // Transient (e.g. ENOMEM): the file may be a healthy peer's, so do not + // mark it reapable. NIXL_WARN << "prometheus_mp: cannot map telemetry store '" << path.string() << "': " << std::strerror(errno); return std::nullopt; @@ -236,11 +250,13 @@ readStoreSnapshot(const std::filesystem::path &path) { // Zeroed header: either a store still being initialized by a live process, // or an orphan left by a process that died mid-creation. Skip quietly (no // WARN); the collector reaps stale orphans by file age. + mark_invalid(); return std::nullopt; } if (magic != MP_STORE_MAGIC) { NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() << "' with bad magic"; + mark_invalid(); return std::nullopt; } if (layout->schemaVersion != MP_STORE_SCHEMA_VERSION || @@ -248,6 +264,7 @@ readStoreSnapshot(const std::filesystem::path &path) { NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() << "' with incompatible schema (version " << layout->schemaVersion << ", slots " << layout->slotCount << ")"; + mark_invalid(); return std::nullopt; } diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 9a94bbb72e..9be5e0ba04 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -159,12 +159,15 @@ class storeWriter { /** * @brief Reads a consistent snapshot of a store file written by another process. * @param path Path to a peer's store file. - * @return The snapshot, or std::nullopt if the file is missing, too small, or - * has a bad magic / incompatible schema version (a WARN is logged for a - * present-but-invalid file). + * @param[out] content_invalid Set to true only when the file was read and its + * content is unusable (too small, bad/zero magic, incompatible schema) -- + * i.e. a genuine orphan safe to reap. Left false when the file could not + * be opened or mapped (missing, or a transient error such as EMFILE/ + * ENOMEM), so a live peer is never mistaken for an orphan. Optional. + * @return The snapshot, or std::nullopt on any of the above. */ [[nodiscard]] std::optional -readStoreSnapshot(const std::filesystem::path &path); +readStoreSnapshot(const std::filesystem::path &path, bool *content_invalid = nullptr); /** * @brief Reads a process's start time (/proc//stat field 22, clock ticks). diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index e1078a53c9..1e2dac271c 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -73,8 +73,10 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { writer.setGauge(TX_BYTES, 1000); writer.addCounter(ERR_BACKEND, 1); - const auto snap = readStoreSnapshot(path); + bool content_invalid = true; + const auto snap = readStoreSnapshot(path, &content_invalid); ASSERT_TRUE(snap.has_value()); + EXPECT_FALSE(content_invalid); EXPECT_EQ(snap->agentName, "agent-a"); EXPECT_EQ(snap->hostname, "host-1"); EXPECT_EQ(snap->localRank, "3"); @@ -136,8 +138,13 @@ TEST_F(MpStoreTest, LongAgentNameTruncated) { EXPECT_EQ(lig.getIgnoredCount(), 1); } -TEST_F(MpStoreTest, MissingFileReturnsNullopt) { - EXPECT_FALSE(readStoreSnapshot(storePath("does-not-exist")).has_value()); +TEST_F(MpStoreTest, MissingFileReturnsNulloptNotContentInvalid) { + // A file we cannot open (missing here, or a transient error) must NOT be + // reported as invalid content -- otherwise the collector could reap a live + // peer it merely failed to read. content_invalid must be reset to false. + bool content_invalid = true; + EXPECT_FALSE(readStoreSnapshot(storePath("does-not-exist"), &content_invalid).has_value()); + EXPECT_FALSE(content_invalid); } TEST_F(MpStoreTest, TooSmallFileReturnsNullopt) { @@ -147,7 +154,9 @@ TEST_F(MpStoreTest, TooSmallFileReturnsNullopt) { const char junk[16] = {0}; f.write(junk, sizeof(junk)); } - EXPECT_FALSE(readStoreSnapshot(path).has_value()); + bool content_invalid = false; + EXPECT_FALSE(readStoreSnapshot(path, &content_invalid).has_value()); + EXPECT_TRUE(content_invalid); } TEST_F(MpStoreTest, ZeroMagicReturnsNulloptQuietly) { @@ -159,7 +168,9 @@ TEST_F(MpStoreTest, ZeroMagicReturnsNulloptQuietly) { const std::string zeros(64 * 1024, '\0'); f.write(zeros.data(), static_cast(zeros.size())); } - EXPECT_FALSE(readStoreSnapshot(path).has_value()); + bool content_invalid = false; + EXPECT_FALSE(readStoreSnapshot(path, &content_invalid).has_value()); + EXPECT_TRUE(content_invalid); } TEST_F(MpStoreTest, BadMagicWarnsAndReturnsNullopt) { @@ -172,7 +183,9 @@ TEST_F(MpStoreTest, BadMagicWarnsAndReturnsNullopt) { f.write(zeros.data(), static_cast(zeros.size())); } const gtest::LogIgnoreGuard lig("bad magic"); - EXPECT_FALSE(readStoreSnapshot(path).has_value()); + bool content_invalid = false; + EXPECT_FALSE(readStoreSnapshot(path, &content_invalid).has_value()); + EXPECT_TRUE(content_invalid); EXPECT_EQ(lig.getIgnoredCount(), 1); } From 557c0e64937c7691f1a3e713c1f5f4a446f90f51 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 16:00:21 +0000 Subject: [PATCH 25/71] telemetry: harden prometheus_mp store reads against exceptions Replace readStoreSnapshot()'s nullable bool out-parameter with a storeReadResult, so callers cannot forget to initialize it and the null-guard lambda at every failure site disappears. Guard the fd in readStoreSnapshot() and storeWriter's constructor with RAII, matching the mmap guard: nothing between open() and close() throws today, but a single added log line would leak the descriptor. Wrap nixlMultiprocessCollector::Collect() in a catch-all. prometheus-cpp invokes it on its HTTP handler thread, where an escaping exception fails the scrape at best and terminates the process at worst; telemetry must never take down the data plane. Silence the plugin-manager probe warning in LoadsThroughPluginManager, which fires only when another telemetry plugin directory is registered first, and document that pid/agent_instance make a restarted rank a new series. Signed-off-by: Efraim Eygin --- src/plugins/telemetry/prometheus_mp/README.md | 9 ++ .../telemetry/prometheus_mp/mp_collector.cpp | 26 +++++- .../telemetry/prometheus_mp/mp_collector.h | 5 ++ .../telemetry/prometheus_mp/mp_store.cpp | 82 ++++++++++--------- .../telemetry/prometheus_mp/mp_store.h | 26 ++++-- test/gtest/telemetry_mp_exporter_test.cpp | 7 +- test/gtest/telemetry_mp_store_test.cpp | 38 ++++----- 7 files changed, 123 insertions(+), 70 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index a03cfd7df5..cb438ab1e1 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -142,6 +142,15 @@ not reuse, Python `prometheus_client`'s multiprocess format): high-cardinality label** whose value varies per observation. No NIXL metric has such a label today; if one is ever added, this exporter would need a different (keyed) store. +- **Process churn creates new series.** `pid` and `agent_instance` are what keep + each process's counters monotonic, but they also mean a restarted rank is a + fresh series rather than a continuation: it gets a new `pid`, and the instance + counter restarts at `0`. The exposition only ever contains live processes, so + scrape size is bounded by the current process count -- but the TSDB accumulates + one series set per process seen within the retention window, so a crash-looping + deployment grows cardinality at the restart rate. Aggregate the churning labels + away (`sum without (pid, agent_instance) (...)`) for stable per-rank or + per-host views. This is the native, dependency-free path. For aggregation through an external telemetry service, use the DOCA/CollectX exporter (IPC to DTS) instead. diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index 80be8b1ab1..be689b50fb 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include namespace nixl::telemetry::mp { @@ -200,6 +201,24 @@ nixlMultiprocessCollector::nixlMultiprocessCollector(std::filesystem::path dir, std::vector nixlMultiprocessCollector::Collect() const { + // prometheus-cpp calls this on its HTTP handler thread; an exception escaping + // into that handler (std::bad_alloc from the allocations below, say) would + // fail the scrape at best and terminate the process at worst. Telemetry must + // never take down the data plane, so degrade to an empty scrape instead. + try { + return buildMetricFamilies(scanLiveStores()); + } + catch (const std::exception &e) { + NIXL_WARN << "prometheus_mp: telemetry collection failed: " << e.what(); + } + catch (...) { + NIXL_WARN << "prometheus_mp: telemetry collection failed"; + } + return {}; +} + +std::vector +nixlMultiprocessCollector::scanLiveStores() const { std::vector live; std::error_code ec; @@ -222,12 +241,11 @@ nixlMultiprocessCollector::Collect() const { if (!entry.is_regular_file(ec) || !nameMatchesStore(entry.path().filename().string())) { continue; } - bool content_invalid = false; - auto snap = readStoreSnapshot(entry.path(), &content_invalid); + auto [snap, content_invalid] = readStoreSnapshot(entry.path()); if (!snap) { // Reap only genuinely bad content (bad/zero magic, wrong schema, // truncated) that is old enough to not be mid-creation. A transient - // open/mmap failure leaves content_invalid false, so a healthy peer we + // open/mmap failure leaves contentInvalid false, so a healthy peer we // simply failed to read is never unlinked. if (reapStale_ && content_invalid && invalidFileReapable(entry.path(), staleTtl_)) { std::error_code rm_ec; @@ -243,7 +261,7 @@ nixlMultiprocessCollector::Collect() const { } } - return buildMetricFamilies(live); + return live; } } // namespace nixl::telemetry::mp diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.h b/src/plugins/telemetry/prometheus_mp/mp_collector.h index 43e7dac40f..e770e3a32c 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.h +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.h @@ -89,6 +89,11 @@ class nixlMultiprocessCollector final : public prometheus::Collectable { Collect() const override; private: + // Scans the shared directory once, dropping (and optionally reaping) stores + // that are stale or unusable. + [[nodiscard]] std::vector + scanLiveStores() const; + const std::filesystem::path dir_; const std::chrono::nanoseconds staleTtl_; const bool reapStale_; diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 576bf2df9a..4328dca244 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -82,6 +82,32 @@ namespace { return std::string(src, n); } + class scopedFd { + public: + explicit scopedFd(int fd) noexcept : fd_(fd) {} + + ~scopedFd() { + if (fd_ >= 0) { + ::close(fd_); + } + } + + scopedFd(const scopedFd &) = delete; + scopedFd & + operator=(const scopedFd &) = delete; + scopedFd(scopedFd &&) = delete; + scopedFd & + operator=(scopedFd &&) = delete; + + [[nodiscard]] int + get() const noexcept { + return fd_; + } + + private: + int fd_; + }; + } // namespace std::string @@ -129,21 +155,18 @@ storeWriter::storeWriter(std::filesystem::path path, uint64_t instance) : path_(std::move(path)), mappingSize_(sizeof(storeLayout)) { - const int fd = ::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC | O_NOFOLLOW, 0600); - if (fd < 0) { + const scopedFd fd(::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC | O_NOFOLLOW, 0600)); + if (fd.get() < 0) { throw std::runtime_error("prometheus_mp: cannot open telemetry store '" + path_.string() + "': " + std::strerror(errno)); } - if (::ftruncate(fd, static_cast(mappingSize_)) != 0) { - const std::string reason = std::strerror(errno); - ::close(fd); + if (::ftruncate(fd.get(), static_cast(mappingSize_)) != 0) { throw std::runtime_error("prometheus_mp: cannot size telemetry store '" + path_.string() + - "': " + reason); + "': " + std::strerror(errno)); } - mapping_ = ::mmap(nullptr, mappingSize_, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); - ::close(fd); + mapping_ = ::mmap(nullptr, mappingSize_, PROT_READ | PROT_WRITE, MAP_SHARED, fd.get(), 0); if (mapping_ == MAP_FAILED) { mapping_ = nullptr; throw std::runtime_error("prometheus_mp: cannot map telemetry store '" + path_.string() + @@ -203,41 +226,29 @@ storeWriter::setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept touch(); } -std::optional -readStoreSnapshot(const std::filesystem::path &path, bool *content_invalid) { - const auto mark_invalid = [content_invalid] { - if (content_invalid != nullptr) { - *content_invalid = true; - } - }; - if (content_invalid != nullptr) { - *content_invalid = false; - } - - const int fd = ::open(path.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW); - if (fd < 0) { +storeReadResult +readStoreSnapshot(const std::filesystem::path &path) { + const scopedFd fd(::open(path.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW)); + if (fd.get() < 0) { // Missing or transiently unreadable (e.g. EMFILE): not necessarily an - // orphan, so leave content_invalid false so the collector never reaps a + // orphan, so leave contentInvalid false and the collector never reaps a // live peer we simply failed to open. - return std::nullopt; + return {std::nullopt, false}; } struct stat st{}; - if (::fstat(fd, &st) != 0 || static_cast(st.st_size) < sizeof(storeLayout)) { + if (::fstat(fd.get(), &st) != 0 || static_cast(st.st_size) < sizeof(storeLayout)) { // Too small: a truncated/mid-creation store -- unusable content. - ::close(fd); - mark_invalid(); - return std::nullopt; + return {std::nullopt, true}; } - void *mapping = ::mmap(nullptr, sizeof(storeLayout), PROT_READ, MAP_SHARED, fd, 0); - ::close(fd); + void *mapping = ::mmap(nullptr, sizeof(storeLayout), PROT_READ, MAP_SHARED, fd.get(), 0); if (mapping == MAP_FAILED) { // Transient (e.g. ENOMEM): the file may be a healthy peer's, so do not // mark it reapable. NIXL_WARN << "prometheus_mp: cannot map telemetry store '" << path.string() << "': " << std::strerror(errno); - return std::nullopt; + return {std::nullopt, false}; } const std::unique_ptr guard( @@ -250,22 +261,19 @@ readStoreSnapshot(const std::filesystem::path &path, bool *content_invalid) { // Zeroed header: either a store still being initialized by a live process, // or an orphan left by a process that died mid-creation. Skip quietly (no // WARN); the collector reaps stale orphans by file age. - mark_invalid(); - return std::nullopt; + return {std::nullopt, true}; } if (magic != MP_STORE_MAGIC) { NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() << "' with bad magic"; - mark_invalid(); - return std::nullopt; + return {std::nullopt, true}; } if (layout->schemaVersion != MP_STORE_SCHEMA_VERSION || layout->slotCount != MP_STORE_SLOT_COUNT) { NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() << "' with incompatible schema (version " << layout->schemaVersion << ", slots " << layout->slotCount << ")"; - mark_invalid(); - return std::nullopt; + return {std::nullopt, true}; } storeSnapshot snap; @@ -281,7 +289,7 @@ readStoreSnapshot(const std::filesystem::path &path, bool *content_invalid) { snap.gauges[i] = __atomic_load_n(&layout->gauges[i], __ATOMIC_RELAXED); } - return snap; + return {std::move(snap), false}; } } // namespace nixl::telemetry::mp diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 9be5e0ba04..61d94f0d01 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -156,18 +156,28 @@ class storeWriter { std::size_t mappingSize_ = 0; }; +/** + * @brief Outcome of readStoreSnapshot(). + * + * @c contentInvalid is true only when the file was read and its content is + * unusable (too small, bad/zero magic, incompatible schema) -- i.e. a genuine + * orphan safe to reap. It stays false when the file could not be opened or + * mapped (missing, or a transient error such as EMFILE/ENOMEM), so a live peer + * is never mistaken for an orphan. + */ +struct storeReadResult { + std::optional snapshot; + bool contentInvalid = false; +}; + /** * @brief Reads a consistent snapshot of a store file written by another process. * @param path Path to a peer's store file. - * @param[out] content_invalid Set to true only when the file was read and its - * content is unusable (too small, bad/zero magic, incompatible schema) -- - * i.e. a genuine orphan safe to reap. Left false when the file could not - * be opened or mapped (missing, or a transient error such as EMFILE/ - * ENOMEM), so a live peer is never mistaken for an orphan. Optional. - * @return The snapshot, or std::nullopt on any of the above. + * @return The snapshot when the store was read and is compatible, otherwise an + * empty snapshot and the reason category (see storeReadResult). */ -[[nodiscard]] std::optional -readStoreSnapshot(const std::filesystem::path &path, bool *content_invalid = nullptr); +[[nodiscard]] storeReadResult +readStoreSnapshot(const std::filesystem::path &path); /** * @brief Reads a process's start time (/proc//stat field 22, clock ticks). diff --git a/test/gtest/telemetry_mp_exporter_test.cpp b/test/gtest/telemetry_mp_exporter_test.cpp index c35d33b028..8507ac6b57 100644 --- a/test/gtest/telemetry_mp_exporter_test.cpp +++ b/test/gtest/telemetry_mp_exporter_test.cpp @@ -94,7 +94,7 @@ TEST_F(MpExporterTest, OwnerBindsAndRecordsToStore) { exporter.exportEvent({TX_BYTES, 1234}); - const auto snap = readStoreSnapshot(file); + const auto snap = readStoreSnapshot(file).snapshot; ASSERT_TRUE(snap.has_value()); EXPECT_EQ(snap->agentName, "agent-owner"); EXPECT_EQ(snap->counters[idx(TX_BYTES)], 1234u); @@ -113,13 +113,16 @@ TEST_F(MpExporterTest, WriterModeWhenPortTaken) { exporter.exportEvent({RX_BYTES, 77}); - const auto snap = readStoreSnapshot(file); + const auto snap = readStoreSnapshot(file).snapshot; ASSERT_TRUE(snap.has_value()); EXPECT_EQ(snap->agentName, "agent-writer"); EXPECT_EQ(snap->counters[idx(RX_BYTES)], 77u); } TEST_F(MpExporterTest, LoadsThroughPluginManager) { + // The plugin manager probes every registered plugin directory, so it warns + // about the ones that do not hold this plugin before finding the one that does. + const gtest::LogIgnoreGuard lig("Plugin file does not exist"); nixlTelemetry telemetry("agent-loader", "prometheus_mp"); EXPECT_FALSE(singleStoreFile().empty()); } diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index 1e2dac271c..678ac28d9f 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -73,10 +73,10 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { writer.setGauge(TX_BYTES, 1000); writer.addCounter(ERR_BACKEND, 1); - bool content_invalid = true; - const auto snap = readStoreSnapshot(path, &content_invalid); + const auto res = readStoreSnapshot(path); + const auto &snap = res.snapshot; ASSERT_TRUE(snap.has_value()); - EXPECT_FALSE(content_invalid); + EXPECT_FALSE(res.contentInvalid); EXPECT_EQ(snap->agentName, "agent-a"); EXPECT_EQ(snap->hostname, "host-1"); EXPECT_EQ(snap->localRank, "3"); @@ -101,7 +101,7 @@ TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { writer.setGauge(TX_BYTES, 100); writer.setGauge(TX_BYTES, 650); // last write wins - const auto snap = readStoreSnapshot(path); + const auto snap = readStoreSnapshot(path).snapshot; ASSERT_TRUE(snap.has_value()); EXPECT_EQ(snap->counters[idx(TX_BYTES)], 1000u); EXPECT_EQ(snap->gauges[idx(TX_BYTES)], 650u); @@ -111,7 +111,7 @@ TEST_F(MpStoreTest, EmptyRankIsEmpty) { const auto path = storePath("agent-c"); storeWriter writer(path, "agent-c", "host-1", "", 0); - const auto snap = readStoreSnapshot(path); + const auto snap = readStoreSnapshot(path).snapshot; ASSERT_TRUE(snap.has_value()); EXPECT_TRUE(snap->localRank.empty()); } @@ -131,7 +131,7 @@ TEST_F(MpStoreTest, LongAgentNameTruncated) { const gtest::LogIgnoreGuard lig("exceeds 255 chars"); storeWriter writer(path, long_name, "host-1", "", 0); - const auto snap = readStoreSnapshot(path); + const auto snap = readStoreSnapshot(path).snapshot; ASSERT_TRUE(snap.has_value()); EXPECT_EQ(snap->agentName.size(), 255u); EXPECT_EQ(snap->agentName, long_name.substr(0, 255)); @@ -141,10 +141,10 @@ TEST_F(MpStoreTest, LongAgentNameTruncated) { TEST_F(MpStoreTest, MissingFileReturnsNulloptNotContentInvalid) { // A file we cannot open (missing here, or a transient error) must NOT be // reported as invalid content -- otherwise the collector could reap a live - // peer it merely failed to read. content_invalid must be reset to false. - bool content_invalid = true; - EXPECT_FALSE(readStoreSnapshot(storePath("does-not-exist"), &content_invalid).has_value()); - EXPECT_FALSE(content_invalid); + // peer it merely failed to read. + const auto res = readStoreSnapshot(storePath("does-not-exist")); + EXPECT_FALSE(res.snapshot.has_value()); + EXPECT_FALSE(res.contentInvalid); } TEST_F(MpStoreTest, TooSmallFileReturnsNullopt) { @@ -154,9 +154,9 @@ TEST_F(MpStoreTest, TooSmallFileReturnsNullopt) { const char junk[16] = {0}; f.write(junk, sizeof(junk)); } - bool content_invalid = false; - EXPECT_FALSE(readStoreSnapshot(path, &content_invalid).has_value()); - EXPECT_TRUE(content_invalid); + const auto res = readStoreSnapshot(path); + EXPECT_FALSE(res.snapshot.has_value()); + EXPECT_TRUE(res.contentInvalid); } TEST_F(MpStoreTest, ZeroMagicReturnsNulloptQuietly) { @@ -168,9 +168,9 @@ TEST_F(MpStoreTest, ZeroMagicReturnsNulloptQuietly) { const std::string zeros(64 * 1024, '\0'); f.write(zeros.data(), static_cast(zeros.size())); } - bool content_invalid = false; - EXPECT_FALSE(readStoreSnapshot(path, &content_invalid).has_value()); - EXPECT_TRUE(content_invalid); + const auto res = readStoreSnapshot(path); + EXPECT_FALSE(res.snapshot.has_value()); + EXPECT_TRUE(res.contentInvalid); } TEST_F(MpStoreTest, BadMagicWarnsAndReturnsNullopt) { @@ -183,9 +183,9 @@ TEST_F(MpStoreTest, BadMagicWarnsAndReturnsNullopt) { f.write(zeros.data(), static_cast(zeros.size())); } const gtest::LogIgnoreGuard lig("bad magic"); - bool content_invalid = false; - EXPECT_FALSE(readStoreSnapshot(path, &content_invalid).has_value()); - EXPECT_TRUE(content_invalid); + const auto res = readStoreSnapshot(path); + EXPECT_FALSE(res.snapshot.has_value()); + EXPECT_TRUE(res.contentInvalid); EXPECT_EQ(lig.getIgnoredCount(), 1); } From b90bc432efad2986fadcdf67c8167eb4a902fe39 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 17:13:02 +0000 Subject: [PATCH 26/71] telemetry: histogram support for the prometheus_mp exporter prometheus_mp exposed counters and gauges but silently dropped the transfer-duration histograms the single-process prometheus exporter publishes, so agent_xfer_time_us and agent_xfer_post_time_us were absent from a multi-process scrape. Land the plugin at parity rather than bumping the store schema for it after merge. The fixed-layout store gains per-bucket observation counts and a sum per event type, plus the bucket bounds themselves, since each process resolves NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US from its own environment. Bucket counts are stored non-cumulative and accumulated at scrape time: a reader racing a writer can then never observe non-monotonic buckets, and sample_count is derived from the same read so it cannot disagree with them. A fixed layout has to cap the bucket count, so an override longer than 32 bounds is rejected at construction instead of being silently truncated - the one behavioural difference from the single-process exporter. Signed-off-by: Efraim Eygin --- docs/telemetry.md | 10 ++- src/plugins/telemetry/prometheus_mp/README.md | 24 +++++-- .../telemetry/prometheus_mp/meson.build | 3 +- .../telemetry/prometheus_mp/mp_collector.cpp | 41 ++++++++++++ .../telemetry/prometheus_mp/mp_store.cpp | 47 +++++++++++++- .../telemetry/prometheus_mp/mp_store.h | 33 +++++++++- .../prometheus_mp/prometheus_mp_exporter.cpp | 7 +- test/gtest/telemetry_mp_collector_test.cpp | 59 ++++++++++++++++- test/gtest/telemetry_mp_e2e_test.cpp | 14 +++- test/gtest/telemetry_mp_exporter_test.cpp | 22 +++++++ test/gtest/telemetry_mp_store_test.cpp | 64 +++++++++++++++++-- 11 files changed, 300 insertions(+), 24 deletions(-) diff --git a/docs/telemetry.md b/docs/telemetry.md index 1020c53191..14cc7d3564 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -74,15 +74,18 @@ stream: type is emitted and the buffer format is unchanged. - **Latency histograms**: the transfer-time events additionally feed distribution histograms `agent_xfer_time_us` / `agent_xfer_post_time_us` (microseconds) on - both the Prometheus and DOCA exporters, at parity (same names, buckets, labels). + the Prometheus, multi-process Prometheus and DOCA exporters, at parity (same + names, buckets, labels). Each is exposed as the standard `_bucket{le="..."}` / `_sum` / `_count` series alongside the existing counter and gauge. Bucket boundaries default to a microsecond range covering ~10us..~10s and are overridable via `NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US` (a comma-separated list of strictly-increasing positive microsecond upper bounds; when absent or empty the built-in defaults are used, while a non-empty but invalid value is rejected and - the exporter fails to initialize rather than silently using the defaults). Like - the other views this is an exporter-side derivation with no new event type. + the exporter fails to initialize rather than silently using the defaults). + `prometheus_mp` additionally caps the override at 32 bounds, since its buckets + live in a fixed-layout shared-memory store. Like the other views this is an + exporter-side derivation with no new event type. - **Error counters**: the Prometheus and DOCA exporters expose error events as `agent_errors_total{status=""}`. The `status` label is bounded by the fixed `AGENT_ERR_*` event set: `not_posted`, `invalid_param`, `backend`, @@ -171,6 +174,7 @@ bind collision is benign, so no rank is dropped. Full details: | `NIXL_TELEMETRY_PROMETHEUS_LOCAL` | Bind `127.0.0.1` instead of `0.0.0.0` | `false` | | `NIXL_TELEMETRY_RANK_ENV` | Name of the env var holding the rank for the optional `local_rank` label; no label if that env var is unset | `LOCAL_RANK` | | `NIXL_TELEMETRY_MP_STALE_TTL` | Seconds after a dead process's last update before its store is stale and reaped | `30` | +| `NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US` | Shared with the other exporters, but capped at 32 bounds by the fixed-layout store | built-in µs defaults | Series are labeled by `hostname`, `agent_name`, `pid` (guarantees cross-process uniqueness), `agent_instance` (distinguishes multiple same-name agents within one diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index cb438ab1e1..e9d4cb50e2 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -44,8 +44,8 @@ Same as the `prometheus` plug-in: the bundled prometheus-cpp subproject and HTTP server). A bind collision is therefore benign -- every process gets a valid telemetry sink; no rank is dropped and no scary error is logged. - **Per-process series.** Each process is exported as its own series (cumulative - counters and last-operation gauges), never summed across processes, so - per-process values stay correct and monotonic. + counters, last-operation gauges and duration histograms), never summed across + processes, so per-process values stay correct and monotonic. - **Stale handling.** On clean shutdown a process removes its own store file. If it instead crashes or is killed -- and so cannot clean up after itself -- the owner drops its series once the process is gone (verified by pid + `/proc` start @@ -104,6 +104,12 @@ export NIXL_TELEMETRY_RANK_ENV="LOCAL_RANK" # Seconds after a dead process's last update before its store is considered stale # and reaped (default 30). A live process is always published regardless of age. export NIXL_TELEMETRY_MP_STALE_TTL="30" + +# Histogram bucket upper bounds in microseconds -- shared with the prometheus and +# DOCA exporters. This exporter keeps its buckets in the fixed-layout store, so at +# most 32 bounds are accepted; a longer list fails agent construction rather than +# being silently truncated. +export NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US="10,100,1000" ``` ## Metric labels @@ -123,9 +129,11 @@ Every series is labeled by: from Dynamo's data-parallel `dp_rank`. - `status` -- only on `agent_errors_total`, bounded by the fixed `AGENT_ERR_*` set. -The metric names, types, counter/gauge semantics, and events are identical to the -single-process [`prometheus`](../prometheus/README.md) exporter (same shared -descriptor). +The metric names, types, semantics, and events are identical to the single-process +[`prometheus`](../prometheus/README.md) exporter (same shared descriptor). That +includes the transfer-duration histograms `agent_xfer_time_us` and +`agent_xfer_post_time_us`, exposed as the usual `_bucket{le="..."}` / `_sum` / +`_count` series per process. ## Design scope & limitations @@ -134,7 +142,11 @@ Prometheus multiprocess store** (in particular it is not compatible with, and do not reuse, Python `prometheus_client`'s multiprocess format): - The metric set is fixed at compile time; slots are positional, so metric names - are never stored in the files. + are never stored in the files. Histogram bucket bounds are the one exception -- + they are stored per file, because each process resolves them from its own + environment. Ranks configured with different bounds therefore contribute series + with different `le` sets to the same family; give every rank the same + `NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US`. - Per-process label values (`hostname`, `agent_name`, `pid`, `local_rank`) are captured once at startup and never change. Events carry only a numeric value -- there are **no per-observation labels**. diff --git a/src/plugins/telemetry/prometheus_mp/meson.build b/src/plugins/telemetry/prometheus_mp/meson.build index 4d08ccda01..0c85b2e25f 100644 --- a/src/plugins/telemetry/prometheus_mp/meson.build +++ b/src/plugins/telemetry/prometheus_mp/meson.build @@ -58,7 +58,8 @@ if prometheus_found telemetry_mp_exporter_lib = static_library( 'nixl_telemetry_mp_exporter', 'prometheus_mp_exporter.cpp', - include_directories: [nixl_inc_dirs, utils_inc_dirs, prometheus_mp_inc_dirs], + include_directories: [nixl_inc_dirs, utils_inc_dirs, prometheus_mp_inc_dirs, + include_directories('../common')], dependencies: [nixl_common_dep, prometheus_core_dep, prometheus_pull_dep, nixl_telemetry_mp_store_dep, nixl_telemetry_mp_collector_dep], install: false, diff --git a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp index be689b50fb..8cf4e2b0b5 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_collector.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_collector.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include namespace nixl::telemetry::mp { @@ -74,6 +75,30 @@ namespace { return m; } + // Turns the store's per-bucket counts into the cumulative form Prometheus + // expects, ending with the explicit +Inf bucket that prometheus-cpp's own + // Histogram::Collect() emits. + [[nodiscard]] ClientMetric + histogramMetric(std::vector labels, + const storeSnapshot &s, + std::size_t slot) { + ClientMetric m; + m.label = std::move(labels); + m.histogram.bucket.reserve(s.bucketCount + 1); + uint64_t cumulative = 0; + for (std::size_t i = 0; i <= s.bucketCount; ++i) { + cumulative += s.histBuckets[slot][i]; + ClientMetric::Bucket bucket; + bucket.cumulative_count = cumulative; + bucket.upper_bound = + (i == s.bucketCount) ? std::numeric_limits::infinity() : s.bucketBounds[i]; + m.histogram.bucket.push_back(bucket); + } + m.histogram.sample_count = cumulative; + m.histogram.sample_sum = static_cast(s.histSums[slot]); + return m; + } + // Minimum age before an unparsable file is reaped, even when the TTL is 0. // Protects a store a live process is actively creating (a sub-second window) // from being deleted out from under it. @@ -175,6 +200,22 @@ buildMetricFamilies(const std::vector &snapshots) { families.push_back(std::move(family)); } + for (const auto type : telemetry_metric_event_types) { + const auto descriptor = nixlEnumStrings::telemetryMetricDescriptor(type); + if (descriptor.histogramName == nullptr) { + continue; + } + MetricFamily family; + family.name = descriptor.histogramName; + family.help = descriptor.histogramHelp; + family.type = MetricType::Histogram; + const auto slot = static_cast(type); + for (const auto &snap : snapshots) { + family.metric.push_back(histogramMetric(baseLabels(snap), snap, slot)); + } + families.push_back(std::move(family)); + } + MetricFamily errors; errors.name = "agent_errors_total"; errors.help = "Cumulative error count by status"; diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index 4328dca244..cb0b99d373 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -58,11 +58,17 @@ namespace { uint64_t startTime; uint64_t lastUpdateNs; uint64_t instance; + // 64-bit purely so the double array that follows stays 8-byte aligned + // without implicit padding. + uint64_t bucketCount; char agentName[MP_MAX_AGENT_NAME]; char hostname[MP_MAX_HOSTNAME]; char localRank[MP_MAX_LOCAL_RANK]; + double bucketBounds[MP_STORE_MAX_BUCKETS]; uint64_t counters[MP_STORE_SLOT_COUNT]; uint64_t gauges[MP_STORE_SLOT_COUNT]; + uint64_t histBuckets[MP_STORE_SLOT_COUNT][MP_STORE_MAX_BUCKETS + 1]; + uint64_t histSums[MP_STORE_SLOT_COUNT]; }; void @@ -152,9 +158,17 @@ storeWriter::storeWriter(std::filesystem::path path, const std::string &agent_name, const std::string &hostname, const std::string &local_rank, - uint64_t instance) + uint64_t instance, + const std::vector &histogram_buckets) : path_(std::move(path)), mappingSize_(sizeof(storeLayout)) { + if (histogram_buckets.size() > MP_STORE_MAX_BUCKETS) { + throw std::runtime_error("prometheus_mp: NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US has " + + std::to_string(histogram_buckets.size()) + + " bounds, more than the " + std::to_string(MP_STORE_MAX_BUCKETS) + + " a multi-process store can hold"); + } + const scopedFd fd(::open(path_.c_str(), O_CREAT | O_RDWR | O_CLOEXEC | O_NOFOLLOW, 0600)); if (fd.get() < 0) { throw std::runtime_error("prometheus_mp: cannot open telemetry store '" + path_.string() + @@ -180,6 +194,8 @@ storeWriter::storeWriter(std::filesystem::path path, layout->pid = static_cast(::getpid()); layout->startTime = readProcessStartTime(layout->pid); layout->instance = instance; + layout->bucketCount = histogram_buckets.size(); + std::copy(histogram_buckets.begin(), histogram_buckets.end(), layout->bucketBounds); copyField(layout->agentName, MP_MAX_AGENT_NAME, agent_name, "agent name"); copyField(layout->hostname, MP_MAX_HOSTNAME, hostname, "hostname"); copyField(layout->localRank, MP_MAX_LOCAL_RANK, local_rank, "local_rank"); @@ -226,6 +242,25 @@ storeWriter::setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept touch(); } +void +storeWriter::observeHistogram(nixl_telemetry_event_type_t type, uint64_t value) noexcept { + const auto idx = static_cast(type); + if (idx >= MP_STORE_SLOT_COUNT) { + return; + } + auto *layout = static_cast(mapping_); + const double *const first = layout->bucketBounds; + const double *const last = first + layout->bucketCount; + // lower_bound, not upper_bound: Prometheus buckets are `value <= le`, so the + // observation belongs in the first bucket whose bound is not below it. Values + // above every bound land in the trailing overflow slot. + const double *const bound = std::lower_bound(first, last, static_cast(value)); + __atomic_fetch_add( + &layout->histBuckets[idx][static_cast(bound - first)], 1, __ATOMIC_RELAXED); + __atomic_fetch_add(&layout->histSums[idx], value, __ATOMIC_RELAXED); + touch(); +} + storeReadResult readStoreSnapshot(const std::filesystem::path &path) { const scopedFd fd(::open(path.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW)); @@ -269,10 +304,10 @@ readStoreSnapshot(const std::filesystem::path &path) { return {std::nullopt, true}; } if (layout->schemaVersion != MP_STORE_SCHEMA_VERSION || - layout->slotCount != MP_STORE_SLOT_COUNT) { + layout->slotCount != MP_STORE_SLOT_COUNT || layout->bucketCount > MP_STORE_MAX_BUCKETS) { NIXL_WARN << "prometheus_mp: ignoring telemetry store '" << path.string() << "' with incompatible schema (version " << layout->schemaVersion << ", slots " - << layout->slotCount << ")"; + << layout->slotCount << ", buckets " << layout->bucketCount << ")"; return {std::nullopt, true}; } @@ -284,9 +319,15 @@ readStoreSnapshot(const std::filesystem::path &path) { snap.agentName = readField(layout->agentName, MP_MAX_AGENT_NAME); snap.hostname = readField(layout->hostname, MP_MAX_HOSTNAME); snap.localRank = readField(layout->localRank, MP_MAX_LOCAL_RANK); + snap.bucketCount = static_cast(layout->bucketCount); + std::copy_n(layout->bucketBounds, snap.bucketCount, snap.bucketBounds.begin()); for (std::size_t i = 0; i < MP_STORE_SLOT_COUNT; ++i) { snap.counters[i] = __atomic_load_n(&layout->counters[i], __ATOMIC_RELAXED); snap.gauges[i] = __atomic_load_n(&layout->gauges[i], __ATOMIC_RELAXED); + snap.histSums[i] = __atomic_load_n(&layout->histSums[i], __ATOMIC_RELAXED); + for (std::size_t b = 0; b <= snap.bucketCount; ++b) { + snap.histBuckets[i][b] = __atomic_load_n(&layout->histBuckets[i][b], __ATOMIC_RELAXED); + } } return {std::move(snap), false}; diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 61d94f0d01..947f261151 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -26,6 +26,7 @@ #include #include #include +#include namespace nixl::telemetry::mp { @@ -47,6 +48,12 @@ inline constexpr std::string_view MP_STORE_FILE_SUFFIX = ".mmap"; inline constexpr std::size_t MP_STORE_SLOT_COUNT = static_cast(nixl_telemetry_event_type_t::AGENT_TELEMETRY_EVENTS_DROPPED) + 1; +// Most histogram bucket boundaries the fixed layout can hold (the built-in set +// uses 18). A NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US override with more bounds than +// this is rejected rather than silently truncated -- the single-process exporter, +// which keeps its buckets on the heap, has no such limit. +inline constexpr std::size_t MP_STORE_MAX_BUCKETS = 32; + namespace detail { // Highest slot index the collector will index into the counter/gauge arrays, // across every event type it publishes. @@ -95,6 +102,18 @@ struct storeSnapshot { std::string localRank; std::array counters{}; std::array gauges{}; + // Histogram bucket upper bounds this process was configured with; only the + // first bucketCount entries are meaningful. Carried per store because each + // process resolves them from its own environment. + uint32_t bucketCount = 0; + std::array bucketBounds{}; + // Per-bucket observation counts, indexed by nixl_telemetry_event_type_t then + // by bucket, where index bucketCount collects values above the last bound. + // Deliberately NOT cumulative: the reader accumulates, so a scrape racing a + // writer can never expose non-monotonic buckets. + std::array, MP_STORE_SLOT_COUNT> histBuckets{}; + // Sum of all observed values per event type, in the event's own units. + std::array histSums{}; }; /** @@ -116,13 +135,17 @@ class storeWriter { * @param local_rank Optional rank label; pass empty to omit it. * @param instance Per-process instance counter; disambiguates multiple * same-named agents in one process so their series stay distinct. - * @throws std::runtime_error on open/ftruncate/mmap failure. + * @param histogram_buckets Histogram bucket upper bounds, at most + * MP_STORE_MAX_BUCKETS of them. + * @throws std::runtime_error on open/ftruncate/mmap failure, or when + * @p histogram_buckets does not fit the store. */ storeWriter(std::filesystem::path path, const std::string &agent_name, const std::string &hostname, const std::string &local_rank, - uint64_t instance); + uint64_t instance, + const std::vector &histogram_buckets); ~storeWriter(); storeWriter(const storeWriter &) = delete; @@ -142,6 +165,12 @@ class storeWriter { void setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept; + // Records @p value in the histogram bucket it falls into (upper bounds are + // inclusive, matching Prometheus) and adds it to the sum for @p type, then + // refreshes the heartbeat. Out-of-range types are ignored. + void + observeHistogram(nixl_telemetry_event_type_t type, uint64_t value) noexcept; + [[nodiscard]] const std::filesystem::path & path() const noexcept { return path_; diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp index 7d158b975b..1365380e03 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -19,6 +19,7 @@ #include "common/configuration.h" #include "common/hostname.h" #include "common/nixl_log.h" +#include "histogram_buckets.h" #include @@ -112,7 +113,8 @@ nixlTelemetryPrometheusMpExporter::nixlTelemetryPrometheusMpExporter( init_params.agentName, nixl::getHostname().value_or("unknown"), resolveLocalRank(), - instance); + instance, + nixl::telemetry::resolveHistogramBucketsUs()); const bool local = nixl::config::getValueDefaulted(prometheusLocalVar, false); const uint16_t port = nixl::config::getValueDefaulted(prometheusPortVar, defaultPort); @@ -151,5 +153,8 @@ nixlTelemetryPrometheusMpExporter::exportEvent(const nixlTelemetryEvent &event) if (descriptor.gaugeName != nullptr) { store_->setGauge(type, event.value_); } + if (descriptor.histogramName != nullptr) { + store_->observeHistogram(type, event.value_); + } return NIXL_SUCCESS; } diff --git a/test/gtest/telemetry_mp_collector_test.cpp b/test/gtest/telemetry_mp_collector_test.cpp index cf806ce8ee..f24eac9baa 100644 --- a/test/gtest/telemetry_mp_collector_test.cpp +++ b/test/gtest/telemetry_mp_collector_test.cpp @@ -27,7 +27,9 @@ #include +#include #include +#include #include #include #include @@ -47,6 +49,9 @@ using nixl::telemetry::mp::readProcessStartTime; constexpr auto TX_BYTES = nixl_telemetry_event_type_t::AGENT_TX_BYTES; constexpr auto ERR_BACKEND = nixl_telemetry_event_type_t::AGENT_ERR_BACKEND; +constexpr auto XFER_TIME = nixl_telemetry_event_type_t::AGENT_XFER_TIME; + +const std::vector kBuckets = {10, 100, 1000}; [[nodiscard]] std::size_t idx(nixl_telemetry_event_type_t t) { @@ -62,6 +67,8 @@ makeSnap(const std::string &agent, const std::string &rank) { s.agentName = agent; s.hostname = "host"; s.localRank = rank; + s.bucketCount = static_cast(kBuckets.size()); + std::copy(kBuckets.begin(), kBuckets.end(), s.bucketBounds.begin()); return s; } @@ -210,6 +217,54 @@ TEST(MpCollectorTest, ErrorFamilyCarriesStatusLabel) { EXPECT_DOUBLE_EQ(canceled->counter.value, 0.0); } +TEST(MpCollectorTest, HistogramFamilyIsCumulativeAndEndsAtInfinity) { + auto a = makeSnap("agent-a", "0"); + // Per-bucket (non-cumulative) counts for bounds {10, 100, 1000} plus overflow. + a.histBuckets[idx(XFER_TIME)] = {1, 2, 3, 4}; + a.histSums[idx(XFER_TIME)] = 9999; + + const auto fams = buildMetricFamilies({a}); + const auto *hist = findFamily(fams, "agent_xfer_time_us"); + ASSERT_NE(hist, nullptr); + EXPECT_EQ(hist->type, prometheus::MetricType::Histogram); + ASSERT_EQ(hist->metric.size(), 1u); + + const auto &buckets = hist->metric[0].histogram.bucket; + ASSERT_EQ(buckets.size(), kBuckets.size() + 1); + EXPECT_DOUBLE_EQ(buckets[0].upper_bound, 10.0); + EXPECT_EQ(buckets[0].cumulative_count, 1u); + EXPECT_DOUBLE_EQ(buckets[1].upper_bound, 100.0); + EXPECT_EQ(buckets[1].cumulative_count, 3u); + EXPECT_DOUBLE_EQ(buckets[2].upper_bound, 1000.0); + EXPECT_EQ(buckets[2].cumulative_count, 6u); + EXPECT_TRUE(std::isinf(buckets[3].upper_bound)); + EXPECT_EQ(buckets[3].cumulative_count, 10u); + + EXPECT_EQ(hist->metric[0].histogram.sample_count, 10u); + EXPECT_DOUBLE_EQ(hist->metric[0].histogram.sample_sum, 9999.0); +} + +TEST(MpCollectorTest, HistogramsAreNotAggregatedAcrossProcesses) { + auto a = makeSnap("agent-a", "0"); + a.pid = 1001; + a.histBuckets[idx(XFER_TIME)] = {1, 0, 0, 0}; + auto b = makeSnap("agent-b", "1"); + b.pid = 1002; + b.histBuckets[idx(XFER_TIME)] = {0, 0, 0, 5}; + + const auto fams = buildMetricFamilies({a, b}); + const auto *hist = findFamily(fams, "agent_xfer_time_us"); + ASSERT_NE(hist, nullptr); + ASSERT_EQ(hist->metric.size(), 2u); + + const auto *m_a = findByLabel(*hist, "pid", "1001"); + const auto *m_b = findByLabel(*hist, "pid", "1002"); + ASSERT_NE(m_a, nullptr); + ASSERT_NE(m_b, nullptr); + EXPECT_EQ(m_a->histogram.sample_count, 1u); + EXPECT_EQ(m_b->histogram.sample_count, 5u); +} + TEST(MpCollectorTest, IsProcessAliveGuardsPidReuse) { EXPECT_TRUE(isProcessAlive(::getpid(), readProcessStartTime(::getpid()))); EXPECT_TRUE(isProcessAlive(::getpid(), 0)); // unknown start time -> existence only @@ -259,10 +314,10 @@ class MpCollectorFileTest : public ::testing::Test { TEST_F(MpCollectorFileTest, CollectReadsLiveStoresAndIgnoresOthers) { // Two distinct store files; both headers stamp this (live) process. - storeWriter w1(dir_ / makeStoreFileName(111, 1, 0), "agent-1", "host", "0", 0); + storeWriter w1(dir_ / makeStoreFileName(111, 1, 0), "agent-1", "host", "0", 0, kBuckets); w1.addCounter(TX_BYTES, 500); w1.setGauge(TX_BYTES, 500); - storeWriter w2(dir_ / makeStoreFileName(222, 2, 0), "agent-2", "host", "1", 0); + storeWriter w2(dir_ / makeStoreFileName(222, 2, 0), "agent-2", "host", "1", 0, kBuckets); w2.addCounter(TX_BYTES, 700); // A non-store file must be ignored. diff --git a/test/gtest/telemetry_mp_e2e_test.cpp b/test/gtest/telemetry_mp_e2e_test.cpp index eadeb35bd8..bc3da3b5b8 100644 --- a/test/gtest/telemetry_mp_e2e_test.cpp +++ b/test/gtest/telemetry_mp_e2e_test.cpp @@ -39,6 +39,7 @@ namespace { constexpr auto TX_BYTES = nixl_telemetry_event_type_t::AGENT_TX_BYTES; +constexpr auto XFER_TIME = nixl_telemetry_event_type_t::AGENT_XFER_TIME; [[nodiscard]] nixlTelemetryExporterInitParams initParams(const std::string &agent) { @@ -215,6 +216,7 @@ TEST_F(MpE2ETest, AllRankProcessesAggregateBehindOneEndpointAndStaleAreDropped) nixlTelemetryPrometheusMpExporter owner(initParams("agent-parent")); ASSERT_TRUE(owner.isExporter()); owner.exportEvent({TX_BYTES, 999}); + owner.exportEvent({XFER_TIME, 1234}); // Release the children (they now become writers) and wait for readiness. ::close(go_pipe[1]); @@ -224,13 +226,23 @@ TEST_F(MpE2ETest, AllRankProcessesAggregateBehindOneEndpointAndStaleAreDropped) } // Phase 1: every process must appear behind the single owner endpoint. - const auto phase1 = parseSeriesByAgent(scrapeMetrics(port_), "agent_tx_bytes_total"); + const auto body = scrapeMetrics(port_); + const auto phase1 = parseSeriesByAgent(body, "agent_tx_bytes_total"); EXPECT_EQ(phase1.size(), static_cast(kChildren + 1)); EXPECT_DOUBLE_EQ(phase1.at("agent-parent"), 999.0); EXPECT_DOUBLE_EQ(phase1.at("agent-0"), 100.0); EXPECT_DOUBLE_EQ(phase1.at("agent-1"), 200.0); EXPECT_DOUBLE_EQ(phase1.at("agent-2"), 300.0); + EXPECT_NE(body.find("# TYPE agent_xfer_time_us histogram"), std::string::npos); + EXPECT_NE(body.find("agent_xfer_time_us_bucket"), std::string::npos); + const auto hist_count = parseSeriesByAgent(body, "agent_xfer_time_us_count"); + const auto hist_sum = parseSeriesByAgent(body, "agent_xfer_time_us_sum"); + EXPECT_DOUBLE_EQ(hist_count.at("agent-parent"), 1.0); + EXPECT_DOUBLE_EQ(hist_sum.at("agent-parent"), 1234.0); + // Writers that observed nothing still expose the family, at zero. + EXPECT_DOUBLE_EQ(hist_count.at("agent-0"), 0.0); + // Kill one child and reap it so its pid is truly gone before the next scrape. ASSERT_EQ(::kill(children[0], SIGKILL), 0); int status = 0; diff --git a/test/gtest/telemetry_mp_exporter_test.cpp b/test/gtest/telemetry_mp_exporter_test.cpp index 8507ac6b57..9e250b51cf 100644 --- a/test/gtest/telemetry_mp_exporter_test.cpp +++ b/test/gtest/telemetry_mp_exporter_test.cpp @@ -15,6 +15,7 @@ * limitations under the License. */ #include "prometheus_mp_exporter.h" +#include "histogram_buckets.h" #include "mp_store.h" #include "telemetry.h" @@ -36,6 +37,7 @@ using nixl::telemetry::mp::readStoreSnapshot; constexpr auto TX_BYTES = nixl_telemetry_event_type_t::AGENT_TX_BYTES; constexpr auto RX_BYTES = nixl_telemetry_event_type_t::AGENT_RX_BYTES; +constexpr auto XFER_TIME = nixl_telemetry_event_type_t::AGENT_XFER_TIME; [[nodiscard]] std::size_t idx(nixl_telemetry_event_type_t t) { @@ -119,6 +121,26 @@ TEST_F(MpExporterTest, WriterModeWhenPortTaken) { EXPECT_EQ(snap->counters[idx(RX_BYTES)], 77u); } +TEST_F(MpExporterTest, DurationEventFeedsCounterGaugeAndHistogram) { + nixlTelemetryPrometheusMpExporter exporter(initParams("agent-hist")); + + const auto file = singleStoreFile(); + ASSERT_FALSE(file.empty()); + + exporter.exportEvent({XFER_TIME, 42}); + + const auto snap = readStoreSnapshot(file).snapshot; + ASSERT_TRUE(snap.has_value()); + EXPECT_EQ(snap->counters[idx(XFER_TIME)], 42u); + EXPECT_EQ(snap->gauges[idx(XFER_TIME)], 42u); + EXPECT_EQ(snap->histSums[idx(XFER_TIME)], 42u); + + // Default bounds start at 10us, so 42us lands in the 50us bucket (index 2). + ASSERT_EQ(snap->bucketCount, nixl::telemetry::defaultHistogramBucketsUs().size()); + EXPECT_DOUBLE_EQ(snap->bucketBounds[2], 50.0); + EXPECT_EQ(snap->histBuckets[idx(XFER_TIME)][2], 1u); +} + TEST_F(MpExporterTest, LoadsThroughPluginManager) { // The plugin manager probes every registered plugin directory, so it warns // about the ones that do not hold this plugin before finding the one that does. diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index 678ac28d9f..c3417455ec 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -25,7 +25,9 @@ #include #include #include +#include #include +#include namespace { @@ -36,6 +38,9 @@ using nixl::telemetry::mp::readStoreSnapshot; constexpr auto TX_BYTES = nixl_telemetry_event_type_t::AGENT_TX_BYTES; constexpr auto RX_BYTES = nixl_telemetry_event_type_t::AGENT_RX_BYTES; constexpr auto ERR_BACKEND = nixl_telemetry_event_type_t::AGENT_ERR_BACKEND; +constexpr auto XFER_TIME = nixl_telemetry_event_type_t::AGENT_XFER_TIME; + +const std::vector kBuckets = {10, 100, 1000}; [[nodiscard]] std::size_t idx(nixl_telemetry_event_type_t t) { @@ -68,7 +73,7 @@ class MpStoreTest : public ::testing::Test { TEST_F(MpStoreTest, WriteReadRoundTrip) { const auto path = storePath("agent-a"); - storeWriter writer(path, "agent-a", "host-1", "3", 7); + storeWriter writer(path, "agent-a", "host-1", "3", 7, kBuckets); writer.addCounter(TX_BYTES, 1000); writer.setGauge(TX_BYTES, 1000); writer.addCounter(ERR_BACKEND, 1); @@ -94,7 +99,7 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { const auto path = storePath("agent-b"); - storeWriter writer(path, "agent-b", "host-1", "", 0); + storeWriter writer(path, "agent-b", "host-1", "", 0, kBuckets); writer.addCounter(TX_BYTES, 100); writer.addCounter(TX_BYTES, 250); writer.addCounter(TX_BYTES, 650); @@ -109,17 +114,66 @@ TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { TEST_F(MpStoreTest, EmptyRankIsEmpty) { const auto path = storePath("agent-c"); - storeWriter writer(path, "agent-c", "host-1", "", 0); + storeWriter writer(path, "agent-c", "host-1", "", 0, kBuckets); const auto snap = readStoreSnapshot(path).snapshot; ASSERT_TRUE(snap.has_value()); EXPECT_TRUE(snap->localRank.empty()); } +TEST_F(MpStoreTest, HistogramBucketBoundsAreInclusive) { + const auto path = storePath("agent-hist"); + storeWriter writer(path, "agent-hist", "host-1", "", 0, kBuckets); + // Bounds {10, 100, 1000}: a value equal to a bound belongs to that bucket, + // and anything above the last bound lands in the trailing overflow slot. + writer.observeHistogram(XFER_TIME, 1); + writer.observeHistogram(XFER_TIME, 10); + writer.observeHistogram(XFER_TIME, 11); + writer.observeHistogram(XFER_TIME, 1000); + writer.observeHistogram(XFER_TIME, 1001); + + const auto snap = readStoreSnapshot(path).snapshot; + ASSERT_TRUE(snap.has_value()); + EXPECT_EQ(snap->bucketCount, kBuckets.size()); + EXPECT_DOUBLE_EQ(snap->bucketBounds[0], 10.0); + EXPECT_DOUBLE_EQ(snap->bucketBounds[2], 1000.0); + + const auto &counts = snap->histBuckets[idx(XFER_TIME)]; + EXPECT_EQ(counts[0], 2u); // 1, 10 + EXPECT_EQ(counts[1], 1u); // 11 + EXPECT_EQ(counts[2], 1u); // 1000 + EXPECT_EQ(counts[3], 1u); // 1001 -> overflow + EXPECT_EQ(snap->histSums[idx(XFER_TIME)], 1u + 10u + 11u + 1000u + 1001u); +} + +TEST_F(MpStoreTest, HistogramUntouchedTypesStayEmpty) { + const auto path = storePath("agent-hist-empty"); + storeWriter writer(path, "agent-hist-empty", "host-1", "", 0, kBuckets); + writer.observeHistogram(XFER_TIME, 5); + + const auto snap = readStoreSnapshot(path).snapshot; + ASSERT_TRUE(snap.has_value()); + EXPECT_EQ(snap->histSums[idx(TX_BYTES)], 0u); + for (const auto count : snap->histBuckets[idx(TX_BYTES)]) { + EXPECT_EQ(count, 0u); + } +} + +TEST_F(MpStoreTest, TooManyBucketsRejected) { + using nixl::telemetry::mp::MP_STORE_MAX_BUCKETS; + std::vector too_many(MP_STORE_MAX_BUCKETS + 1); + for (std::size_t i = 0; i < too_many.size(); ++i) { + too_many[i] = static_cast(i + 1); + } + const auto path = storePath("agent-too-many"); + EXPECT_THROW({ storeWriter writer(path, "a", "host-1", "", 0, too_many); }, std::runtime_error); + EXPECT_FALSE(std::filesystem::exists(path)); +} + TEST_F(MpStoreTest, DestructorRemovesStoreFile) { const auto path = storePath("agent-cleanup"); { - storeWriter writer(path, "agent-cleanup", "host-1", "", 0); + storeWriter writer(path, "agent-cleanup", "host-1", "", 0, kBuckets); EXPECT_TRUE(std::filesystem::exists(path)); } EXPECT_FALSE(std::filesystem::exists(path)); @@ -129,7 +183,7 @@ TEST_F(MpStoreTest, LongAgentNameTruncated) { const auto path = storePath("agent-long"); const std::string long_name(1000, 'x'); const gtest::LogIgnoreGuard lig("exceeds 255 chars"); - storeWriter writer(path, long_name, "host-1", "", 0); + storeWriter writer(path, long_name, "host-1", "", 0, kBuckets); const auto snap = readStoreSnapshot(path).snapshot; ASSERT_TRUE(snap.has_value()); From 6d0f506b9f1bc0adc4d9ecdc84bc24778e750b61 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 18:29:50 +0000 Subject: [PATCH 27/71] telemetry: fix prometheus_mp plugin-loader test, document owner death MpExporterTest.LoadsThroughPluginManager never registered the build-tree plugin directory, so it only passed where the plugin already sat on NIXL_PLUGIN_DIR. Running the gtest binary from a build tree -- as test_sanitizer.sh does, since it invokes meson test before exporting NIXL_PLUGIN_DIR -- failed with "Failed to load telemetry plugin". Register the build path once per suite, as the sibling prometheus fixture does, and only when it exists so a run from an install tree logs no error. Also document that the scrape owner is elected once and never fails over: if it exits, the endpoint stays down for the rest of the run while the surviving ranks keep recording, and reaping stops with it. Signed-off-by: Efraim Eygin --- docs/telemetry.md | 11 +++++++++-- src/plugins/telemetry/prometheus_mp/README.md | 9 +++++++++ test/gtest/telemetry_mp_exporter_test.cpp | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/docs/telemetry.md b/docs/telemetry.md index 14cc7d3564..19a1ceba04 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -199,8 +199,15 @@ names are not stored) and per-process label values are captured once at startup never change -- events carry only a numeric value, with no per-observation labels. It therefore **cannot represent a metric with a dynamic / high-cardinality label** that varies per observation. No NIXL metric needs that today; if one is ever added, -a different (keyed) store would be required. For aggregation via an external -service, use the DOCA/CollectX exporter instead. +a different (keyed) store would be required. + +The scrape owner is also elected once, at startup, with **no failover**: if that +process exits, the endpoint stays down for the rest of the run even though the +surviving ranks keep recording, and stale store files stop being reaped. Scraping +resumes only when a process starts and wins the bind, or the run is relaunched. +Alert on the scrape target's `up` metric rather than on missing series. + +For aggregation via an external service, use the DOCA/CollectX exporter instead. ## Cyclic Buffer diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index e9d4cb50e2..82d77593fe 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -50,6 +50,15 @@ Same as the `prometheus` plug-in: the bundled prometheus-cpp subproject and it instead crashes or is killed -- and so cannot clean up after itself -- the owner drops its series once the process is gone (verified by pid + `/proc` start time) or its data ages past the TTL, and reaps the file. +- **The owner is elected once, and there is no failover.** If the owner process + exits, no surviving writer promotes itself: the endpoint stays down for the rest + of the run while every remaining process keeps updating its store file. Reaping + stops with it, since the owner was the reaper -- a killed owner leaves its own + file behind. Scraping resumes only when some process starts and wins the bind + (a restarted rank), or the process family is relaunched. Because this looks to + Prometheus like the target going down rather than the ranks going idle, alert on + the target's `up` metric, not on absent series. Automatic failover is deliberately + out of scope for now. ## Configuration diff --git a/test/gtest/telemetry_mp_exporter_test.cpp b/test/gtest/telemetry_mp_exporter_test.cpp index 9e250b51cf..0041120107 100644 --- a/test/gtest/telemetry_mp_exporter_test.cpp +++ b/test/gtest/telemetry_mp_exporter_test.cpp @@ -17,6 +17,7 @@ #include "prometheus_mp_exporter.h" #include "histogram_buckets.h" #include "mp_store.h" +#include "plugin_manager.h" #include "telemetry.h" #include "common.h" @@ -51,6 +52,21 @@ initParams(const std::string &agent) { class MpExporterTest : public ::testing::Test { protected: + // A build tree has no /plugins, so LoadsThroughPluginManager + // finds the plugin only if the build path is registered. Registered once per + // suite, and only when it exists: re-registering, or registering a missing + // directory, logs a warning/error that the gtest main counts as a failure. + // When it is absent (a binary run from an install tree) NIXL_PLUGIN_DIR is + // what supplies the plugin. + static void + SetUpTestSuite() { + const std::string build_plugin_dir = + std::string(BUILD_DIR) + "/src/plugins/telemetry/prometheus_mp"; + if (std::filesystem::is_directory(build_plugin_dir)) { + nixlPluginManager::getInstance().addPluginDirectory(build_plugin_dir); + } + } + void SetUp() override { const auto *info = ::testing::UnitTest::GetInstance()->current_test_info(); From fb0a3292bd018bd68b987cf9966dca1e54a81026 Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 18:59:21 +0000 Subject: [PATCH 28/71] telemetry: cut redundant clock reads and zero loads in prometheus_mp Every storeWriter mutator ended with a heartbeat refresh, i.e. a CLOCK_MONOTONIC read measured at ~25ns here -- several times the atomic it accompanied. A completed transfer emits four events that fan out to nine mutator calls (bytes: counter+gauge, requests_num: counter, and each of the two duration events: counter+gauge+histogram), so it paid nine clock reads to maintain a field whose only consumer is a staleness check with a 30s default TTL. Restore refreshHeartbeat() as explicit API and call it once per exportEvent, taking nine reads per transfer down to four. This runs on the flush thread draining the staging queue, so a cheaper drain loop also widens the margin before AGENT_TELEMETRY_EVENTS_DROPPED starts counting. Fewer refreshes cannot hide a live process: the TTL gates dead processes only, since a live one is published on kill(pid, 0) + /proc start time regardless of its heartbeat. It only shortens how long a dead process's last values linger. readStoreSnapshot also loaded histogram buckets and sums for all 21 slots when only two event types carry histograms, moving 420 words per store per scrape where 40 suffice. Derive the histogram-bearing slots from the shared descriptor at compile time so the reader skips the rest and any future histogram is picked up automatically. Signed-off-by: Efraim Eygin --- .../telemetry/prometheus_mp/mp_store.cpp | 7 +-- .../telemetry/prometheus_mp/mp_store.h | 56 ++++++++++++++++--- .../prometheus_mp/prometheus_mp_exporter.cpp | 3 + test/gtest/telemetry_mp_exporter_test.cpp | 18 ++++++ test/gtest/telemetry_mp_store_test.cpp | 26 +++++++++ 5 files changed, 97 insertions(+), 13 deletions(-) diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.cpp b/src/plugins/telemetry/prometheus_mp/mp_store.cpp index cb0b99d373..8ff68e7b44 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.cpp +++ b/src/plugins/telemetry/prometheus_mp/mp_store.cpp @@ -215,7 +215,7 @@ storeWriter::~storeWriter() { } void -storeWriter::touch() noexcept { +storeWriter::refreshHeartbeat() noexcept { auto *layout = static_cast(mapping_); __atomic_store_n(&layout->lastUpdateNs, nixlTime::getNs(), __ATOMIC_RELAXED); } @@ -228,7 +228,6 @@ storeWriter::addCounter(nixl_telemetry_event_type_t type, uint64_t delta) noexce } auto *layout = static_cast(mapping_); __atomic_fetch_add(&layout->counters[idx], delta, __ATOMIC_RELAXED); - touch(); } void @@ -239,7 +238,6 @@ storeWriter::setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept } auto *layout = static_cast(mapping_); __atomic_store_n(&layout->gauges[idx], value, __ATOMIC_RELAXED); - touch(); } void @@ -258,7 +256,6 @@ storeWriter::observeHistogram(nixl_telemetry_event_type_t type, uint64_t value) __atomic_fetch_add( &layout->histBuckets[idx][static_cast(bound - first)], 1, __ATOMIC_RELAXED); __atomic_fetch_add(&layout->histSums[idx], value, __ATOMIC_RELAXED); - touch(); } storeReadResult @@ -324,6 +321,8 @@ readStoreSnapshot(const std::filesystem::path &path) { for (std::size_t i = 0; i < MP_STORE_SLOT_COUNT; ++i) { snap.counters[i] = __atomic_load_n(&layout->counters[i], __ATOMIC_RELAXED); snap.gauges[i] = __atomic_load_n(&layout->gauges[i], __ATOMIC_RELAXED); + } + for (const auto i : MP_STORE_HISTOGRAM_SLOTS) { snap.histSums[i] = __atomic_load_n(&layout->histSums[i], __ATOMIC_RELAXED); for (std::size_t b = 0; b <= snap.bucketCount; ++b) { snap.histBuckets[i][b] = __atomic_load_n(&layout->histBuckets[i][b], __ATOMIC_RELAXED); diff --git a/src/plugins/telemetry/prometheus_mp/mp_store.h b/src/plugins/telemetry/prometheus_mp/mp_store.h index 947f261151..8f50b1704a 100644 --- a/src/plugins/telemetry/prometheus_mp/mp_store.h +++ b/src/plugins/telemetry/prometheus_mp/mp_store.h @@ -68,8 +68,36 @@ namespace detail { } return max_slot; } + + [[nodiscard]] constexpr std::size_t + histogramSlotCount() { + std::size_t count = 0; + for (const auto type : telemetry_metric_event_types) { + if (nixlEnumStrings::telemetryMetricDescriptor(type).histogramName != nullptr) { + ++count; + } + } + return count; + } + + [[nodiscard]] constexpr std::array + histogramSlots() { + std::array slots{}; + std::size_t next = 0; + for (const auto type : telemetry_metric_event_types) { + if (nixlEnumStrings::telemetryMetricDescriptor(type).histogramName != nullptr) { + slots[next++] = static_cast(type); + } + } + return slots; + } } // namespace detail +// The slots that can ever hold histogram data, derived at compile time from the +// shared descriptor. Every other slot is provably zero, so the reader skips it +// instead of loading MP_STORE_MAX_BUCKETS + 2 words per slot. +inline constexpr auto MP_STORE_HISTOGRAM_SLOTS = detail::histogramSlots(); + // Compile-time guard: if the enum is extended past AGENT_TELEMETRY_EVENTS_DROPPED // (so it is no longer last), the fixed-slot store would be indexed out of bounds // by the collector. Fail the build instead, forcing MP_STORE_SLOT_COUNT to be @@ -155,31 +183,41 @@ class storeWriter { storeWriter & operator=(storeWriter &&) = delete; - // Adds @p delta to the cumulative counter slot for @p type and refreshes the - // heartbeat. Out-of-range types are ignored. + // Adds @p delta to the cumulative counter slot for @p type. Out-of-range + // types are ignored. void addCounter(nixl_telemetry_event_type_t type, uint64_t delta) noexcept; - // Stores @p value as the last-operation gauge for @p type and refreshes the - // heartbeat. Out-of-range types are ignored. + // Stores @p value as the last-operation gauge for @p type. Out-of-range + // types are ignored. void setGauge(nixl_telemetry_event_type_t type, uint64_t value) noexcept; // Records @p value in the histogram bucket it falls into (upper bounds are - // inclusive, matching Prometheus) and adds it to the sum for @p type, then - // refreshes the heartbeat. Out-of-range types are ignored. + // inclusive, matching Prometheus) and adds it to the sum for @p type. + // Out-of-range types are ignored. void observeHistogram(nixl_telemetry_event_type_t type, uint64_t value) noexcept; + /** + * @brief Republishes this process's liveness timestamp. + * + * Deliberately separate from the mutators: it costs a CLOCK_MONOTONIC read + * (~24ns, several times a metric update's atomic), and the only consumer is + * the collector's staleness check, which has a 30s default TTL and applies + * to dead processes only. Call it once per batch of updates rather than per + * update; missing a refresh never hides a live process, it only shortens how + * long a dead one's last values linger. + */ + void + refreshHeartbeat() noexcept; + [[nodiscard]] const std::filesystem::path & path() const noexcept { return path_; } private: - void - touch() noexcept; - std::filesystem::path path_; void *mapping_ = nullptr; std::size_t mappingSize_ = 0; diff --git a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp index 1365380e03..a38681b7b0 100644 --- a/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp +++ b/src/plugins/telemetry/prometheus_mp/prometheus_mp_exporter.cpp @@ -156,5 +156,8 @@ nixlTelemetryPrometheusMpExporter::exportEvent(const nixlTelemetryEvent &event) if (descriptor.histogramName != nullptr) { store_->observeHistogram(type, event.value_); } + // Once per event, not once per slot updated: a duration event touches three + // slots, and the clock read costs several times the atomics it would follow. + store_->refreshHeartbeat(); return NIXL_SUCCESS; } diff --git a/test/gtest/telemetry_mp_exporter_test.cpp b/test/gtest/telemetry_mp_exporter_test.cpp index 0041120107..3c391340bb 100644 --- a/test/gtest/telemetry_mp_exporter_test.cpp +++ b/test/gtest/telemetry_mp_exporter_test.cpp @@ -28,9 +28,11 @@ #include +#include #include #include #include +#include namespace { @@ -157,6 +159,22 @@ TEST_F(MpExporterTest, DurationEventFeedsCounterGaugeAndHistogram) { EXPECT_EQ(snap->histBuckets[idx(XFER_TIME)][2], 1u); } +TEST_F(MpExporterTest, ExportEventRefreshesHeartbeat) { + nixlTelemetryPrometheusMpExporter exporter(initParams("agent-beat")); + + const auto file = singleStoreFile(); + ASSERT_FALSE(file.empty()); + const auto before = readStoreSnapshot(file).snapshot; + ASSERT_TRUE(before.has_value()); + + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + exporter.exportEvent({TX_BYTES, 1}); + + const auto after = readStoreSnapshot(file).snapshot; + ASSERT_TRUE(after.has_value()); + EXPECT_GT(after->lastUpdateNs, before->lastUpdateNs); +} + TEST_F(MpExporterTest, LoadsThroughPluginManager) { // The plugin manager probes every registered plugin directory, so it warns // about the ones that do not hold this plugin before finding the one that does. diff --git a/test/gtest/telemetry_mp_store_test.cpp b/test/gtest/telemetry_mp_store_test.cpp index c3417455ec..0314090e80 100644 --- a/test/gtest/telemetry_mp_store_test.cpp +++ b/test/gtest/telemetry_mp_store_test.cpp @@ -22,11 +22,13 @@ #include +#include #include #include #include #include #include +#include #include namespace { @@ -97,6 +99,30 @@ TEST_F(MpStoreTest, WriteReadRoundTrip) { EXPECT_EQ(snap->gauges[idx(RX_BYTES)], 0u); } +TEST_F(MpStoreTest, HeartbeatAdvancesOnlyWhenRefreshed) { + const auto path = storePath("agent-heartbeat"); + storeWriter writer(path, "agent-heartbeat", "host-1", "", 0, kBuckets); + + const auto created = readStoreSnapshot(path).snapshot; + ASSERT_TRUE(created.has_value()); + ASSERT_GT(created->lastUpdateNs, 0u); + + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + writer.addCounter(TX_BYTES, 1); + writer.setGauge(TX_BYTES, 1); + writer.observeHistogram(XFER_TIME, 1); + + const auto after_updates = readStoreSnapshot(path).snapshot; + ASSERT_TRUE(after_updates.has_value()); + EXPECT_EQ(after_updates->lastUpdateNs, created->lastUpdateNs); + + writer.refreshHeartbeat(); + + const auto after_refresh = readStoreSnapshot(path).snapshot; + ASSERT_TRUE(after_refresh.has_value()); + EXPECT_GT(after_refresh->lastUpdateNs, created->lastUpdateNs); +} + TEST_F(MpStoreTest, CounterAccumulatesGaugeReplaces) { const auto path = storePath("agent-b"); storeWriter writer(path, "agent-b", "host-1", "", 0, kBuckets); From 9f18292657cdced6f55daa6401e4a8c41292217b Mon Sep 17 00:00:00 2001 From: Efraim Eygin Date: Sat, 25 Jul 2026 18:59:51 +0000 Subject: [PATCH 29/71] telemetry: harden the prometheus_mp e2e test and fix doc/header nits The e2e test forks three writer children and waits on a readiness pipe with a bare read(), so a child that stays alive without signalling hangs the test instead of failing it. Bound that wait with poll(). Its cleanup also lived in the test body, where a fatal assertion after fork() skips it and leaves the children running against the store directory for the rest of the binary; move the child pids and the parent-side pipe ends into the fixture so TearDown() reaps and closes them on every exit path. Register the build-tree plugin directory once per process rather than once per test suite: --gtest_repeat re-enters SetUpTestSuite and the second registration logs a warning that the gtest main counts as a failure. Also list prometheus_mp among the consumers of NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US, and use a single copyright year on the files this PR adds. Signed-off-by: Efraim Eygin --- docs/telemetry.md | 2 +- src/plugins/telemetry/prometheus_mp/README.md | 2 +- .../telemetry/prometheus_mp/meson.build | 2 +- .../telemetry/prometheus_mp/mp_collector.cpp | 2 +- .../telemetry/prometheus_mp/mp_collector.h | 2 +- .../telemetry/prometheus_mp/mp_store.cpp | 2 +- .../telemetry/prometheus_mp/mp_store.h | 2 +- .../prometheus_mp/prometheus_mp_exporter.cpp | 2 +- .../prometheus_mp/prometheus_mp_exporter.h | 2 +- .../prometheus_mp/prometheus_mp_plugin.cpp | 2 +- src/utils/common/hostname.h | 2 +- test/gtest/telemetry_mp_collector_test.cpp | 2 +- test/gtest/telemetry_mp_e2e_test.cpp | 50 +++++++++++++------ test/gtest/telemetry_mp_exporter_test.cpp | 23 +++++---- test/gtest/telemetry_mp_store_test.cpp | 2 +- 15 files changed, 62 insertions(+), 37 deletions(-) diff --git a/docs/telemetry.md b/docs/telemetry.md index 19a1ceba04..8dbf66d679 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -142,7 +142,7 @@ Telemetry is configured by environment variables: | `NIXL_TELEMETRY_BUFFER_SIZE` | Number of events in buffer | `4096` | | `NIXL_TELEMETRY_RUN_INTERVAL` | Flush interval (ms) | `100` | | `NIXL_TELEMETRY_EXPORTER` | Name of the exporter plugin to use | - | -| `NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US` | Comma-separated microsecond bucket bounds for the transfer-time histograms (Prometheus/DOCA) | built-in µs defaults | +| `NIXL_TELEMETRY_HISTOGRAM_BUCKETS_US` | Comma-separated microsecond bucket bounds for the transfer-time histograms (`prometheus`, `prometheus_mp`, DOCA) | built-in µs defaults | | `NIXL_TELEMETRY_ENABLED_METRICS` | Comma-separated allowlist of metric names to export (glob) | all | - `NIXL_TELEMETRY_ENABLE` can be set to `y`/`yes`/`on`/`true`/`enable`/`1` to be enabled, and `n`/`no`/`off`/`false`/`disable`/`0` (or not set) to be disabled. Matching is case insensitive. diff --git a/src/plugins/telemetry/prometheus_mp/README.md b/src/plugins/telemetry/prometheus_mp/README.md index 82d77593fe..499763f4ae 100644 --- a/src/plugins/telemetry/prometheus_mp/README.md +++ b/src/plugins/telemetry/prometheus_mp/README.md @@ -1,5 +1,5 @@