feat(telemetry-utils): escalate selected performance events to essential#27462
Conversation
…y are cancel events or if they were above a certain duration
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (125 lines, 2 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
Adds two opt-in markers on IPerformanceEventMarkers to selectively promote performance telemetry events to essential log level: endEventEssentialDurationThresholdMs (escalates _end events whose duration exceeds the threshold) and logCancelAsEssential (escalates _cancel events while preserving their category). Escalation is implemented by returning undefined from a new getLogLevel helper, leveraging the existing sendPerformanceEvent contract that treats undefined as LogLevel.essential. Also fixes a small comment typo (wether → whether).
Changes:
- Extend
IPerformanceEventMarkerswith the two new escalation markers and routereportEventthrough a newgetLogLevelto apply them. - Expand
MockLoggerin tests to capture per-event log levels and add aLog level escalationsuite covering below/above threshold end events, cancel escalation, default behavior, and error-category essential behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/utils/telemetry-utils/src/logger.ts | Adds endEventEssentialDurationThresholdMs/logCancelAsEssential markers and getLogLevel to escalate the corresponding events to essential by emitting undefined. |
| packages/utils/telemetry-utils/src/test/performanceEvent.spec.ts | Adds log-level capture to MockLogger and new tests covering threshold-based end escalation, cancel escalation, and unchanged default/error-category behaviors. |
markfields
left a comment
markfields
left a comment
There was a problem hiding this comment.
Thanks for knocking this out! Left some small suggestions
| private getLogLevel( | ||
| eventNameSuffix: string, | ||
| event: ITelemetryPerformanceEventExt, | ||
| ): typeof LogLevel.verbose | typeof LogLevel.info | undefined { |
There was a problem hiding this comment.
Why are we using undefined to signal essential? Seems like unnecessarily tight coupling with the default specified elsewhere
There was a problem hiding this comment.
because the sendPerformanceEvent can only receive LogLevel.info, LogLevel.verbose or undefined. So we need it to send undefined so that it defaults to LogLevel.essential
There was a problem hiding this comment.
Oh ok. Might be clearer to update that to take essential at some point
There was a problem hiding this comment.
So the reason why I made it that way is because sendPerformanceEvent can only take LogLevel.verbose, LogLevel.info or undefined. With the breaking change happening in #26910, that should be fixable and then it can take esential.
| const callback = (): void => { | ||
| callbackCalls++; | ||
| }; | ||
| const delay = async (): Promise<void> => |
There was a problem hiding this comment.
See if you can quickly switch to using Sinon fake timers, which lets you test the delay without taking any real time. Hopefully you can one-shot it with Copilot.
These little ms setTimeouts can quickly add up and unnecessarily bloat unit test run times.
There was a problem hiding this comment.
Admittedly 1ms is probably not make-or-break but still good practice to avoid real delays. More reliable way to test too
| { eventName: "EssentialCancel" }, | ||
| { | ||
| cancel: "generic", | ||
| logCancelAsEssential: true, |
There was a problem hiding this comment.
Can we make this the default? Feels likely we'll always want this
There was a problem hiding this comment.
I assumed that when you made this the default, I'm guessing you were referring to the logCancelAsEssential code. That change was made
MarioJGMsoft
requested review from
a team,
WillieHabi,
jason-ha,
jatgarg,
kian-thompson,
markfields and
steffenloesch
Description
Adds opt-in
PerformanceEventmarkers for promoting selected performance telemetry to essential:endEventEssentialDurationThresholdMslogs_endevents as essential when their duration exceeds the configured threshold.logCancelAsEssentiallogs_cancelevents as essential while preserving the configured cancel category.This lets callers keep normal performance events at their configured verbosity while ensuring long-running operations and configured cancel events are not filtered out. Tests cover threshold behavior, cancel escalation, unchanged default log-level behavior, and existing error-category essential behavior.
Reviewer Guidance
The review process is outlined on this wiki page.
Please focus on whether the marker names and escalation semantics are clear, especially using the existing undefined-log-level path to preserve essential-event behavior.
Fixes: AB#74788