profiler/inspector: report ring buffer drops instead of losing data silently

[shaq918]

Problem

The Inspector plugin stores completed collective/P2P records in a fixed-size
per-communicator ring buffer (NCCL_INSPECTOR_DUMP_COLL_RING_SIZE, default
1024) that a background thread drains every
NCCL_INSPECTOR_DUMP_THREAD_INTERVAL_MICROSECONDS. When operations complete
faster than the ring is drained, inspectorRingEnqueue overwrites the oldest
unread entry — silently. Nothing is logged, no counter is incremented, and
nothing in the output indicates that data is missing, so downstream consumers
treat a biased sample as complete data.

This is easy to hit in practice. On a 2-node × 8 H100 run of nccl-tests
all_reduce_perf looping 256 MB messages (~770 collectives/s against 1024
ring slots per 3 s dump interval), the JSON output captured only 48.4% of
collectives: 5,825,536 records out of a 12,036,596 coll_sn span across 16
ranks (6,211,060 lost). Per-rank record counts were exact multiples of 1024
(355×1024 / 356×1024) — the ring-overwrite fingerprint. Prometheus mode reads
the same ring, so its windowed aggregates are computed over the same biased
sample with no indication either.

Relatedly, the per-window operation count (agg.count) was already computed
for the exec-time mean in Prometheus mode but never emitted, so there was no
rate signal to notice the loss (or to use for straggler detection).

Solution

Count the overwrites and surface them in every output mode, without changing
behavior for jobs that never overflow:

• inspectorCompletedRing gains dropped (cumulative) and droppedReported
  (snapshot at last drain). The enqueue path increments dropped in the
  existing overwrite branch — one increment under the already-held
  per-communicator write lock; no new locks or allocations.
• JSON: metadata gains dropped_total and dropped_since_last_dump
  (snapshotted under the same guard as the drain). Format version bumped
  v4.0 → v4.1.
• Prometheus: new metrics
  • nccl_collective_count / nccl_p2p_count — per-bucket operation count
    per dump window (the previously computed-but-unemitted agg.count);
  • nccl_collectives_dropped_total / nccl_p2p_dropped_total — cumulative
    drops with device-level labels (version, slurm_job_id, node, gpu),
    summed across the device's communicators.
    Format minor bumped → version="v5.2".
• One-time warning (per process) via the existing logging macros on first
  overflow, suggesting a larger ring or shorter dump interval.

Design notes: the drop counters are kept device-level in Prometheus mode to
avoid adding per-bucket cardinality; per-communicator detail is available in
JSON mode. Summing cumulative per-communicator counters can decrease if a
communicator is destroyed mid-job; in practice communicators outlive the
scrape windows and the counter is monotonic.

Validation

2 nodes × 8 H100, Slurm, nccl-tests all_reduce_perf, plugin built with the
standalone Makefile (gcc -Wall -Wextra, no warnings).

1. Low-rate control (1 GB messages, ~100 colls/s « ring capacity):
   dropped_total = 0 on all 16 ranks, coll_sn gap-free (span == records,
   e.g. 1826/1826) — no behavior change on the non-overflow path, and no
   warning logged.
2. Overflow run (256 MB looping, ~770 colls/s, 3 s interval, ~5 min,
   ~500k collectives/rank): on 15 of 16 ranks the final dropped_total
   equals the ground truth computed from the output itself,
   (max(coll_sn) + 1) − records_written, exactly — e.g. rank 0:
   222,208 records, max coll_sn 499,750 → 277,543 expected, 277,543
   reported. The 16th rank differed by exactly 1 record out of ~500k,
   consistent with a single event discarded by the default
   NCCL_INSPECTOR_REQUIRE_KERNEL_TIMING filter (which consumes a sequence
   number without entering the ring). On every rank the
   dropped_since_last_dump values across dumps sum to dropped_total.
   The overflow warning appeared exactly once per process (16 total).
   Notably, each rank's first ~770–800 records were overwritten before the
   first drain ever ran (first surviving coll_sn ≈ 770), so simple
   span-based gap analysis undercounts the loss — the counter sees it.
3. Prometheus spot-check (same workload, 30 s interval), sampled
   mid-run: nccl_collective_count = 1024 (exactly ring size per window
   while overflowing), nccl_collectives_dropped_total = 87,841 and
   growing, nccl_p2p_dropped_total = 0, all under version="v5.2".

