Skip to content

feat(core-interfaces): Add info and essential log levels to LogLevel#26947

Draft
MarioJGMsoft wants to merge 23 commits intomicrosoft:mainfrom
MarioJGMsoft:marioja/LogLevelBaseUpdate
Draft

feat(core-interfaces): Add info and essential log levels to LogLevel#26947
MarioJGMsoft wants to merge 23 commits intomicrosoft:mainfrom
MarioJGMsoft:marioja/LogLevelBaseUpdate

Conversation

@MarioJGMsoft
Copy link
Copy Markdown
Contributor

@MarioJGMsoft MarioJGMsoft commented Apr 6, 2026

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 toinfo 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 #26969 (expected in v3.0).

Reviewer Guidance

The review process is outlined on this wiki page.

  • 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.

Copilot AI review requested due to automatic review settings April 6, 2026 22:40
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Extends @fluidframework/core-interfacesLogLevel constant to introduce clearer named levels (info, essential) while keeping backward compatibility via deprecated aliases (default, error).

Changes:

  • Add LogLevel.info (20) and LogLevel.essential (30) with expanded documentation.
  • Deprecate LogLevel.default and LogLevel.error while preserving their numeric values.

@MarioJGMsoft MarioJGMsoft requested a review from a team as a code owner April 6, 2026 22:56
Copy link
Copy Markdown
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't trigger a lot of deprecation warnings/errors in FF codebase?
I would expect this change to retain more of the changes you had in #26789. How much of that can you keep without changing ITelemetryLoggerExt?

/**
* Value to assume when not otherwise given.
* @see {@link (LogLevel:variable).info}
* @deprecated Use {@link (LogLevel:variable).info} to preserve prior treatment.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied change

*/
public send(event: ITelemetryBaseEvent, logLevel?: LogLevel): void {
if ((logLevel ?? LogLevel.default) >= this.minLogLevel) {
if ((logLevel ?? LogLevel.essential) >= this.minLogLevel) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this as info for this PR to avoid behavior change, and include this one-line change in a follow-up

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied change

event: ITelemetryPerformanceEventExt,
error?: unknown,
logLevel: typeof LogLevel.verbose | typeof LogLevel.default = LogLevel.default,
logLevel?: typeof LogLevel.verbose | typeof LogLevel.info,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're going to restore this default value of info right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I restored it

configValue: JSON.stringify(parsed),
}),
},
LogLevel.essential,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave this behavior change out of this PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied change

content.details = event.details;
}
log(`[${logLevel ?? LogLevel.default}]`, content);
log(`[${logLevel ?? "unspecified"}]`, content);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary? What's the intent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this change needs to go in this PR because it's using LogLevel.default

eventName: "DeviceSpec",
...getDeviceSpec(),
},
LogLevel.essential,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid any behavior change in this PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied change

count: blobManagerLoadInfo.ids?.length ?? 0,
redirectTable: blobManagerLoadInfo.redirectTable?.length,
},
undefined,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add comment with param name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied

message:
"Legacy document, SharedMap is masquerading as SharedDirectory in DataObject",
},
LogLevel.essential,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior change - do in subsequent PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied change

Copy link
Copy Markdown
Member

@markfields markfields left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MarioJGMsoft MarioJGMsoft marked this pull request as ready for review April 9, 2026 22:50
@MarioJGMsoft MarioJGMsoft requested a review from a team as a code owner April 9, 2026 22:50
@MarioJGMsoft MarioJGMsoft marked this pull request as draft April 9, 2026 22:50
* @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.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@markfields markfields left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  278943 links
    1879 destination URLs
    2124 URLs ignored
       0 warnings
       0 errors


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants