Repository navigation
Conversation
Pull request dashboard statusWaiting on reviewers · refreshed 2026-10-22 00:55 UTC Review the latest changes. Status above doesn't look right?
|
9049ca9 to
ce077e0
Compare
Add minimum-severity and trace-based filtering to Logger.enabled() and wire both options through declarative configuration.
ce077e0 to
5ee6a84
Compare
|
@djbell261 we need you to sign the CLA please |
This comment has been minimized.
This comment has been minimized.
Thanks! I’ve signed the CLA. The EasyCLA check is passing now. |
chrikrah
left a comment
There was a problem hiding this comment.
@ocelotl @lzchen, a scope question on this one. At head 5ee6a84, Logger.enabled() honors minimum_severity and trace_based, but Logger.emit() still checks only _is_enabled(). LoggingHandler.emit calls logger.emit without asking enabled() first. On that path, which most Python users are on, both records still reach every processor.
$ python -I otel_min_sev_emit.py # opentelemetry-sdk installed from head 5ee6a84, Python 3.12.3
enabled(INFO) False
enabled(unsampled) False
reached processor: ['info below minimum', 'error in unsampled trace']
$ python -m pytest -q opentelemetry-sdk/tests/logs/test_logs.py
37 passed in 0.33s
The probe sets _LoggerConfig(minimum_severity=SeverityNumber.WARN, trace_based=True) and emits one INFO record, then one ERROR record under a NonRecordingSpan with TraceFlags(0).
blocking: the PR also removes the _to_logger_config warning that says these two fields are ignored. Without it, declarative config accepts minimum_severity and still exports everything through LoggingHandler. The spec's emit section (Development status) says the Logger MUST drop such records. Separately, #4765 (rads-1996, on hold) filters these two fields on the emit path as a processor.
I'd keep the warning until emit drops these records. That holds this PR to the enabled() scope of #5525. The cost is one more release where the setting affects enabled() only. cc @djbell261. Should the emit-path drop land here, through #4765, or in a follow-up?
This comment has been minimized.
This comment has been minimized.
Restore warnings for unsupported declarative minimum_severity and trace_based configuration until emit-time filtering is implemented.
|
Thanks for catching this. I agree that accepting these fields through declarative configuration would be misleading while Logger.emit() does not enforce them. |
chrikrah
left a comment
There was a problem hiding this comment.
@djbell261 approving at 65aff4e. With the _to_logger_config warning back, declarative config again says that minimum_severity and trace_based are ignored, so my blocking point is gone. The PR now matches the enabled() scope of #5525.
$ python -m pytest -q opentelemetry-sdk/tests/logs/test_logs.py # export of 65aff4e, opentelemetry.sdk._logs imported from it, Python 3.12
37 passed
6 failed, 31 passed # _logs/_internal/__init__.py from base 567027b, your tests kept
$ cd opentelemetry-configuration && python -m pytest -q tests/test_logger_provider.py
48 passed
$ git diff --quiet 567027b 65aff4e -- opentelemetry-configuration; echo $?
0 # the config package is unchanged from base 567027b
# not run: tox lint, the full sdk suite
non-blocking: _LoggerConfig gains both fields, but only code that builds one by hand can set them, because _to_logger_config still drops them. That is the right order. The emit-path drop then has one place to land when it comes, through #4765 or after it. @lzchen, from my side this is ready to merge.
Description
Add minimum-severity and trace-based filtering to Logger.enabled().
Fixes #5525
Type of change
New feature (non-breaking change which adds functionality)
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
uv run tox -e py314-test-opentelemetry-sdkuv run tox -e py314-test-opentelemetry-configurationuv run tox -e precommituv run tox -e typecheck -x 'testenv:typecheck.base_python=python3.13'Does This PR Require a Contrib Repo Change?
Checklist: