[heft] Require a logging name for every operation #4469
Conversation
octogonz
requested review from
iclanton,
apostolisms,
D4N14L and
dmichon-msft
as code owners
| @@ -453,6 +453,7 @@ function _getOrCreatePhaseOperation( | |||
| // Only create the operation. Dependencies are hooked up separately | |||
| operation = new Operation({ | |||
| groupName: phase.phaseName, | |||
| name: `${phase.phaseName} phase`, | |||
There was a problem hiding this comment.
What is the appropriate name for a phase?
In the source code, there currently seem to be very few places that actually read the name or groupName.
There was a problem hiding this comment.
A phase operation is setup work that runs before all tasks in the given phase, so it should indicate something about setup or initialization.
There was a problem hiding this comment.
Currently the phase operation is only used for performing the clean.
…ration names percolate through the call graphs - Fix spelling of "requestor" and make it non-optional
| return requestRun(this.name); | ||
| return requestRun(this.operationName); | ||
| case undefined: | ||
| throw new InternalError(`The operation state is undefined`); |
There was a problem hiding this comment.
(Improved this error message)
| @@ -20,7 +20,7 @@ export interface IOperationExecutionOptions { | |||
| parallelism: number; | |||
| terminal: ITerminal; | |||
|
|
|||
| requestRun?: (requestor?: string) => void; | |||
| requestRun?: (requester: string) => void; | |||
There was a problem hiding this comment.
(The requester is now also non-optional)
There was a problem hiding this comment.
The requester could ultimately be multiple layers of identifiers; I guess we can just concatenate strings, but it might be better to make this an array of hierarchy nodes (e.g. Operation, file). When Rush receives one of these messages over IPC it'll get wrapped in a further node for the Rush-level Operation.
| @@ -193,7 +193,9 @@ export class WatchLoop implements IWatchLoopState { | |||
|
|
|||
| try { | |||
| status = await this.runUntilStableAsync(abortController.signal); | |||
| this._requestRunPromise.finally(requestRunFromHost); | |||
| this._requestRunPromise.finally(() => { | |||
| requestRunFromHost('runIPCAsync'); | |||
There was a problem hiding this comment.
@dmichon-msft what is the appropriate name to supply here? In the current implementation it was implicitly getting mapped to undefined.
(If we're going to be referring to requesters/operations by names, then we should provide a meaningful name in every code path, rather than printing awkward messages like "This job was started by an unknown source.")
There was a problem hiding this comment.
If .finally is not forwarding the promise result (which is the underlying requester), use .then instead. The error code path is unreachable code, since the reject callback is discarded.
|
any cycles available to cleanup this PR @octogonz? it would be great to see this fix in a future release. I'm happy to help if useful. |
Summary
Fixes #4467
Details
@bartvandenende-wm has proposed a simpler fix in PR #4468
However, it seems error-prone and unnecessary to allow operations to be started entirely anonymously. My PR proposes a more aggressive API change to require the
namefield.👉 Let's discuss the design in the comments for issue #4467
How it was tested
Redo repro steps from #4467
Impacted documentation
None