-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Add ActionCacheStatistics to BEP #17615
Add ActionCacheStatistics to BEP #17615
Conversation
The field action_cache_statistics has been added to the ActionSummary message of the BuildMetrics message in the build event protocol. This field is defined with the already-existing ActionCacheStatistics message and is set in the MetricsCollector when the action cache is saved.
@bazelbuild/triage Can we get someone to review this please? |
Are there any updates on the review of this PR? |
@bazelbuild/triage @michaeledgar Can we get a review, or assigned to someone else, please? Thanks. |
@bazel-io flag |
@bazel-io fork 6.3.0 |
@brentleyjones @haxorz @crydell-ericsson
cc: @bazelbuild/triage |
I'll leave it to @crydell-ericsson on how easy it is to get the dependent commits in, or if this can only really work with 7.0. The change looked small enough, so I was hopeful. |
All of the differences mentioned by @iancha1992 are changes that have ended up being added while keeping this PR up to date with the master branch (perhaps that's something I shouldn't have done?). If you look at the initial commit in this PR 5f68c8f, you'll see that it does not have any of these dependencies. In other words, the listed dependencies are not needed for the commit, they just happened to cause conflicts by being right next to the dependencies that were needed. Should I do anything from my side to amend this? |
I think you can submit the cherry-pick PR (based on your original commit maybe?) and @iancha1992 would accept that. |
I have put up a cherry pick PR in #18914 now. I haven't submitted PRs for cherry picks before, let me know if I've done anything incorrectly and I'll try to fix it. |
I approved. The diff looked fine. I've never authored or reviewed a cherrypick before, so someone else should probably double-check. |
These proto files are taken from commit bazelbuild/bazel@76117b4 Highlight: - Inclusion of ActionCacheStatistics in build_event_stream bazelbuild/bazel#17615 - Explicit Execution phase timing (in prepare for Skymeld) bazelbuild/bazel@be63eee - If `--noallow_analysis_cache_discard` is set, BuildFinished may contains FailureDetails -> CONFIGURATION_DISCARDED_ANALYSIS_CACHE. bazelbuild/bazel#16805 - ExecRequest event is included for `bazel run` bazelbuild/bazel@9a047de - TestProgress event is included for TestRunner action bazelbuild/bazel@d8b8ab0 - ActionExecuted now includes start/end time and strategy information bazelbuild/bazel@2ddacab
Solves #17315 .
The field action_cache_statistics has been added to the ActionSummary message of the BuildMetrics message in the build event protocol. This field is defined with the already-existing ActionCacheStatistics message and is set in the MetricsCollector when the action cache is saved.