feat(core-interfaces): Add info and essential log levels to LogLevel#26947
feat(core-interfaces): Add info and essential log levels to LogLevel#26947MarioJGMsoft wants to merge 23 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Extends @fluidframework/core-interfaces’ LogLevel constant to introduce clearer named levels (info, essential) while keeping backward compatibility via deprecated aliases (default, error).
Changes:
- Add
LogLevel.info(20) andLogLevel.essential(30) with expanded documentation. - Deprecate
LogLevel.defaultandLogLevel.errorwhile preserving their numeric values.
| /** | ||
| * Value to assume when not otherwise given. | ||
| * @see {@link (LogLevel:variable).info} | ||
| * @deprecated Use {@link (LogLevel:variable).info} to preserve prior treatment. |
There was a problem hiding this comment.
Deprecations should have an associated issue. These look like they should be associated with 3.0 breaks to note removal timeframe.
See https://github.com/microsoft/FluidFramework/wiki/API-Deprecation
There was a problem hiding this comment.
Updated the file to follow the deprecation docs properly.
| private shouldFilterOutEvent(event: ITelemetryBaseEvent, logLevel?: LogLevel): boolean { | ||
| const eventLogLevel = logLevel ?? LogLevel.default; | ||
| const configLogLevel = this.baseLogger.minLogLevel ?? LogLevel.default; | ||
| const eventLogLevel = logLevel ?? LogLevel.essential; |
There was a problem hiding this comment.
Let's leave this as info in this PR so there's no behavior change. A follow-up PR can make the one-line switch.
| */ | ||
| public send(event: ITelemetryBaseEvent, logLevel?: LogLevel): void { | ||
| if ((logLevel ?? LogLevel.default) >= this.minLogLevel) { | ||
| if ((logLevel ?? LogLevel.essential) >= this.minLogLevel) { |
There was a problem hiding this comment.
Let's leave this as info for this PR to avoid behavior change, and include this one-line change in a follow-up
| event: ITelemetryPerformanceEventExt, | ||
| error?: unknown, | ||
| logLevel: typeof LogLevel.verbose | typeof LogLevel.default = LogLevel.default, | ||
| logLevel?: typeof LogLevel.verbose | typeof LogLevel.info, |
There was a problem hiding this comment.
You're going to restore this default value of info right?
There was a problem hiding this comment.
Yes, I restored it
| configValue: JSON.stringify(parsed), | ||
| }), | ||
| }, | ||
| LogLevel.essential, |
There was a problem hiding this comment.
I'd leave this behavior change out of this PR
| content.details = event.details; | ||
| } | ||
| log(`[${logLevel ?? LogLevel.default}]`, content); | ||
| log(`[${logLevel ?? "unspecified"}]`, content); |
There was a problem hiding this comment.
Is this change necessary? What's the intent?
There was a problem hiding this comment.
This was something that was brought up in the previous PR, where Jason pointed out that this change could be done: Original comment. I'll make it's own PR for it
There was a problem hiding this comment.
Actually, this change needs to go in this PR because it's using LogLevel.default
| eventName: "DeviceSpec", | ||
| ...getDeviceSpec(), | ||
| }, | ||
| LogLevel.essential, |
There was a problem hiding this comment.
I'd avoid any behavior change in this PR
| count: blobManagerLoadInfo.ids?.length ?? 0, | ||
| redirectTable: blobManagerLoadInfo.redirectTable?.length, | ||
| }, | ||
| undefined, |
There was a problem hiding this comment.
nit: add comment with param name
| message: | ||
| "Legacy document, SharedMap is masquerading as SharedDirectory in DataObject", | ||
| }, | ||
| LogLevel.essential, |
There was a problem hiding this comment.
Behavior change - do in subsequent PR
markfields
left a comment
There was a problem hiding this comment.
Once you remove all behavior changes and leave this PR as rename-only (including updating function signatures is ok) I'll approve. Then a very small scoped PR with all the behavior changes can come right after and we can more easily reason over those changes.
| * @param logLevel - optional level of the log. It category of event is set as error, | ||
| * then the logLevel will be upgraded to be an error. | ||
| * | ||
| * @remarks The default value for logLevel will be updated to {@link @fluidframework/core-interfaces#LogLevel.essential} once issue #26910 is resolved. |
There was a problem hiding this comment.
I really believe we can update the default of the implementation without touching the API, and thus don't need to wait on that issue. But I don't mind if you put this comment here anyway, just saying.
There was a problem hiding this comment.
I removed the issue part of the comment but I still think I should leave the remark to let people know that the change is happening and they should be aware of it.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Description
Adds two new named log levels to the
LogLevelconstant in@fluidframework/core-interfaces:info(20) — Session-level information that may be omitted in some sessions if needed (e.g. to reduce overall telemetry volume), but if any are collected in a session, all should be.essential(30) — Essential information about the operation of Fluid that should always be collected, even in production, for diagnostic purposes.The existing
defaultanderrorentries are preserved with their original numeric values but marked@deprecated, pointing callers toinfoandessentialrespectively. Because the numeric values are unchanged, existing callers that passLogLevel.defaultorLogLevel.errorwill continue to work without modification.Internal usages of the deprecated values have been updated; call sites now reference
LogLevel.infoandLogLevel.essentialdirectly.Expected Future Timeline
Removal of
LogLevel.defaultandLogLevel.erroris tracked in #26969 (expected in v3.0).Reviewer Guidance
The review process is outlined on this wiki page.
default: 20,error: 30) are unchanged, so existing callers are unaffected.LogLevelconstant is extended.@deprecatedtags ondefaultanderrorare not currently being picked up by the API reports, but they are detected by IntelliSense. Reviewers should keep this in mind when evaluating the API surface changes.