Skip to content

Add critical path to BEP #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add critical path to BEP #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cc415fe
Add critical path to BEP
Yannic Oct 30, 2022
bbe7166
fix bootstrap
Yannic Nov 1, 2022
d72ca65
Merge branch 'master' into yannic-critical-path-bep
Yannic Apr 6, 2023
8618bf8
implement total time
Yannic Apr 6, 2023
7e3be06
undo
Yannic Apr 6, 2023
d4523bc
more
Yannic Apr 6, 2023
a9ddafb
some cleanup
Yannic Apr 6, 2023
5bc29ea
register
Yannic Apr 6, 2023
b755142
Update BUILD
Yannic Apr 6, 2023
bc17e9a
Update BuildCompleteEvent.java
Yannic Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/buildeventstream/proto/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ proto_library(
srcs = ["build_event_stream.proto"],
deps = [
"//src/main/protobuf:command_line_proto",
"//src/main/protobuf:critical_path_proto",
"//src/main/protobuf:failure_details_proto",
"//src/main/protobuf:invocation_policy_proto",
"@com_google_protobuf//:duration_proto",
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package build_event_stream;
import "google/protobuf/duration.proto";
import "google/protobuf/timestamp.proto";
import "src/main/protobuf/command_line.proto";
import "src/main/protobuf/critical_path.proto";
import "src/main/protobuf/failure_details.proto";
import "src/main/protobuf/invocation_policy.proto";

Expand Down Expand Up @@ -222,6 +223,7 @@ message BuildEventId {
// Identifier of an event providing convenience symlinks information.
message ConvenienceSymlinksIdentifiedId {}

// Next id: 28.
oneof id {
UnknownBuildEventId unknown = 1;
ProgressId progress = 2;
Expand Down Expand Up @@ -249,6 +251,7 @@ message BuildEventId {
WorkspaceConfigId workspace = 23;
BuildMetadataId build_metadata = 24;
ConvenienceSymlinksIdentifiedId convenience_symlinks_identified = 25;
build.bazel.bep.CriticalPath.BepId critical_path = 27;
}
}

Expand Down Expand Up @@ -1151,6 +1154,8 @@ message ConvenienceSymlink {
// events as children. More details, which are specific to the kind of event
// that is observed, is provided in the payload. More options for the payload
// might be added in the future.
//
// Next id: 30.
message BuildEvent {
reserved 11, 19;
BuildEventId id = 1;
Expand Down Expand Up @@ -1180,5 +1185,6 @@ message BuildEvent {
WorkspaceConfig workspace_info = 25;
BuildMetadata build_metadata = 26;
ConvenienceSymlinksIdentified convenience_symlinks_identified = 27;
build.bazel.bep.CriticalPath critical_path = 29;
}
}
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.google.devtools.build.lib.events.OutputFilter;
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.exec.ExecutionOptions;
import com.google.devtools.build.lib.metrics.criticalpath.CriticalPathEvent;
import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
import com.google.devtools.build.lib.profiler.ProfilePhase;
import com.google.devtools.build.lib.profiler.Profiler;
Expand Down Expand Up @@ -694,7 +695,9 @@ public void stopRequest(
new BuildCompleteEvent(
result,
ImmutableList.of(
BuildEventIdUtil.buildToolLogs(), BuildEventIdUtil.buildMetrics())));
BuildEventIdUtil.buildToolLogs(),
BuildEventIdUtil.buildMetrics(),
CriticalPathEvent.BEP_ID)));
}
Comment on lines +698 to 701
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Null Reference

Added CriticalPathEvent.BEP_ID reference without null checking could cause NullPointerException if BEP_ID is not properly initialized. This would break the build completion event posting, potentially causing build failures or incomplete build events.

Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness

// Post the build tool logs event; the corresponding local files may be contributed from
// modules, and this has to happen after posting the BuildCompleteEvent because that's when
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/com/google/devtools/build/lib/metrics/criticalpath/AggregatedCriticalPath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
package com.google.devtools.build.lib.metrics.criticalpath;

