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

[API] Introduce a new global level api.diag for internal Diagnostic Logging (Part 2) #1878

Closed
MSNev opened this issue Jan 27, 2021 · 3 comments

Comments

@MSNev
Copy link
Contributor

MSNev commented Jan 27, 2021

Introduction of new api.diag global logger as discussed during SIG meetings and linked to #1754 and Part 1 #1877

Basic outline

  • Provide a top-level diagnostic logger from the API as api.diag with methods like api.diag.warn, api.diag.debug, etc.
  • diag logger is no-op by default
  • register a default logging implementation by calling api.diag.setLogger(loggingImpl)

Part 2

Will include breaking changes

  • Once contrib is changed to use api.diag, these exports will be removed, this will require that they stop using local Logger instances
  • This will also have breaking changes for
    • Configuration definitions that use api.Logger and core.LogLevel
    • Functions that currently get or pass api.Logger instances
    • The type of the environment OTEL_LOG_LEVEL
@MSNev
Copy link
Contributor Author

MSNev commented Jan 27, 2021

Please assign to me as I can't assign it myself

This Part 2, will also wait until Part 1 #1877 is complete, released and the contrib repo updated

@dyladan
Copy link
Member

dyladan commented Feb 16, 2021

@MSNev @obecny and I had a quick call about the diag logger. documenting decisions here for future reference:

  1. Should we keep the log levels or reduce to just console levels?

Error, warn, info, debug, verbose, none/all

  1. Do we want to pass loggers as part of the config of each component?

No. For now, components will call the global logger. This removes the OptionalDiagLogger and the getDiagLoggerFromConfig helpers. The logging global error handler will go to api.diag.error

  1. Do we want to create a base class with setLogger, logger getter helpers? Do we want to have a component namespace for logging a single component?

We are going to leave this for a future enhancement. Existing getters for the logger will be changed to point to the api.diag logger. Other classes which don’t have existing getters will be changed to use api.diag directly at the point of use.

  1. Keep the existing numbering from the existing PR to keep space between log levels to allow additional levels to be added in the future. This will break numeric compatibility with the old logger.

@MSNev
Copy link
Contributor Author

MSNev commented Feb 18, 2021

This is included in v0.17

@MSNev MSNev closed this as completed Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants