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
fix(timeline,cobuilds): cobuilt operations should reflect the cobuild time and not cache restore time #4680
Open
aramissennyeydd
wants to merge
18
commits into
microsoft:main
Choose a base branch
from
aramissennyeydd:sennyeya/operation-timings
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
fix(timeline,cobuilds): cobuilt operations should reflect the cobuild time and not cache restore time #4680
aramissennyeydd
wants to merge
18
commits into
microsoft:main
from
aramissennyeydd:sennyeya/operation-timings
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aramissennyeydd
requested review from
iclanton,
octogonz,
apostolisms,
D4N14L and
dmichon-msft
as code owners
May 7, 2024 17:10
iclanton
approved these changes
May 10, 2024
libraries/rush-lib/src/logic/operations/ConsoleTimelinePlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
iclanton
reviewed
May 13, 2024
common/changes/@microsoft/rush/sennyeya-operation-timings_2024-05-07-16-36.json
Outdated
Show resolved
Hide resolved
iclanton
approved these changes
May 13, 2024
libraries/rush-lib/src/logic/operations/ConsoleTimelinePlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/ConsoleTimelinePlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/IOperationExecutionResult.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
aramissennyeydd
force-pushed
the
sennyeya/operation-timings
branch
from
May 21, 2024 20:12
582ef41
to
1da8e9d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
OperationExecutionRecord
is currently only tracking cache restore time for cobuilt operations that are marked asRemoteOperating
and are then restored from cache. This is a confusing UX and causes developers + maintainers to have to search across multiple machine logs to determine operation run times. This PR adjusts that to use thenonCachedDurationMs
from the state file. This felt like something that #3649 was intending to do or is in a similar vein.Before:
After:
Details
This overwrites the existing stopwatch for operations that were not cobuilt on the specific agent. It adds a new
beforeResult
method toOperationExecutionRecord#executeAsync
to handle theafterExecuteOperation
hook instead of passing that work into theonResult
method which ended up receiving running stopwatches and had other assumptions of state (collated terminal not closed). This does add possibly breaking behavior where cobuilds were showing cache times which might be useful, but I don't think those are as useful as having the cobuild time available across all agents. That also has the secondary effect of making the timeline views of all agents much more cohesive - though as we see above they're not the exact same across agents.How it was tested
I tested this locally using the
build-tests/rush-redis-cobuild-plugin-integration-test
sandbox repo with 2 runners, confirmed that timings generally matched across the instances. There's about 10ms (rounded up) of difference between the agents, but this already seems much more useful.Impacted documentation
None