Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace logger with taggedLogger #8191

Closed
Tracked by #8005
scottn12 opened this issue Nov 9, 2021 · 2 comments · Fixed by #8886
Closed
Tracked by #8005

Replace logger with taggedLogger #8191

scottn12 opened this issue Nov 9, 2021 · 2 comments · Fixed by #8886
Assignees
Labels
api deprecation Changes to a deprecated API
Milestone

Comments

@scottn12
Copy link
Contributor

scottn12 commented Nov 9, 2021

This issue aims to replace the vanilla logger with taggedLogger in the IContainerContext interface. The logger property was deprecated in release 0.45.

The files affected are:

  1. common\lib\container-definitions\src\runtime.ts
  2. packages\loader\container-loader\src\containerContext.ts
  3. packages\runtime\container-runtime\src\containerRuntime.ts
@ChumpChief
Copy link
Contributor

This seems fine to me, with one of two approaches, @markfields in case he has more context/feedback to help choose which.

  1. If we think there's risk that some old loader is trying to load modern containers (and also that it's a scenario we need to support), then a reasonable approach might be:
    • Remove the property from the interface and ContainerContext, but in ContainerRuntime continue to do a last-ditch TaggedLoggerAdapter fallback
    • Then log a bug for removing the adapter usage from ContainerRuntime and the TaggedLoggerAdapter class, including an explanation of when we think it will be safe to do (and/or what metric will inform that decision).
  2. If we don't think there's risk, then we should just go ahead and remove it in all locations (including removing the TaggedLoggerAdapter class.

@markfields
Copy link
Member

markfields commented Dec 3, 2021

Let's go with 1. Reason being, even though there are only a hundred or so clients with very recent runtimes and pre-0.45 loaders in the last month, just 1 will result in bogus columns getting added to our internal data tables (for the tag and value). And worse, if someone does add privacy-impacting data and tag it, we would have to do a whole thing to clean it up.

So in this case, let's wait til there are 0 loaders pre-0.45, even if it takes a year or more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api deprecation Changes to a deprecated API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants