feat(telemetry-utils): deprecate LogLevel.default/error and tag unspecified events as essential#27207
feat(telemetry-utils): deprecate LogLevel.default/error and tag unspecified events as essential#27207MarioJGMsoft wants to merge 23 commits intomicrosoft:mainfrom
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (102 lines, 13 files), I've queued these reviewers:
Toggle the reviewer checkboxes above to adjust, then tick the box below to start:
|
There was a problem hiding this comment.
Pull request overview
This PR updates Fluid’s telemetry log-level semantics by deprecating legacy LogLevel members and changing how ChildLogger treats events with no explicit logLevel, to better distinguish “essential” telemetry.
Changes:
- Deprecates
LogLevel.defaultandLogLevel.error, and migrates in-repo usages toLogLevel.info/LogLevel.essential. - Changes
ChildLoggerto treat events with no explicitlogLevelasessential, including forwardingessentialto the base logger. - Updates related types/docs, tests, API reports, and adds changesets documenting the behavior change.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/telemetry-utils/src/test/multiSinkLogger.spec.ts | Updates tests to use info/essential instead of deprecated default/error. |
| packages/utils/telemetry-utils/src/test/childLogger.spec.ts | Updates log-level constants in tests; should be extended to cover the new “unspecified logLevel ⇒ essential” behavior. |
| packages/utils/telemetry-utils/src/telemetryTypes.ts | Updates logLevel unions to use info; updates TSDoc (currently inconsistent with new runtime defaults). |
| packages/utils/telemetry-utils/src/mockLogger.ts | Updates MockLogger default min level / filtering fallback from default to info. |
| packages/utils/telemetry-utils/src/logger.ts | Removes parameter defaults, changes ChildLogger fallback/forwarding to essential, updates MultiSink defaults, and adds PerformanceEvent docs (currently inaccurate). |
| packages/utils/telemetry-utils/api-report/telemetry-utils.legacy.beta.api.md | API report update reflecting new logLevel unions. |
| packages/loader/container-loader/src/test/container.spec.ts | Import ordering/type-only import adjustment. |
| packages/common/core-interfaces/src/logger.ts | Adds @deprecated tags for LogLevelConst.default/error and updates base logger docs to default to info. |
| packages/common/core-interfaces/api-report/core-interfaces.public.api.md | API report reflects deprecated members. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.public.api.md | API report reflects deprecated members. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.beta.api.md | API report reflects deprecated members. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md | API report reflects deprecated members. |
| packages/common/core-interfaces/api-report/core-interfaces.beta.api.md | API report reflects deprecated members. |
| .changeset/soft-doors-trade.md | Changeset documenting the ChildLogger unspecified-logLevel behavior change and downstream impact. |
| .changeset/salty-colts-appear.md | Changeset documenting the LogLevel.default/error deprecations and migration guidance. |
| TypedEventEmitter, | ||
| type IProvideLayerCompatDetails, | ||
| } from "@fluid-internal/client-utils"; | ||
| import { AttachState, type IAudience } from "@fluidframework/container-definitions"; |
There was a problem hiding this comment.
It didn't feel big enough to deserve it's own PR, so I decided to add it here, but I can definitely split it off for it's own thing.
| * Log a telemetry event, if it meets the appropriate log-level threshold (see {@link ITelemetryBaseLogger.minLogLevel}). | ||
| * @param event - The event to log. | ||
| * @param logLevel - The log level of the event. Default: {@link LogLevelConst.default | LogLevel.default}. | ||
| * @param logLevel - The log level of the event. Default: {@link LogLevelConst.info | LogLevel.info}. |
There was a problem hiding this comment.
Isn't the default supposed to be essential? Maybe it's clearer if we say it like "suggested interpretation of undefined: LogLevel.essential"? Since this is an interface so claiming a "default value" is kind of in vain, it could be implemented any way.
| logLevel: typeof LogLevel.verbose | typeof LogLevel.default = LogLevel.default, | ||
| logLevel: | ||
| | typeof LogLevel.verbose | ||
| | typeof LogLevel.info | ||
| | typeof LogLevel.essential = LogLevel.essential, |
There was a problem hiding this comment.
These cannot be changes to be different from the definition in TelemetryLoggerExt and ITelemetryLoggerExt.
TypeScript's silly function variance allowance strikes again. :(
There was a problem hiding this comment.
Applied change, should work properly now
Removes 11 zero-byte files added by mistake in 2c13181. These are bind-mounted from the dev container host (shell rc files, IDE config, MCP config) and don't belong in the repo.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
2 similar comments
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Description
Tracks issue #26969 (planned removal in v3.0).
This PR has two parts:
1. Deprecate
LogLevel.defaultandLogLevel.errorAdds
@deprecatedtags onLogLevel.defaultandLogLevel.errorand migratesall in-repo callsites:
LogLevel.default→LogLevel.info(numerically identical, value20)LogLevel.error→LogLevel.essential(numerically identical, value30)Affected files include
telemetry-utils(logger.ts,mockLogger.ts,telemetryTypes.ts, and the related tests) and the corresponding API reports.2. Treat events with no explicit
logLevelasessentialinChildLoggerTwo related changes in
packages/utils/telemetry-utils/src/logger.ts:ChildLogger.shouldFilterOutEventnow falls back toLogLevel.essential(30)when an event arrives without a
logLevel(previouslyLogLevel.default=20).ChildLogger.sendnow forwardsLogLevel.essentialto the base logger whenno
logLevelwas supplied, instead of forwardingundefined.Additionally, the parameter defaults on
TelemetryLogger.sendTelemetryEventandsendPerformanceEventwere dropped (they were= LogLevel.default). Callersthat don't pass a
logLevelnow propagateundefinedthrough the chain, whereChildLoggerapplies the newessentialfallback above.3.
createSampledLoggerandMultiSinkLogger.sendnow forwarderrorandlogLevelTwo pre-existing bugs surfaced while wiring up the
essentialfallback above:createSampledLogger(packages/utils/telemetry-utils/src/utils.ts)silently dropped
errorandlogLevelarguments before delegating to thewrapped logger — the wrapper's method signatures didn't even accept those
arguments. The wrapper now accepts and forwards them:
sendacceptslogLevel?sendTelemetryEventacceptserror?andlogLevel?(verbose | info)sendErrorEventacceptserror?sendPerformanceEventacceptserror?andlogLevel?(verbose | info)MultiSinkLogger.send(packages/utils/telemetry-utils/src/logger.ts)had the same bug —
logLevelwas dropped before fanning out to sinks. It nowaccepts
logLevel?and forwards it to every registered sink (defaulting toLogLevel.essentialwhen omitted, consistent with the part 2 behavior).New test coverage:
utils.spec.ts—describe("logLevel forwarding")verifies the sampledlogger forwards
logLevelthroughsend,sendTelemetryEvent, andsendPerformanceEvent, and propagatesundefinedwhen the caller omits it.multiSinkLogger.spec.ts—describe("logLevel propagation")verifies thedefault
LogLevel.essentialfallback, explicit-level forwarding for bothsendTelemetryEventandsendPerformanceEvent, and includes a regressiontest for the
MultiSinkLogger.senddrop bug.childLogger.spec.ts—describe("logLevel forwarding")verifiesChildLoggerforwards both theessentialdefault and explicit levels tothe underlying sink.
Misc
typeimport ordering incontainer.spec.ts.Behavior change for downstream consumers
Although no public API is removed in this PR, downstream consumers that filter
telemetry by numeric level should be aware:
Internal Fluid events that were previously emitted to the base logger with
logLevel = undefined(effectively treated as20) will now arrive tagged as30(essential). If a host application starts droppinglogLevel = 20eventsas "non-essential", it must ensure all running Fluid versions include this
change — older Fluid versions still send those same critical events at level
20, and dropping them would lose telemetry.Breaking Changes
No public API surface is removed. See #26969 for the planned v3.0 removal of
LogLevel.defaultandLogLevel.error. See the "Behavior change for downstreamconsumers" section above for a runtime behavior note.
Reviewer Guidance
The review process is outlined on this wiki page.
Specific asks:
LogLevel.essentialfallback inChildLogger.shouldFilterOutEventand
ChildLogger.sendis the intended semantic (vs. leavingundefinedtoflow to the base logger).
= LogLevel.defaultparameter defaults onsendTelemetryEvent/sendPerformanceEventis acceptable for consumers whomay have relied on the implicit default.
createSampledLoggerandMultiSinkLogger.sendargumentforwarding fix is acceptable as part of this PR (it changes observable
behavior for hosts wrapping with these helpers).