Limitations

• Drop counts identify how many records were lost, not which windows they
  came from within a dump interval.
• In Prometheus mode drops are attributed to the device, not to individual
  communicators/buckets (deliberate, to bound label cardinality).

[xiaofanl-nvidia]

++ @rishdas can you take a look? Please let me know if we want to take in this contribution.

[rishdas]

First, thank you for the contribution, I think this is good and essential change needed in Inspector.
Overall I agree with the change and think it should be accepted after suggested changes.
Few suggestions in these lines :

• The JSON metadata is written per record, not per dump so I just lets add a new record and report all the dump stats in that record. Its like marker for the beginning of dump cycle. (in the stats we should coll and p2p record drops as seperate stats)
• I would suggest renaming nccl_collective_count and nccl_p2p_count and instead of placing it per record report it once per dump along with dropped stats. ALso flag protect this feature with default disabled.

[shaq918]

Good call — I'll extract this into inspectorCommInfoUpdateDropStats(...) so the collective and P2P paths share it. Thanks!

[rishdas]

Thanks

[rishdas]

Instead of having this printed for every JSON record, I was wondering of we can print single record per dump and call dump stats and have dropped + even print record stats, make this feature more complete.

[shaq918]

Agreed, stamping this on every record is wasteful. I'll emit a dedicated stats record once per dump carrying the drop counts and the records-written count, so the capture ratio is readable in one place. Since the counters are per-communicator, I'm planning one stats record per comm per dump rather than a single global one, to preserve the per-comm attribution JSON mode has today — let me know if you'd rather it be aggregated.

One small thing I'll be careful about: a separate record type means a consumer filtering on coll_perf lines would skip it, which is a faint echo of the very bug we're fixing — so I'll name and document the record to make it hard to miss.

[rishdas]

Per comm dump sounds better.

[rishdas]

Same comments as for the collective counterparts.

[shaq918]

Will apply the same per-dump stats-record treatment to the P2P path.

[shaq918]

Happy to change it. One quick check so I stay consistent: the existing messages in the plugin use the "NCCL Inspector:" prefix (e.g. the pool init/exhaustion logs). Do you want just this line changed to "NCCL Inspector Profiler Plugin:", or is there a broader prefix rename you'd like? Just want to avoid introducing a third variant.

[rishdas]

Actually I take back what I said its better to stay consistent ignore this comment.

[rishdas]

Lets flag protect this via env variable and have it as optional.
Rationale, Prometheus metrics emitted need to be picked by external exporters, adding new metrics would have implications on metrics storage footprint as usally metrics have their own quota.

[rishdas]

NCCL_INSPECTOR_PROM_DUMP_VERBOSE

[rishdas]

Actually I would suggest to not have this metric here at all
"nccl_collective_count / nccl_p2p_count are the wrong shape. These are per-window gauges named with the _count suffix Prometheus reserves for histogram/summary components. Worse, a per-window gauge is only correct when the scrape interval exactly matches the dump interval — a double scrape counts the window twice, a missed scrape loses it. A cumulative per device write counter (nccl_collectives_total) would make rate()
work and is strictly more useful."

[shaq918]

Makes sense — the footprint/quota concern is fair for anything that adds cardinality to an exporter people already scrape. I'll gate this behind an env var, default off. (Combined with your other note on this line about the metric shape, it'll become a cumulative counter that's opt-in.)

[shaq918]

Got it — I'll use NCCL_INSPECTOR_PROM_DUMP_VERBOSE for the per-window / verbose metrics.

[shaq918]

You're completely right, and thank you for this — it's the comment I'm most grateful for. I'll switch to a cumulative monotonic counter (e.g. nccl_collectives_total) so rate()/increase() handle the windowing at query time and it stays correct under scrape jitter, double scrapes, or a missed scrape — rather than the per-window gauge with the _count suffix. As a nice side effect, rate(dropped) / rate(total) then gives a clean capture-ratio SLI.

[rishdas]

Sounds good thank you

[rishdas]

Same comments as above for Collective.

[shaq918]

Will gate the P2P equivalent the same way.

[rishdas]

Lets flag protect this metric by and you can make it more verbose if you want like per comminicator and stuff.
NCCL_INSPECTOR_PROM_DUMP_STATS

[shaq918]

Will do — I'll add NCCL_INSPECTOR_PROM_DUMP_STATS and can make it per-communicator when verbose.

