-
Notifications
You must be signed in to change notification settings - Fork 577
feat(telemetry-utils): escalate selected performance events to essential #27462
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
Changes from 1 commit
2ee6acf
38642e5
17c50a3
310ae39
e16f46d
a820bf7
82ad45b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,25 +5,29 @@ | |
|
|
||
| import { strict as assert } from "node:assert"; | ||
|
|
||
| import type { ITelemetryBaseEvent } from "@fluidframework/core-interfaces"; | ||
| import { LogLevel, type ITelemetryBaseEvent } from "@fluidframework/core-interfaces"; | ||
|
|
||
| import { PerformanceEvent, TelemetryLogger } from "../logger.js"; | ||
| import type { TelemetryLoggerExt } from "../telemetryTypes.js"; | ||
|
|
||
| class MockLogger extends TelemetryLogger implements TelemetryLoggerExt { | ||
| public errorsLogged: number = 0; | ||
| public eventsLogged: number = 0; | ||
| public readonly events: ITelemetryBaseEvent[] = []; | ||
| public readonly logLevels: (LogLevel | undefined)[] = []; | ||
|
|
||
| public constructor() { | ||
| super(); | ||
| } | ||
|
|
||
| public send(event: ITelemetryBaseEvent): void { | ||
| public send(event: ITelemetryBaseEvent, logLevel?: LogLevel): void { | ||
| if (event.category === "error") { | ||
| ++this.errorsLogged; | ||
| } | ||
|
|
||
| ++this.eventsLogged; | ||
| this.events.push(event); | ||
| this.logLevels.push(logLevel); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -34,6 +38,10 @@ describe("PerformanceEvent", () => { | |
| const callback = (): void => { | ||
| callbackCalls++; | ||
| }; | ||
| const delay = async (): Promise<void> => | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Admittedly 1ms is probably not make-or-break but still good practice to avoid real delays. More reliable way to test too
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied change |
||
| new Promise<void>((resolve) => { | ||
| setTimeout(resolve, 1); | ||
| }); | ||
| const asyncCallback = async (event: PerformanceEvent): Promise<string | void> => { | ||
| const outerPromise = new Promise<string>((resolve, reject) => { | ||
| Promise.resolve("A") | ||
|
|
@@ -95,6 +103,100 @@ describe("PerformanceEvent", () => { | |
| ); | ||
| }); | ||
|
|
||
| describe("Log level escalation", () => { | ||
| it("Logs below-threshold end events at the configured log level", () => { | ||
| const perfEvent = PerformanceEvent.start( | ||
| logger, | ||
| { eventName: "BelowThreshold" }, | ||
| { | ||
| end: true, | ||
| endEventEssentialDurationThresholdMs: Number.MAX_SAFE_INTEGER, | ||
| }, | ||
| true, | ||
| LogLevel.info, | ||
| ); | ||
|
|
||
| perfEvent.end(); | ||
|
|
||
| assert.deepStrictEqual(logger.logLevels, [LogLevel.info]); | ||
| assert.equal(logger.events[0]?.eventName, "BelowThreshold_end"); | ||
| }); | ||
|
|
||
| it("Logs above-threshold end events as essential", async () => { | ||
| const perfEvent = PerformanceEvent.start( | ||
| logger, | ||
| { eventName: "AboveThreshold" }, | ||
| { | ||
| end: true, | ||
| endEventEssentialDurationThresholdMs: 0, | ||
| }, | ||
| true, | ||
| LogLevel.info, | ||
| ); | ||
|
|
||
| await delay(); | ||
| perfEvent.end(); | ||
|
|
||
| assert.deepStrictEqual(logger.logLevels, [LogLevel.essential]); | ||
| assert.equal(logger.events[0]?.eventName, "AboveThreshold_end"); | ||
| }); | ||
|
|
||
| it("Logs cancel events as essential when configured", () => { | ||
| const perfEvent = PerformanceEvent.start( | ||
| logger, | ||
| { eventName: "EssentialCancel" }, | ||
| { | ||
| cancel: "generic", | ||
| logCancelAsEssential: true, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this the default? Feels likely we'll always want this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed that when you made this the default, I'm guessing you were referring to the |
||
| }, | ||
| true, | ||
| LogLevel.info, | ||
| ); | ||
|
|
||
| perfEvent.cancel(); | ||
|
|
||
| assert.deepStrictEqual(logger.logLevels, [LogLevel.essential]); | ||
| assert.equal(logger.events[0]?.eventName, "EssentialCancel_cancel"); | ||
| assert.equal(logger.events[0]?.category, "generic"); | ||
| }); | ||
|
|
||
| it("Preserves scalar LogLevel.info behavior when marker escalation is not configured", () => { | ||
| const perfEvent = PerformanceEvent.start( | ||
| logger, | ||
| { eventName: "InfoCancel" }, | ||
| { | ||
| cancel: "generic", | ||
| }, | ||
| true, | ||
| LogLevel.info, | ||
| ); | ||
|
|
||
| perfEvent.cancel(); | ||
|
|
||
| assert.deepStrictEqual(logger.logLevels, [LogLevel.info]); | ||
| assert.equal(logger.events[0]?.eventName, "InfoCancel_cancel"); | ||
| }); | ||
|
|
||
| it("Continues to log error-category performance events as essential", () => { | ||
| const perfEvent = PerformanceEvent.start( | ||
| logger, | ||
| { eventName: "ErrorCategory", category: "error" }, | ||
| { | ||
| end: true, | ||
| endEventEssentialDurationThresholdMs: Number.MAX_SAFE_INTEGER, | ||
| }, | ||
| true, | ||
| LogLevel.info, | ||
| ); | ||
|
|
||
| perfEvent.end(); | ||
|
|
||
| assert.deepStrictEqual(logger.logLevels, [LogLevel.essential]); | ||
| assert.equal(logger.events[0]?.eventName, "ErrorCategory_end"); | ||
| assert.equal(logger.events[0]?.category, "error"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Event sampling", () => { | ||
| it("Events are logged at least once", async () => { | ||
| await PerformanceEvent.timedExecAsync( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok. Might be clearer to update that to take essential at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the reason why I made it that way is because
sendPerformanceEventcan only takeLogLevel.verbose,LogLevel.infoor undefined. With the breaking change happening in #26910, that should be fixable and then it can take esential.