feat(core-interfaces): Add info and essential log levels to LogLevel#26947
feat(core-interfaces): Add info and essential log levels to LogLevel#26947MarioJGMsoft merged 24 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 |
Josmithr
left a comment
There was a problem hiding this comment.
Docs and API changes look good!
…icrosoft#26947) ## Description Adds two new named log levels to the `LogLevel` constant 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 `default` and `error` entries are preserved with their original numeric values but marked `@deprecated`, pointing callers to`info` and `essential` respectively. Because the numeric values are unchanged, existing callers that pass `LogLevel.default` or `LogLevel.error` will continue to work without modification. Internal usages of the deprecated values have been updated; call sites now reference `LogLevel.info` and `LogLevel.essential` directly. ### Expected Future Timeline Removal of `LogLevel.default` and `LogLevel.error` is tracked in microsoft#26969 (expected in v3.0). ## Reviewer Guidance The review process is outlined on [this wiki page](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines). - The numeric values of the deprecated entries (`default: 20`, `error: 30`) are unchanged, so existing callers are unaffected. - No logger implementations are updated in this PR — only the `LogLevel` constant is extended. - **Note:** The `@deprecated` tags on `default` and `error` are 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.
…ogLevel" (microsoft#27005) Reverts microsoft#26947 because it broke the integration pipeline. Need to stage the change somehow.
…icrosoft#26947) ## Description Adds two new named log levels to the `LogLevel` constant 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 `default` and `error` entries are preserved with their original numeric values but marked `@deprecated`, pointing callers to`info` and `essential` respectively. Because the numeric values are unchanged, existing callers that pass `LogLevel.default` or `LogLevel.error` will continue to work without modification. Internal usages of the deprecated values have been updated; call sites now reference `LogLevel.info` and `LogLevel.essential` directly. ### Expected Future Timeline Removal of `LogLevel.default` and `LogLevel.error` is tracked in microsoft#26969 (expected in v3.0). ## Reviewer Guidance The review process is outlined on [this wiki page](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines). - The numeric values of the deprecated entries (`default: 20`, `error: 30`) are unchanged, so existing callers are unaffected. - No logger implementations are updated in this PR — only the `LogLevel` constant is extended. - **Note:** The `@deprecated` tags on `default` and `error` are 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.
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.