One thing I'd love your read on for this specific counter. Since the premise of the change is that overflow is silent today, I'd like to keep the JSON dropped_total and the one-time WARN on by default, so the "you're losing data" signal is present out of the box — the WARN is one line per process lifetime, and the device-level dropped counter is a single series per GPU ({node, gpu}), which is tiny next to the existing default-on per-bucket metrics (those already multiply by collective × message_size × algo_proto).

So my proposal would be: JSON dropped_total + WARN default-on, and all the richer Prometheus emission (this metric included) opt-in via the flag. If you'd still prefer this counter flag-gated too, I'll happily do it — the WARN + JSON evidence still cover the silent-loss case — I just wanted to make the case before flipping the default.

[rishdas]

My take is JSON you can keep at as verbose as you want, you dont have to curb anything, I will leave that call upto you.
I envision JSON mode for Ninja User and aware of all the system level impications and Probably can modify code if needs be.
Prometheus Mode is for regular high level sys admin wanting to get handle on NCCL perf for their workload by plumbing the data to dashbaord.
So the default version should be minimalistic version any extra metrics or stats can be turned on and off by runtime knob.

Coming to the WARN, currently default make file translates WARN to Info because other upstream systems look at WARN level messages hence we are carefully on what NCCL puts as WARN.
You can keep it as WARN but in your org if you want to use be midful you will have to compile it with this flag NCCL_INSPECTOR_ENABLE_WARN to actually have it as WARn in running system.
I am fine with this proposal
"So my proposal would be: JSON dropped_total + WARN default-on, and all the richer Prometheus emission (this metric included) opt-in via the flag"

[rishdas]

I would refrain from this suggestion on NCCL_INSPECTOR_DUMP_THREAD_INTERVAL_MICROSECONDS as its bit nuanced especially in Prometheus Textfile collector mode where you can cant drop below a certain value because of the exporter properties.

[shaq918]

Agreed — I'll drop the NCCL_INSPECTOR_DUMP_THREAD_INTERVAL_MICROSECONDS suggestion from the warning. You're right that it's misleading in Prometheus textfile-collector mode, where the interval can't go below the exporter's floor. I'll point at the ring-size knob and the docs instead.

[rishdas]

Revised draft:

Thanks for the rework — the dump_stats record and the move to cumulative counters
are both what I was after. I've answered the open questions inline in the individual
threads; consolidating here since the last push (9905b65) predates those answers.

Decisions, in one place

• JSON dump_stats: keep it per-communicator as implemented. No gating needed in
  JSON mode — I see JSON as the ninja-user path, so be as verbose there as is useful.
• Log prefix nit: withdrawn. Stay consistent with the existing NCCL Inspector:
  prefix.
• One-time WARN_INSPECTOR on first overflow: fine as-is, default-on. Note the
  default build maps it to INFO; you'll need to compile with
  NCCL_INSPECTOR_ENABLE_WARN=1 to get a real WARN in your deployment. We keep NCCL's
  WARN level conservative because upstream systems alert on it.
• Prometheus: the default output stays minimalistic. It's the sysadmin-facing path,
  scraped into dashboards under a metrics quota, so any extra metric or stat goes behind
  a runtime knob.

Must fix before merge

1. Please rebase onto — or retarget this PR at — the dev branch. Inspector work
   lands there first, and dev has already moved under you: it carries a pure refactor
   that passes inspectorPromDevice& into inspectorPromCommInfoDumpColl() /
   ...DumpP2p() / ...CommInfoDump() instead of the bucket maps, which is the same
   plumbing change your patch makes. Rebasing should mostly mean dropping those hunks.
   inspector_json.cc, inspector_plugin.cc and inspector_ring.{cc,h} apply cleanly
   as-is; only inspector_prom.cc and README.md need rework.
2. There is a third output mode on dev: inspector_otel.cc (OTLP export). It
   drains completedCollRing / completedP2pRing through the same
   inspectorRingDrain() calls, so it silently loses records exactly like the other
   two. Either extend the drop reporting there as well, or scope it out explicitly — but
   the PR description's "surface them in every output mode" should match whatever you
   choose.
