refactor(container-runtime): Tighten up exposure of ContainerRuntime's logger#20839
Merged
Conversation
github-actions
Bot
added
area: framework
Collaborator
⯅ @fluid-example/bundle-size-tests: +385 Bytes
Baseline commit: a5f84ed |
Josmithr
reviewed
Apr 30, 2024
Josmithr
reviewed
Apr 30, 2024
Josmithr
reviewed
Apr 30, 2024
markfields
reviewed
May 13, 2024
markfields
approved these changes
May 13, 2024
chentong7
force-pushed
the
container-runtime
branch
from
8d2cad9 to
929e123
Compare
alexvy86
reviewed
May 14, 2024
Contributor
alexvy86
left a comment
alexvy86
left a comment
There was a problem hiding this comment.
I'm still uneasy with the changes here. Didn't we decide to update it to be completely non-breaking to anything? Just add the new baseLogger property where it makes sense and deprecate the old one, pointing users to use the new one? There's still plenty of alpha types that are getting breaking changes here.
| "typeValidation": { | ||
| "broken": { | ||
| "InterfaceDeclaration_IDataObjectProps": { | ||
| "forwardCompat": false, |
Contributor
There was a problem hiding this comment.
Revert since we don't have changes in this package.
Contributor
Author
There was a problem hiding this comment.
Seems like it does affected this package. Type error was thrown after rebasing from main
Contributor
There was a problem hiding this comment.
Oh, because the interface has a property of a type that we're breaking... so back to my general comment on the last review, I thought we had decided to make all changes here non-breaking?
chentong7
force-pushed
the
container-runtime
branch
from
b9073b9 to
c4cf077
Compare
kekachmar
pushed a commit
to kekachmar/FluidFramework
that referenced
this pull request
May 21, 2024
…s logger (microsoft#20839) ## Description ContainerRuntime.logger is currently public, though ideally, it should not be, and it uses an internal type instead of the preferable base type. When sharing the logger, the standard practice should be to pass around the base logger, allowing each consumer to create a ChildLogger from it. ## Changes * Rename ContainerRuntime.logger to ContainerRuntime.baseLogger, change its type to ITelemetryBaseLogger, and set its access level to private. * Adjust the dependencies that receive the logger to accept the ITelemetryBaseLogger type and utilize createChildLogger to create a child logger as needed (this operation will be a no-op if the logger is already a child logger). * Modify the following classes in Fluid Framework (FF) so they no longer directly access the logger from the runtime but instead receive their own base logger via constructor parameters: BlobManager Summarizer DataStoreContext * DataStoreContext currently makes the same logger available through its public API. This may be unnecessary—as indicated by its lack of use in the FF codebase—especially since IFluidDataStoreRuntime already exposes a logger. This exposure could potentially be removed to streamline the API and enhance encapsulation.
4 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Description
ContainerRuntime.logger is currently public, though ideally, it should not be, and it uses an internal type instead of the preferable base type. When sharing the logger, the standard practice should be to pass around the base logger, allowing each consumer to create a ChildLogger from it.
Changes
Rename ContainerRuntime.logger to ContainerRuntime.baseLogger, change its type to ITelemetryBaseLogger, and set its access level to private.
Adjust the dependencies that receive the logger to accept the ITelemetryBaseLogger type and utilize createChildLogger to create a child logger as needed (this operation will be a no-op if the logger is already a child logger).
Modify the following classes in Fluid Framework (FF) so they no longer directly access the logger from the runtime but instead receive their own base logger via constructor parameters:
BlobManager
Summarizer
DataStoreContext