fix(telemetry-utils): prevent performance mark/measure entry accumulation#26401
fix(telemetry-utils): prevent performance mark/measure entry accumulation#26401tylerbutler wants to merge 1 commit intomicrosoft:mainfrom
Conversation
…tion PerformanceEvent now clears performance marks and measures from the Performance API after use. Previously, cancel() orphaned the start mark and even the success path never cleared entries, causing indefinite accumulation in long-running hosts. Uses try/finally in timedExec and timedExecAsync for scope-bound cleanup.
There was a problem hiding this comment.
Pull request overview
This PR updates PerformanceEvent in @fluidframework/telemetry-utils to prevent unbounded accumulation of window.performance mark/measure entries in long-running applications by ensuring marks/measures are cleared on both success and cancellation/error paths.
Changes:
- Add
finally-based cleanup totimedExec()/timedExecAsync()to ensure Performance API marks are cleared regardless of execution path. - Update
performanceEndMark()to clear created marks/measures after recording the measure. - Update
cancel()to clear orphaned start marks so cancelled events don’t leak entries.
| if (this.startMark !== undefined && this.event) { | ||
| const endMark = `${this.event.eventName}-end`; | ||
| window.performance.mark(endMark); | ||
| window.performance.measure(`${this.event.eventName}`, this.startMark, endMark); | ||
| window.performance.measure(this.event.eventName, this.startMark, endMark); | ||
| window.performance.clearMarks(this.startMark); | ||
| window.performance.clearMarks(endMark); | ||
| window.performance.clearMeasures(this.event.eventName); | ||
| this.startMark = undefined; |
There was a problem hiding this comment.
performanceEndMark() now clears marks/measures by name, but the mark/measure names are derived only from eventName (e.g. ${eventName}-start). If two PerformanceEvents with the same eventName overlap (common when multiple containers load concurrently), clearMarks(this.startMark) will remove all matching start marks, which can cause the other in-flight event’s subsequent performance.measure() to throw or record incorrect timings. Consider generating per-instance unique start/end mark names (and a unique measure name) and clearing by those unique names to avoid cross-talk between concurrent events.
| if (this.startMark !== undefined && this.event) { | ||
| const endMark = `${this.event.eventName}-end`; | ||
| window.performance.mark(endMark); | ||
| window.performance.measure(`${this.event.eventName}`, this.startMark, endMark); | ||
| window.performance.measure(this.event.eventName, this.startMark, endMark); | ||
| window.performance.clearMarks(this.startMark); | ||
| window.performance.clearMarks(endMark); | ||
| window.performance.clearMeasures(this.event.eventName); |
There was a problem hiding this comment.
The constructor only guards on window.performance.mark being present, but the new code unconditionally calls window.performance.measure, clearMarks, and clearMeasures whenever startMark is set. In environments with partial/incorrect Performance API shims (or tests), this can throw during cleanup and fail the measured operation. Consider gating startMark creation on the full set of required methods (mark/measure/clearMarks/clearMeasures) or defensively checking method existence before calling them.
| } finally { | ||
| // Scope-bound cleanup: ensure performance marks are always cleared, | ||
| // even if cancel() was called (which doesn't create a measure). | ||
| perfEvent.clearPerformanceMarks(); | ||
| } |
There was a problem hiding this comment.
There are existing unit tests for PerformanceEvent, but none assert the new Performance API cleanup behavior. To prevent regressions, consider adding tests that stub window.performance and verify marks/measures are cleared on (1) success, (2) exception path, and (3) cancel() without end() (including a concurrent same-eventName scenario if you keep name-based clearing).
1 participant
Summary
PerformanceEvent.cancel()now clears orphaned start marks from the Performance API — previously it setthis.event = undefinedwithout any cleanup, leaking the start mark foreverperformanceEndMark()now callsclearMarks()/clearMeasures()after creating the measure, so entries don't accumulate on the success path eithertimedExec()andtimedExecAsync()use afinallyblock for scope-bound cleanup, ensuring marks are always cleared regardless of code path