3. Gate the new Prometheus metrics behind NCCL_INSPECTOR_PROM_DUMP_STATS, default
   0. Today all four (nccl_collectives_total, nccl_collectives_dropped_total,
   nccl_p2p_total, nccl_p2p_dropped_total) are unconditional, because
   inspectorPromWriteDeviceTotals() is called unconditionally from
   inspectorPromWriteDeviceBuckets(). NCCL_INSPECTOR_OTEL_VERBOSE on dev is a good
   precedent for the shape of this. Please also register the knob in the env-var table in
   inspector.cc alongside NCCL_INSPECTOR_PROM_DUMP, add the matching parse block, and
   document it in the README knob list. A per-communicator breakdown under the same flag
   is fine if you want it.
4. README no longer matches the code, in three places (all of which need redoing
   against dev anyway, since the metrics and knob sections have moved). The
   ..._RING_SIZE paragraph still says drop counts are reported as dropped_total /
   dropped_since_last_dump "in JSON metadata" — they moved into the dump_stats record
   with coll_ / p2p_ prefixes, and metadata no longer carries them at all. That same
   paragraph still suggests lowering NCCL_INSPECTOR_DUMP_THREAD_INTERVAL_MICROSECONDS,
   which you correctly removed from the warning text, and the new stats-record section
   repeats it as "(or the dump interval shortened)".

Should fix

5. Device totals are still written only when device.hasData, so both counters drop
   out of the scrape once a GPU goes idle and rate() across the gap becomes unusable —
   which undercuts the rate(dropped)/rate(total) ratio you documented. Emit them for
   every known device instead.
6. Lock hold time on the completion hot path. The merged JSON dump path now drains
   the collective and P2P rings under a single commInfo->guard write-lock hold instead
   of two, so worst-case hold time roughly doubles on a lock that the completion path
   takes for every operation. Please take the lock once per ring as before. While you're
   in there: the INS_CHK around inspectorRingDrain returns with the write lock still
   held — that predates this PR, but the longer combined critical section makes it a
   wider window.
7. droppedReported is assigned in both Prometheus drain paths but never read there
   (only JSON computes deltas). Dead write — drop it, or add a comment explaining why

[shaq918]

Thanks — this was a really useful review. I've rebased the PR onto dev and reworked it against everything below; it's now a single commit (0140e55). Point by point:

1. Retarget to dev. Done — the branch now sits on top of dev. As you predicted, inspector_ring.{cc,h}, inspector_plugin.cc and inspector_json.cc applied cleanly, and the inspectorPromDevice& refactor meant most of the prom plumbing was already there — I only added the drop accounting on top of it.

2. OTLP mode. Extended rather than scoped out. inspector_otel.cc now accumulates the same per-device totals and emits nccl_collectives_total / nccl_collectives_dropped_total / nccl_p2p_total / nccl_p2p_dropped_total as data points. I gated them under the existing NCCL_INSPECTOR_OTEL_VERBOSE so the default aggregated OTLP output stays minimal, reusing OTLP's own "extra detail" knob rather than inventing a second flag — but I'm very open to a dedicated NCCL_INSPECTOR_OTEL_DUMP_STATS if you'd prefer the low-cardinality drop counters to be available without the full per-operation verbose payload. Your call.

Worth noting: the one-time overflow WARN lives in the enqueue path, so it fires in all three modes regardless of gating — the metrics are the opt-in dashboard nicety, but the "you're losing data" signal is never fully silent.

3. Gate the Prometheus metrics. Done — NCCL_INSPECTOR_PROM_DUMP_STATS (default 0), registered in the env-var table next to NCCL_INSPECTOR_PROM_DUMP, with a matching parse block and a README entry. Default Prometheus output is byte-identical to before.

4. README. Reworked against dev: the ring-size entries now document the overwrite-on-full behavior and where drops surface; the drop fields are described in the dump_stats record (not per-record metadata); and there's no "lower the dump interval" advice anywhere (neither the warning nor the docs).

5. Idle-device totals. The per-device stats are now emitted for every known device each dump, not just those with hasData, so the counters stay continuous and rate() works across quiet windows.

6. Lock hold time. Reverted to one guard hold per ring — the JSON path drains collectives and P2P under separate commInfo->guard acquisitions (as before), while still emitting a single combined dump_stats record per comm. (The pre-existing INS_CHK-returns-with-lock pattern is unchanged from dev.)

7. Dead write. Removed — the Prometheus drain paths no longer touch droppedReported (only JSON computes since-last-dump deltas, so only JSON advances that watermark).

[rishdas]

/mirror dev

[xiaofanl-nvidia]

/mirror dev