Skip to content

Commit b5f672e

Browse files
committed
gh-138122: Fix sample counting for filtered profiling modes
The live collector's efficiency bar was incorrectly showing 0% success rate when using filtered modes like --mode exception where no thread had an active exception. This happened because samples were only counted as successful when frames were actually processed, conflating "profiler health" with "filter hit rate". Samples are now always counted as successful when the profiler can read from the target process, regardless of whether any frames matched the current filter. This ensures the efficiency bar accurately reflects profiler connectivity rather than filter selectivity. The invariant total_samples == successful_samples + failed_samples is now properly maintained.
1 parent c865ab3 commit b5f672e

File tree

3 files changed

+61
-39
lines changed

3 files changed

+61
-39
lines changed

Lib/profiling/sampling/live_collector/collector.py

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -395,11 +395,9 @@ def collect(self, stack_frames):
395395
if has_gc_frame:
396396
self.gc_frame_samples += 1
397397

398-
# Only count as successful if we actually processed frames
399-
# This is important for modes like --mode exception where most samples
400-
# may be filtered out at the C level
401-
if frames_processed:
402-
self.successful_samples += 1
398+
# Count as successful - the sample worked even if no frames matched the filter
399+
# (e.g., in --mode exception when no thread has an active exception)
400+
self.successful_samples += 1
403401
self.total_samples += 1
404402

405403
# Handle input on every sample for instant responsiveness

Lib/profiling/sampling/live_collector/widgets.py

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -308,31 +308,21 @@ def draw_sample_stats(self, line, width, elapsed):
308308

309309
def draw_efficiency_bar(self, line, width):
310310
"""Draw sample efficiency bar showing success/failure rates."""
311-
success_pct = (
312-
self.collector.successful_samples
313-
/ max(1, self.collector.total_samples)
314-
) * 100
315-
failed_pct = (
316-
self.collector.failed_samples
317-
/ max(1, self.collector.total_samples)
318-
) * 100
311+
# total_samples = successful_samples + failed_samples, so percentages add to 100%
312+
total = max(1, self.collector.total_samples)
313+
success_pct = (self.collector.successful_samples / total) * 100
314+
failed_pct = (self.collector.failed_samples / total) * 100
319315

320316
col = 0
321317
self.add_str(line, col, "Efficiency:", curses.A_BOLD)
322318
col += 11
323319

324-
label = f" {success_pct:>5.2f}% good, {failed_pct:>4.2f}% failed"
320+
label = f" {success_pct:>5.2f}% good, {failed_pct:>5.2f}% failed"
325321
available_width = width - col - len(label) - 3
326322

327323
if available_width >= MIN_BAR_WIDTH:
328324
bar_width = min(MAX_EFFICIENCY_BAR_WIDTH, available_width)
329-
success_fill = int(
330-
(
331-
self.collector.successful_samples
332-
/ max(1, self.collector.total_samples)
333-
)
334-
* bar_width
335-
)
325+
success_fill = int((self.collector.successful_samples / total) * bar_width)
336326
failed_fill = bar_width - success_fill
337327

338328
self.add_str(line, col, "[", curses.A_NORMAL)

Lib/test/test_profiling/test_sampling_profiler/test_live_collector_core.py

Lines changed: 52 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -267,21 +267,59 @@ def test_collect_with_frames(self):
267267
self.assertEqual(collector.failed_samples, 0)
268268

269269
def test_collect_with_empty_frames(self):
270-
"""Test collect with empty frames."""
270+
"""Test collect with empty frames counts as successful.
271+
272+
A sample is considered successful if the profiler could read from the
273+
target process, even if no frames matched the current filter (e.g.,
274+
--mode exception when no thread has an active exception). The sample
275+
itself worked; it just didn't produce frame data.
276+
"""
271277
collector = LiveStatsCollector(1000)
272278
thread_info = MockThreadInfo(123, [])
273279
interpreter_info = MockInterpreterInfo(0, [thread_info])
274280
stack_frames = [interpreter_info]
275281

276282
collector.collect(stack_frames)
277283

278-
# Empty frames do NOT count as successful - this is important for
279-
# filtered modes like --mode exception where most samples may have
280-
# no matching data. Only samples with actual frame data are counted.
281-
self.assertEqual(collector.successful_samples, 0)
284+
# Empty frames still count as successful - the sample worked even
285+
# though no frames matched the filter
286+
self.assertEqual(collector.successful_samples, 1)
282287
self.assertEqual(collector.total_samples, 1)
283288
self.assertEqual(collector.failed_samples, 0)
284289