import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.actions.AggregatedSpawnMetrics;
import com.google.devtools.build.lib.actions.SpawnMetrics;
import java.time.Duration;
Expand Down Expand Up @@ -103,4 +105,11 @@ public String toStringSummary() {
public String toStringSummaryNoRemote() {
return toString(true, false);
}

/**Posts the {@code BEP} event for the critical path on the provided {@link EventBus}. */
public void postEvent(EventBus eventBus) {
Preconditions.checkNotNull(eventBus);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Event Validation

The eventBus.post() call lacks null checking for totalTimeInMs which could cause runtime exceptions if the field is uninitialized. This could lead to failure when posting critical path events, potentially breaking the build event protocol integration.

Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • DbC-Precondition

eventBus.post(new CriticalPathEvent(Duration.ofMillis(totalTimeInMs)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Event Registration

The CriticalPathEvent is posted to the eventBus but there's no corresponding registration of the event handler. Without proper registration, the event will be posted but may not be processed by any listeners, causing the critical path information to be lost.

Standards
  • Business-Rule-Event-Handling
  • Logic-Verification-Event-Flow
  • Algorithm-Correctness-Event-Processing

}
Comment on lines +109 to +114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding a try-catch block around the eventBus.post call to handle potential exceptions during event posting. This could prevent the entire build from failing if there's an issue with the event bus.

  /**Posts the {@code BEP} event for the critical path on the provided {@link EventBus}. */
  public void postEvent(EventBus eventBus) {
    Preconditions.checkNotNull(eventBus);
    try {
      eventBus.post(new CriticalPathEvent(Duration.ofMillis(totalTimeInMs)));
    } catch (Exception e) {
      // Log the exception or handle it appropriately.
      System.err.println("Error posting CriticalPathEvent: " + e.getMessage());
    }
  }

Comment on lines +110 to +114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factory Method Missing

Direct instantiation of CriticalPathEvent couples AggregatedCriticalPath to the concrete implementation. Consider adding a factory method in CriticalPathEvent for creating instances, improving maintainability through better abstraction.

Standards
  • Design-Pattern-Factory
  • SOLID-DIP
  • Maintainability-Quality-Coupling

}
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/metrics/criticalpath/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,17 @@ java_library(
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
"//src/main/java/com/google/devtools/build/lib/clock",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/skyframe/rewinding:action_rewound_event",
"//src/main/protobuf:critical_path_java_proto",
"//third_party:flogger",
"//third_party:guava",
"//third_party:jsr305",
"@com_google_protobuf//:protobuf_java",
],
)
47 changes: 47 additions & 0 deletions src/main/java/com/google/devtools/build/lib/metrics/criticalpath/CriticalPathEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package com.google.devtools.build.lib.metrics.criticalpath;

import build.bazel.bep.CriticalPath;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId;
import com.google.devtools.build.lib.buildeventstream.BuildEventWithOrderConstraint;
import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
import java.time.Duration;
import java.util.Collection;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Information Disclosure Consideration: The CriticalPathEvent exposes detailed timing information (down to nanoseconds) about the build's critical path. While this is low severity, consider if this level of detail is necessary for all users or if access controls should limit this information to authorized users only. In security-sensitive environments, this information could potentially aid in reconnaissance by revealing system bottlenecks or optimization targets.

/** {@code Build event protocol} event for the {@code critical path} of a build. */
public final class CriticalPathEvent extends GenericBuildEvent
implements BuildEventWithOrderConstraint {
public static final BuildEventId BEP_ID =
BuildEventId.newBuilder().setCriticalPath(CriticalPath.BepId.getDefaultInstance()).build();

private final Duration totalTime;

CriticalPathEvent(Duration totalTime) {
super(BEP_ID, ImmutableList.of());

this.totalTime = Preconditions.checkNotNull(totalTime);
}
Comment on lines +23 to +27
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limited Constructor Access

CriticalPathEvent constructor has package-private access, limiting reusability from other packages. Consider making it public with appropriate factory methods if this event should be creatable outside its package.

Standards
  • Clean-Code-API-Design
  • SOLID-OCP
  • Maintainability-Quality-Reusability


@Override
public Collection<BuildEventId> postedAfter() {
return ImmutableList.of(BuildEventIdUtil.buildFinished());
}

@Override
public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) {
return GenericBuildEvent.protoChaining(this)
.setCriticalPath(
CriticalPath.newBuilder()
.setTotalTime(
com.google.protobuf.Duration.newBuilder()
.setSeconds(totalTime.getSeconds())
.setNanos(totalTime.getNano())
.build())
.build())
.build();
}
}
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/runtime/BuildSummaryStatsModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ public void buildComplete(BuildCompleteEvent event) {
.getResult()
.getBuildToolLogCollection()
.addDirectValue("process stats", spawnSummaryString.getBytes(StandardCharsets.UTF_8));

criticalPath.postEvent(eventBus);
} finally {
if (criticalPathComputer != null) {
eventBus.unregister(criticalPathComputer);
Expand Down
25 changes: 25 additions & 0 deletions src/main/protobuf/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,30 @@ java_library_srcs(
deps = [":command_server_java_proto"],
)

proto_library(
name = "critical_path_proto",
srcs = [
"critical_path.proto",
],
deps = [
"@com_google_protobuf//:duration_proto",
],
)

java_proto_library(
name = "critical_path_java_proto",
deps = [
":critical_path_proto",
],
)

java_library_srcs(
name = "critical_path_java_proto_srcs",
deps = [
":critical_path_java_proto",
],
)

proto_library(
name = "failure_details_proto",
srcs = ["failure_details.proto"],
Expand Down Expand Up @@ -272,6 +296,7 @@ filegroup(
":command_line_java_proto_srcs",
":command_server_java_grpc_srcs",
":command_server_java_proto_srcs",
":critical_path_java_proto_srcs",
":failure_details_java_proto_srcs",
":option_filters_java_proto_srcs",
":profile_java_proto_srcs",
Expand Down
33 changes: 33 additions & 0 deletions src/main/protobuf/critical_path.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright 2022 The Bazel Authors. All rights reserved.
//
// 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.

syntax = "proto3";

package build.bazel.bep;

import "google/protobuf/duration.proto";

option java_package = "build.bazel.bep";
option java_outer_classname = "CriticalPathProtos";
// option java_api_version = 2;
option java_multiple_files = true;

// Represents the critical path of a build.
message CriticalPath {
// Identifier of a BES event of this type.
message BepId {}

// The total time of the critical path.
google.protobuf.Duration total_time = 1;
Comment on lines +26 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete Documentation

The CriticalPath message lacks comprehensive documentation about its purpose and usage. Adding more detailed comments would improve maintainability by clarifying what critical path represents and how it's calculated.

Standards
  • Clean-Code-Documentation
  • Maintainability-Quality-Documentation

Comment on lines +27 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limited Event Data

The CriticalPath message contains only total_time with no additional information about path components or bottlenecks. This limits extensibility and future analysis capabilities. Consider adding fields for critical path components.

Standards
  • SOLID-OCP
  • Design-Pattern-Extensibility
  • Maintainability-Quality-Evolvability

}
4 changes: 3 additions & 1 deletion src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
import com.google.devtools.build.lib.runtime.BlazeRuntime;
import com.google.devtools.build.lib.runtime.BlazeServerStartupOptions;
import com.google.devtools.build.lib.runtime.BlazeWorkspace;
import com.google.devtools.build.lib.runtime.BuildSummaryStatsModule;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.runtime.NoSpawnCacheModule;
import com.google.devtools.build.lib.runtime.ServerBuilder;
Expand Down Expand Up @@ -563,7 +564,8 @@ protected BlazeRuntime.Builder getRuntimeBuilder() throws Exception {
.addBlazeModule(new OutputFilteringModule())
.addBlazeModule(connectivityModule)
.addBlazeModule(new SkymeldModule())
.addBlazeModule(getMockBazelRepositoryModule());
.addBlazeModule(getMockBazelRepositoryModule())
.addBlazeModule(new BuildSummaryStatsModule());
getSpawnModules().forEach(builder::addBlazeModule);
builder
.addBlazeModule(getBuildInfoModule())
Expand Down
Loading