Ensure ResourceCollector executes a final collection pass on stop. Previously, ResourceCollector abruptly exited the collection loop upon interruption without performing a collection pass for the tail time interval between the last 200 ms sleep tick and when the profiler was stopped. For fast builds, this caused metrics like Skyframe ACTION_EXECUTION counts to be omitted entirely from the JSON trace profile, leading to flaky failures in integration tests (such as profiler_test). Buildkite failure: https://buildkite.com/bazel/bazel-bazel/builds/36681#01a035ac-dc09-4093-9c89-8824e93b8452 Wrap the collection loop in a try-finally block in ResourceCollector that runs one last collectOnce pass before exiting, ensuring metrics up to the stop timestamp are fully recorded into the profile. Also guard against null SystemNetworkStatsService in LocalResourceUsageCollectors and NetworkMetricsCollector, which can be null in test environments where network services are not registered. PiperOrigin-RevId: 970500451 Change-Id: Ib6edd35fef47ddc134b9c7f152f09ff5177bddcf
diff --git a/src/main/java/com/google/devtools/build/lib/profiler/BUILD b/src/main/java/com/google/devtools/build/lib/profiler/BUILD index 44eab88..026056f 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/BUILD +++ b/src/main/java/com/google/devtools/build/lib/profiler/BUILD
@@ -32,6 +32,7 @@ "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party/java/guava:base", "//third_party/java/guava:collect", + "//third_party/java/jsr305_annotations", ], )
diff --git a/src/main/java/com/google/devtools/build/lib/profiler/LocalResourceUsageCollectors.java b/src/main/java/com/google/devtools/build/lib/profiler/LocalResourceUsageCollectors.java index b2d897c..ca56331 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/LocalResourceUsageCollectors.java +++ b/src/main/java/com/google/devtools/build/lib/profiler/LocalResourceUsageCollectors.java
@@ -40,6 +40,7 @@ import java.lang.management.MemoryMXBean; import java.util.HashMap; import java.util.function.BiConsumer; +import javax.annotation.Nullable; /** An assortment of classes that collects various interesting metrics about the local system. */ public class LocalResourceUsageCollectors { @@ -50,14 +51,14 @@ private final ResourceEstimator resourceEstimator; - private final SystemNetworkStatsService systemNetworkStatsService; + @Nullable private final SystemNetworkStatsService systemNetworkStatsService; public LocalResourceUsageCollectors( BugReporter bugReporter, InMemoryGraph graph, WorkerProcessMetricsCollector workerProcessMetricsCollector, ResourceEstimator resourceEstimator, - SystemNetworkStatsService systemNetworkStatsService) { + @Nullable SystemNetworkStatsService systemNetworkStatsService) { this.bugReporter = bugReporter; this.graph = graph; this.workerProcessMetricsCollector = workerProcessMetricsCollector; @@ -99,7 +100,7 @@ if (collectLoadAverage) { Profiler.instance().registerCounterSeriesCollector(new SystemLoadAverageCollector(osBean)); } - if (collectSystemNetworkUsage) { + if (collectSystemNetworkUsage && systemNetworkStatsService != null) { Profiler.instance() .registerCounterSeriesCollector( new SystemNetworkUsageCollector(systemNetworkStatsService));
diff --git a/src/main/java/com/google/devtools/build/lib/profiler/NetworkMetricsCollector.java b/src/main/java/com/google/devtools/build/lib/profiler/NetworkMetricsCollector.java index 36cfe0a..32ce72d 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/NetworkMetricsCollector.java +++ b/src/main/java/com/google/devtools/build/lib/profiler/NetworkMetricsCollector.java
@@ -53,7 +53,10 @@ @Nullable public SystemNetworkUsages collectSystemNetworkUsages( - double deltaNanos, SystemNetworkStatsService systemNetworkStatsService) { + double deltaNanos, @Nullable SystemNetworkStatsService systemNetworkStatsService) { + if (systemNetworkStatsService == null) { + return null; + } if (loopbackInterfaceNames == null) { try { loopbackInterfaceNames = getLoopbackInterfaceNames();
diff --git a/src/main/java/com/google/devtools/build/lib/profiler/ResourceCollector.java b/src/main/java/com/google/devtools/build/lib/profiler/ResourceCollector.java index 4bd547d..2297c75 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/ResourceCollector.java +++ b/src/main/java/com/google/devtools/build/lib/profiler/ResourceCollector.java
@@ -19,6 +19,7 @@ import com.google.common.base.Stopwatch; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.skybridge.ScOnly; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.concurrent.GuardedBy; import java.time.Duration; import java.util.LinkedHashMap; @@ -84,26 +85,37 @@ Duration startTime = stopwatch.elapsed(); Duration previousElapsed = stopwatch.elapsed(); profilingStarted = true; - while (!stopCollection) { - try { - Thread.sleep(COLLECT_SLEEP_INTERVAL.toMillis()); - } catch (InterruptedException e) { - return; - } - Duration nextElapsed = stopwatch.elapsed(); - double deltaNanos = nextElapsed.minus(previousElapsed).toNanos(); - Duration finalPreviousElapsed = previousElapsed; - synchronized (ResourceCollector.this) { - for (var collector : collectors) { - collector.collect( - deltaNanos, - (type, value) -> - addRange(type, startTime, finalPreviousElapsed, nextElapsed, value)); + try { + while (!stopCollection) { + try { + Thread.sleep(COLLECT_SLEEP_INTERVAL.toMillis()); + } catch (InterruptedException e) { + break; } + previousElapsed = collectOnce(startTime, previousElapsed); } - previousElapsed = nextElapsed; + } finally { + collectOnce(startTime, previousElapsed); } } + + @CanIgnoreReturnValue + private Duration collectOnce(Duration startTime, Duration previousElapsed) { + Duration nextElapsed = stopwatch.elapsed(); + double deltaNanos = nextElapsed.minus(previousElapsed).toNanos(); + if (deltaNanos <= 0) { + return previousElapsed; + } + Duration finalPreviousElapsed = previousElapsed; + synchronized (ResourceCollector.this) { + for (var collector : collectors) { + collector.collect( + deltaNanos, + (type, value) -> addRange(type, startTime, finalPreviousElapsed, nextElapsed, value)); + } + } + return nextElapsed; + } } public void stop() {
diff --git a/src/test/java/com/google/devtools/build/lib/profiler/ProfilerTest.java b/src/test/java/com/google/devtools/build/lib/profiler/ProfilerTest.java index cacfadd..62e2898 100644 --- a/src/test/java/com/google/devtools/build/lib/profiler/ProfilerTest.java +++ b/src/test/java/com/google/devtools/build/lib/profiler/ProfilerTest.java
@@ -354,6 +354,51 @@ } @Test + public void testResourceCollectorCollectsOnStop() throws Exception { + record TestTask(String laneName, String seriesName, CounterSeriesTask.Color color) + implements CounterSeriesTask {} + + AtomicInteger collectCount = new AtomicInteger(0); + CounterSeriesCollector customCollector = + (deltaNanos, consumer) -> { + collectCount.incrementAndGet(); + consumer.accept(new TestTask("Test Series", "test_metric", null), 42.0); + }; + + profiler.registerCounterSeriesCollector(customCollector); + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + profiler.start( + getAllProfilerTasks(), + buffer, + JSON_TRACE_FILE_FORMAT, + "dummy_output_base", + UUID.randomUUID(), + true, + clock, + clock.nanoTime(), + /* slimProfile= */ false, + /* slimProfileSizeLimit= */ -1, + /* includePrimaryOutput= */ false, + /* includeTargetLabel= */ false, + /* includeConfiguration= */ false, + /* collectTaskHistograms= */ true); + + // Stop after short delay without waiting for COLLECT_SLEEP_INTERVAL (200ms). + Thread.sleep(10); + profiler.stop(); + + JsonProfile jsonProfile = new JsonProfile(new ByteArrayInputStream(buffer.toByteArray())); + ImmutableList<TraceEvent> events = + jsonProfile.getTraceEvents().stream() + .filter(e -> e.name().equals("Test Series")) + .collect(toImmutableList()); + + assertThat(collectCount.get()).isAtLeast(1); + assertThat(events).isNotEmpty(); + assertThat(events.get(0).args()).containsKey("test_metric"); + } + + @Test public void testProfilerRecordingOnlySlowestEvents() throws Exception { ByteArrayOutputStream buffer = new ByteArrayOutputStream();