fix(timeline,cobuilds): cobuilt operations should reflect the cobuild time and not cache restore time#4680
Merged
iclanton merged 29 commits intomicrosoft:mainfrom May 7, 2025
Conversation
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
Contributor
Author
Contributor
Author
iclanton
reviewed
May 13, 2024
common/changes/@microsoft/rush/sennyeya-operation-timings_2024-05-07-16-36.json
Outdated
Show resolved
Hide resolved
iclanton
previously 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
582ef41 to
1da8e9d
Compare
This was referenced May 24, 2024
Contributor
|
Any reason this hasn't been merged? It's a real problem trying to work out performance of co-build enabled builds 😬 |
Contributor
Author
|
@UberMouse Lost track of this, I was waiting on #4755 to get merged so we didn't cause more telemetry skew with this. |
Member
|
@aramissennyeydd - this should be unblocked now. |
77ffeee to
44b9ff2
Compare
Contributor
Author
|
@iclanton This should be good for another round of 👀 🙏 |
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionRecord.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionRecord.ts
Outdated
Show resolved
Hide resolved
aramissennyeydd
commented
Jan 3, 2025
4171584 to
5a426de
Compare
dmichon-msft
reviewed
Mar 5, 2025
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/ConsoleTimelinePlugin.ts
Outdated
Show resolved
Hide resolved
iclanton
reviewed
Mar 5, 2025
aramissennyeydd
commented
Mar 5, 2025
aramissennyeydd
commented
Mar 5, 2025
Contributor
|
Any chance of this getting merged soon? |
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
Co-authored-by: David Michon <dmichon@microsoft.com>
Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
5c6a68d to
2dfa48e
Compare
Contributor
Author
|
Posted in Zulip for a review, sorry for the slow cycles here - fixed the merge conflict that was here as well |
Collaborator
|
Looks like @iclanton already approved it -- let me see what's up |
iclanton
approved these changes
May 7, 2025
This file contains hidden or 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
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
OperationExecutionRecordis currently only tracking cache restore time for cobuilt operations that are marked asRemoteOperatingand 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 thenonCachedDurationMsfrom 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
beforeResultmethod toOperationExecutionRecord#executeAsyncto handle theafterExecuteOperationhook instead of passing that work into theonResultmethod 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-testsandbox 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