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

[logging] ensure isEnabledFor implements the right semantics #2292

Open
vchudnov-g opened this issue Dec 10, 2024 · 1 comment
Open

[logging] ensure isEnabledFor implements the right semantics #2292

vchudnov-g opened this issue Dec 10, 2024 · 1 comment
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@vchudnov-g
Copy link
Contributor

vchudnov-g commented Dec 10, 2024

We are guarding logging statements inside a check for _LOGGER.isEnabledFor(LEVEL) where LEVEL is typically (for now) std_logging.DEBUG, though we may use other levels, such as INFO, WARNING, in some messages.

As per the official docs, isEnabledFor and a function it calls, getEffectiveLevel, check whether a certain severity is handled by the current logger, and only traverse up the hierarchy if the logger's severity is NOTSET.

This does not seem to be the semantics we intend. If the current _LOGGER is set to level INFO, say, but propagation is on, and the parent logger is set to level DEBUG, we want _LOGGER.debug() statements to not be handled by _LOGGER but to be handled by its parent. This means the logging statements should execute, which means the guard statements should evaluate to true, which means we need a different function than isEnabledFor.

This is also relevant to the internal issue b/382299158 for logging encapsulation.

@vchudnov-g vchudnov-g added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Dec 10, 2024
@parthea parthea added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Dec 19, 2024
@parthea
Copy link
Contributor

parthea commented Dec 19, 2024

Bumping to P2. This is part of the logging feature but it does not block releases. As per the definition, Moderately-important priority. Fix may not be included in next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

2 participants