feat(telemetry-utils): deprecate LogLevel.default/error and tag unspecified events as essential #27207
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| --- | ||
| "@fluidframework/core-interfaces": minor | ||
| "__section": deprecation | ||
| --- | ||
|
|
||
| Deprecated LogLevel.default and LogLevel.error | ||
|
|
||
| `LogLevel.default` and `LogLevel.error` in `@fluidframework/core-interfaces` are deprecated in favor of the semantically clearer `LogLevel.info` and `LogLevel.essential`. | ||
|
|
||
| #### Migration | ||
|
|
||
| The recommended replacement for `LogLevel.default` depends on how the value is used: | ||
|
|
||
| - For an **event's default `logLevel`** (e.g. the `logLevel` argument to `sendTelemetryEvent` / `sendPerformanceEvent` / `ITelemetryBaseLogger.send`), the recommendation is `LogLevel.essential`. | ||
|
Check failure on line 14 in .changeset/salty-colts-appear.md
|
||
|
Contributor
There was a problem hiding this comment. This is a little funny because we don't want anyone implementing |
||
| - For a logger's **default `minLogLevel`** (the threshold that filters events), `LogLevel.info` is the recommendation. | ||
|
|
||
| The replacement for `LogLevel.error` should always be essential. | ||
|
Contributor
There was a problem hiding this comment. "essential" -> " |
||
|
|
||
| See [issue #26969](https://github.com/microsoft/FluidFramework/issues/26969) for full guidance and removal tracking (planned for v3.0). | ||
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
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
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -224,18 +224,18 @@ export abstract class TelemetryLogger implements TelemetryLoggerExt { | |
| * | ||
| * @param event - the event to send | ||
| * @param error - optional error object to log | ||
| * @param logLevel - optional level of the log. If the event's category is `error`, | ||
| * the logLevel will be upgraded to {@link @fluidframework/core-interfaces#LogLevelConst.essential | LogLevel.essential}. | ||
| */ | ||
| public sendTelemetryEvent( | ||
| event: ITelemetryGenericEventExt, | ||
| error?: unknown, | ||
| logLevel?: typeof LogLevel.verbose | typeof LogLevel.info, | ||
| ): void { | ||
| this.sendTelemetryEventCore( | ||
| { ...event, category: event.category ?? "generic" }, | ||
| error, | ||
| event.category === "error" ? LogLevel.essential : (logLevel ?? LogLevel.essential), | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -244,12 +244,12 @@ export abstract class TelemetryLogger implements TelemetryLoggerExt { | |
| * | ||
| * @param event - the event to send | ||
| * @param error - optional error object to log | ||
| * @param logLevel - level of the log. | ||
| */ | ||
| private sendTelemetryEventCore( | ||
| event: ITelemetryGenericEventExt & { category: TelemetryEventCategory }, | ||
| error: unknown, | ||
| logLevel: LogLevel, | ||
| ): void { | ||
| const newEvent = convertToBaseEvent(event); | ||
| if (error !== undefined) { | ||
|
|
@@ -280,7 +280,7 @@ export abstract class TelemetryLogger implements TelemetryLoggerExt { | |
| category: "error", | ||
| }, | ||
| error, | ||
| LogLevel.essential, | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -289,13 +289,13 @@ export abstract class TelemetryLogger implements TelemetryLoggerExt { | |
| * | ||
| * @param event - Event to send | ||
| * @param error - optional error object to log | ||
| * @param logLevel - optional level of the log. If the event's category is `error`, | ||
| * the logLevel will be upgraded to {@link @fluidframework/core-interfaces#LogLevelConst.essential | LogLevel.essential}. | ||
| */ | ||
| public sendPerformanceEvent( | ||
| event: ITelemetryPerformanceEventExt, | ||
| error?: unknown, | ||
| logLevel?: typeof LogLevel.verbose | typeof LogLevel.info, | ||
| ): void { | ||
| const perfEvent = { | ||
| ...event, | ||
|
|
@@ -305,7 +305,7 @@ export abstract class TelemetryLogger implements TelemetryLoggerExt { | |
| this.sendTelemetryEventCore( | ||
| perfEvent, | ||
| error, | ||
| perfEvent.category === "error" ? LogLevel.essential : (logLevel ?? LogLevel.essential), | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -518,8 +518,8 @@ export class ChildLogger extends TelemetryLogger { | |
| } | ||
|
|
||
| private shouldFilterOutEvent(event: ITelemetryBaseEvent, logLevel?: LogLevel): boolean { | ||
| const eventLogLevel = logLevel ?? LogLevel.essential; | ||
| const configLogLevel = this.baseLogger.minLogLevel ?? LogLevel.info; | ||
| // Filter out in case event log level is below what is wanted in config. | ||
| return eventLogLevel < configLogLevel; | ||
| } | ||
|
|
@@ -533,7 +533,7 @@ export class ChildLogger extends TelemetryLogger { | |
| if (this.shouldFilterOutEvent(event, logLevel)) { | ||
| return; | ||
| } | ||
| this.baseLogger.send(this.prepareEvent(event), logLevel ?? LogLevel.essential); | ||
|
Member
There was a problem hiding this comment. What happened to making logLevel required on send for the sub-classes? Now we have two places in the same flow filling in defaults
Contributor
Author
There was a problem hiding this comment. It couldn't be made there might still be chances that ChildLogger and Multisink receive undefined from old containerRuntime (At least that's what I understood). @jason-ha to confirm |
||
| } | ||
|
MarioJGMsoft marked this conversation as resolved.
|
||
| } | ||
|
|
||
|
|
@@ -618,7 +618,7 @@ export class MultiSinkLogger extends TelemetryLogger { | |
|
|
||
| super(namespace, realProperties); | ||
| this.loggers = loggers; | ||
| this._minLogLevelOfAllLoggers = LogLevel.info; | ||
| this.calculateMinLogLevel(); | ||
| } | ||
|
|
||
|
|
@@ -630,7 +630,7 @@ export class MultiSinkLogger extends TelemetryLogger { | |
| if (this.loggers.length > 0) { | ||
| const logLevels: LogLevel[] = []; | ||
| for (const logger of this.loggers) { | ||
| logLevels.push(logger.minLogLevel ?? LogLevel.info); | ||
| } | ||
| this._minLogLevelOfAllLoggers = Math.min(...logLevels) as LogLevel; | ||
| } | ||
|
|
@@ -653,10 +653,10 @@ export class MultiSinkLogger extends TelemetryLogger { | |
| * | ||
| * @param event - the event to send to all the registered logger | ||
| */ | ||
| public send(event: ITelemetryBaseEvent, logLevel?: LogLevel): void { | ||
| const newEvent = this.prepareEvent(event); | ||
| for (const logger of this.loggers) { | ||
| logger.send(newEvent, logLevel ?? LogLevel.essential); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -693,14 +693,15 @@ export class PerformanceEvent { | |
| * @param emitLogs - should this instance emit logs. If set to false, logs will not be emitted to the logger, | ||
| * but measurements will still be performed and any specified markers will be generated. | ||
| * @param logLevel - optional {@link LogLevel} for events emitted by this performance event. | ||
| * If unspecified, {@link @fluidframework/core-interfaces#LogLevelConst.essential | LogLevel.essential} will be used. | ||
| * @returns An instance of {@link PerformanceEvent} | ||
|
MarioJGMsoft marked this conversation as resolved.
|
||
| */ | ||
| public static start( | ||
| logger: TelemetryLoggerExt | ITelemetryLoggerExt, | ||
| event: ITelemetryGenericEventExt, | ||
| markers?: IPerformanceEventMarkers, | ||
| emitLogs: boolean = true, | ||
| logLevel?: typeof LogLevel.verbose | typeof LogLevel.info, | ||
| ): PerformanceEvent { | ||
| return new PerformanceEvent( | ||
| extractTelemetryLoggerExt(logger), | ||
|
|
@@ -720,6 +721,7 @@ export class PerformanceEvent { | |
| * @param sampleThreshold - events with the same name and category will be sent to the logger | ||
| * only when we hit this many executions of the task. If unspecified, all events will be sent. | ||
| * @param logLevel - optional {@link LogLevel} for events emitted by this performance event. | ||
| * If unspecified, {@link @fluidframework/core-interfaces#LogLevelConst.essential | LogLevel.essential} will be used. | ||
| * @returns The results of the executed task | ||
|
MarioJGMsoft marked this conversation as resolved.
|
||
| * | ||
| * @remarks Note that if the "same" event (category + eventName) would be emitted by different | ||
|
|
@@ -733,7 +735,7 @@ export class PerformanceEvent { | |
| callback: (event: PerformanceEvent) => T, | ||
| markers?: IPerformanceEventMarkers, | ||
| sampleThreshold: number = 1, | ||
| logLevel?: typeof LogLevel.verbose | typeof LogLevel.info, | ||
| ): T { | ||
| const perfEvent = PerformanceEvent.start( | ||
| logger, | ||
|
|
@@ -762,6 +764,7 @@ export class PerformanceEvent { | |
| * @param sampleThreshold - events with the same name and category will be sent to the logger | ||
| * only when we hit this many executions of the task. If unspecified, all events will be sent. | ||
| * @param logLevel - optional {@link LogLevel} for events emitted by this performance event. | ||
| * If unspecified, {@link @fluidframework/core-interfaces#LogLevelConst.essential | LogLevel.essential} will be used. | ||
| * @returns The results of the executed task | ||
| * | ||
| * @remarks Note that if the "same" event (category + eventName) would be emitted by different | ||
|
|
@@ -775,7 +778,7 @@ export class PerformanceEvent { | |
| callback: (event: PerformanceEvent) => Promise<T>, | ||
| markers?: IPerformanceEventMarkers, | ||
| sampleThreshold: number = 1, | ||
| logLevel?: typeof LogLevel.verbose | typeof LogLevel.info, | ||
| ): Promise<T> { | ||
| const perfEvent = PerformanceEvent.start( | ||
| logger, | ||
|
|
@@ -802,12 +805,12 @@ export class PerformanceEvent { | |
| private readonly startTime = performanceNow(); | ||
| private startMark?: string; | ||
|
|
||
| private constructor( | ||
| private readonly logger: TelemetryLoggerExt, | ||
| event: ITelemetryGenericEventExt, | ||
| private readonly markers: IPerformanceEventMarkers = { end: true, cancel: "generic" }, | ||
| private readonly emitLogs: boolean = true, | ||
| private readonly logLevel: typeof LogLevel.verbose | typeof LogLevel.info | undefined, | ||
| ) { | ||
| this.event = { ...event }; | ||
| if (this.markers.start) { | ||
|
|
||
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
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.
Applied change