290+
def test_sample_counts_invariant(self):
291+
"""Test that total_samples == successful_samples + failed_samples.
292+
293+
Empty frame data (e.g., from --mode exception with no active exception)
294+
still counts as successful since the profiler could read process state.
295+
"""
296+
collector = LiveStatsCollector(1000)
297+
298+
# Mix of samples with and without frame data
299+
frames = [MockFrameInfo("test.py", 10, "func")]
300+
thread_with_frames = MockThreadInfo(123, frames)
301+
thread_empty = MockThreadInfo(456, [])
302+
interp_with_frames = MockInterpreterInfo(0, [thread_with_frames])
303+
interp_empty = MockInterpreterInfo(0, [thread_empty])
304+
305+
# Collect various samples
306+
collector.collect([interp_with_frames]) # Has frames
307+
collector.collect([interp_empty]) # No frames (filtered)
308+
collector.collect([interp_with_frames]) # Has frames
309+
collector.collect([interp_empty]) # No frames (filtered)
310+
collector.collect([interp_empty]) # No frames (filtered)
311+
312+
# All 5 samples are successful (profiler could read process state)
313+
self.assertEqual(collector.total_samples, 5)
314+
self.assertEqual(collector.successful_samples, 5)
315+
self.assertEqual(collector.failed_samples, 0)
316+
317+
# Invariant must hold
318+
self.assertEqual(
319+
collector.total_samples,
320+
collector.successful_samples + collector.failed_samples
321+
)
322+
285323
def test_collect_skip_idle_threads(self):
286324
"""Test that idle threads are skipped when skip_idle=True."""
287325
collector = LiveStatsCollector(1000, skip_idle=True)
@@ -327,9 +365,10 @@ def test_collect_multiple_threads(self):
327365
def test_collect_filtered_mode_percentage_calculation(self):
328366
"""Test that percentages use successful_samples, not total_samples.
329367
330-
This is critical for filtered modes like --mode exception where most
331-
samples may be filtered out at the C level. The percentages should
332-
be relative to samples that actually had frame data, not all attempts.
368+
With the current behavior, all samples are considered successful
369+
(the profiler could read from the process), even when filters result
370+
in no frame data. This means percentages are relative to all sampling
371+
attempts that succeeded in reading process state.
333372
"""
334373
collector = LiveStatsCollector(1000)
335374

@@ -338,35 +377,30 @@ def test_collect_filtered_mode_percentage_calculation(self):
338377
thread_with_data = MockThreadInfo(123, frames_with_data)
339378
interpreter_with_data = MockInterpreterInfo(0, [thread_with_data])
340379

341-
# Empty thread simulates filtered-out data
380+
# Empty thread simulates filtered-out data at C level
342381
thread_empty = MockThreadInfo(456, [])
343382
interpreter_empty = MockInterpreterInfo(0, [thread_empty])
344383

345384
# 2 samples with data
346385
collector.collect([interpreter_with_data])
347386
collector.collect([interpreter_with_data])
348387

349-
# 8 samples without data (filtered out)
388+
# 8 samples without data (filtered out at C level, but sample still succeeded)
350389
for _ in range(8):
351390
collector.collect([interpreter_empty])
352391

353-
# Verify counts
392+
# All 10 samples are successful - the profiler could read from the process
354393
self.assertEqual(collector.total_samples, 10)
355-
self.assertEqual(collector.successful_samples, 2)
394+
self.assertEqual(collector.successful_samples, 10)
356395

357396
# Build stats and check percentage
358397
stats_list = collector.build_stats_list()
359398
self.assertEqual(len(stats_list), 1)
360399

361-
# The function appeared in 2 out of 2 successful samples = 100%
362-
# NOT 2 out of 10 total samples = 20%
400+
# The function appeared in 2 out of 10 successful samples = 20%
363401
location = ("test.py", 10, "exception_handler")
364402
self.assertEqual(collector.result[location]["direct_calls"], 2)
365403

366-
# Verify the percentage calculation in build_stats_list
367-
# direct_calls / successful_samples * 100 = 2/2 * 100 = 100%
368-
# This would be 20% if using total_samples incorrectly
369-
370404
def test_percentage_values_use_successful_samples(self):
371405
"""Test that percentages are calculated from successful_samples.
372406

0 commit comments

Comments
 (0)