Skip to content

Implement filtering logic for min_severity and trace_based parameters#4765

Open
rads-1996 wants to merge 6 commits intoopen-telemetry:mainfrom
rads-1996:trace-based-sampling
Open

Implement filtering logic for min_severity and trace_based parameters#4765
rads-1996 wants to merge 6 commits intoopen-telemetry:mainfrom
rads-1996:trace-based-sampling

Conversation

@rads-1996
Copy link
Copy Markdown
Contributor

@rads-1996 rads-1996 commented Oct 6, 2025

Description

Implement Spec to filter logs based on min_severity and trace_based parameters.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

  • Added unit tests

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@rads-1996 rads-1996 requested a review from a team as a code owner October 6, 2025 22:10
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py Outdated
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py Outdated
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py Outdated
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py Outdated
@rads-1996 rads-1996 force-pushed the trace-based-sampling branch from e61189d to 4f3ee75 Compare October 20, 2025 18:11
@rads-1996 rads-1996 force-pushed the trace-based-sampling branch from c209824 to 6bef931 Compare November 5, 2025 20:02
@rads-1996 rads-1996 force-pushed the trace-based-sampling branch from c1c27e2 to 7eab50b Compare December 10, 2025 20:03
@rads-1996 rads-1996 marked this pull request as draft December 10, 2025 20:25
@rads-1996 rads-1996 force-pushed the trace-based-sampling branch from c55426d to 6ee2697 Compare December 11, 2025 00:42
@rads-1996 rads-1996 marked this pull request as ready for review December 11, 2025 00:57
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 4, 2026

This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment.
If you're still working on this, please add a comment or push new commits.

@github-actions github-actions Bot added the Stale label Mar 4, 2026
@rads-1996
Copy link
Copy Markdown
Contributor Author

In progress

@github-actions github-actions Bot removed the Stale label Mar 5, 2026
@github-actions
Copy link
Copy Markdown

This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment.
If you're still working on this, please add a comment or push new commits.

@github-actions github-actions Bot added the Stale label Mar 19, 2026
@rads-1996
Copy link
Copy Markdown
Contributor Author

Waiting for logs stabilization to complete. Will bring this up in the SIG once that is done.

@github-actions github-actions Bot removed the Stale label Mar 20, 2026
@MikeGoldsmith
Copy link
Copy Markdown
Member

Thanks for the work on this @rads-1996. A few notes now that the referenced spec PR merged in September 2025:

  • Architectural question: The spec added minimum_severity and trace_based as logger configuration parameters, but this implements them as a wrapping FilteringLogRecordProcessor. Worth confirming with the SIG whether the processor approach is the intended implementation strategy, or whether these should be configurable on the Logger/LoggerProvider directly.

  • Potential bug: is_less_than_minimum_severity_level only guards against record.severity_number is None, but SeverityNumber.UNSPECIFIED (value 0) is not None. If a record has severity_number=UNSPECIFIED and minimum_severity_level=DEBUG (value 5), 0 < 5 is True so the record gets dropped — but the docstring says UNSPECIFIED should bypass filtering. There's no test covering this case.

Please can someone from @open-telemetry/python-approvers add a hold label to keep the stale bot off this while logs stabilization is in progress.

@pmcollins pmcollins added the hold label Apr 2, 2026
@rads-1996
Copy link
Copy Markdown
Contributor Author

Thanks for the work on this @rads-1996. A few notes now that the referenced spec PR merged in September 2025:

  • Architectural question: The spec added minimum_severity and trace_based as logger configuration parameters, but this implements them as a wrapping FilteringLogRecordProcessor. Worth confirming with the SIG whether the processor approach is the intended implementation strategy, or whether these should be configurable on the Logger/LoggerProvider directly.
  • Potential bug: is_less_than_minimum_severity_level only guards against record.severity_number is None, but SeverityNumber.UNSPECIFIED (value 0) is not None. If a record has severity_number=UNSPECIFIED and minimum_severity_level=DEBUG (value 5), 0 < 5 is True so the record gets dropped — but the docstring says UNSPECIFIED should bypass filtering. There's no test covering this case.

Please can someone from @open-telemetry/python-approvers add a hold label to keep the stale bot off this while logs stabilization is in progress.

Thank you for taking a look at this. Initially I had implemented this with these configs directly on the Logger, but in the SIG it was suggested to go with this approach. I can bring it up again to see what folks think.

Gotcha, will take care of it. As previously discussed, this PR will be merged after the logs are stabilized, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

7 participants