Skip to content

Fix SeverityProcessor example #4541

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.

Sign up for GitHub

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

Closed
pellared wants to merge 3 commits into from
Closed

Fix SeverityProcessor example #4541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
05c28c6
Fix SeverityProcessor example
pellared Jun 4, 2025
f383772
Update PR number
pellared Jun 4, 2025
a64d153
Merge branch 'main' into fix-SeverityProcessor
pellared Jun 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ release.

### Logs

- Fix the `SeverityProcessor` example implementation so that it filters out only
the records with `SeverityNumber` in the 1..24 range.
([#4541](https://github.com/open-telemetry/opentelemetry-specification/pull/4541))

### Baggage

### Resource
Expand Down
11 changes: 7 additions & 4 deletions specification/logs/supplementary-guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,19 +195,22 @@ type SeverityProcessor struct {
}

// OnEmit passes ctx and record to the wrapped sdklog.Processor
// if the record's severity is greater than or equal to p.Min.
// if the record's severity is greater than or equal to p.Min
// or oustide the log.SeverityTrace1..log.SeverityFatal4 range.
Comment thread
pellared marked this conversation as resolved.
// Otherwise, the record is dropped (the wrapped processor is not invoked).
func (p *SeverityProcessor) OnEmit(ctx context.Context, record *sdklog.Record) error {
if record.Severity() != log.SeverityUndefined && record.Severity() < p.Min {
sev := record.Severity()
if sev >= log.SeverityTrace1 && sev <= log.SeverityFatal4 && sev < p.Min {
Copy link
Copy Markdown
Contributor

@dashpole dashpole Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, might be too go-specific: Should severity have an IsValid() or IsComparable() helper? Reminds me of handling zero values for trace context.

Copy link
Copy Markdown
Member

@lmolkova lmolkova Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I push for severity being treated as a number. It's always valid and always comparable, but you probably don't want to use <=0 values unless you're building something custom.

Copy link
Copy Markdown
Member Author

@pellared pellared Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dashpole, I think that currently nothing in the spec says that values outside the 1..24 range are invalid. However, it might have be indeed the intention of the authors.
See #4509 (comment)

I also do not want to make this PR too controversial.

Copy link
Copy Markdown
Member Author

@pellared pellared Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that based on today's SIG discussion this could even be

Suggested change
if sev >= log.SeverityTrace1 && sev <= log.SeverityFatal4 && sev < p.Min {
if sev < p.Min {

Copy link
Copy Markdown
Member

@lmolkova lmolkova Jun 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favor of treating 0 as yet another severity when it comes to filtering and comparison, i.e. logs with unspecified/0 severity are dropped when threshold > 0 and are recorded otherwise.

Reasoning:
Producers that don't set severity don't work nicely with threshold-based log filtering. Assuming one of the producers sends spammy logs, they would go through app log filter and may significantly increase telemetry volume and costs.

However, It doesn't seem like there is a consensus around it.

Maybe we can take discussion around 0 separately and/or make it configurable.
We should have cross-language default for log filtering unspecified/0 severity, but if it's controversial, let's separate it from this PR and focus on integer range.

The example, however, should be adjusted to force anyone copy-pasting it to think how they want to handle unspecified/0 value. I believe it's what @jack-berg suggested earlier as custom treatment for nulls.

@trask suggested something like:

Suggested change
if sev >= log.SeverityTrace1 && sev <= log.SeverityFatal4 && sev < p.Min {
if (sev == 0 && dropLogsWithUnspecifiedSeverity()) || sev < p.Min {
// drop

This would at least make someone copy-pasting the example to think how they want to handle 0.

(PS: I spent a minute to triple-check this expression and I'm pretty sure you, my reader, did it too, that's exactly the reason why sev < p.Min is easier for everyone)

Copy link
Copy Markdown
Member

@trask trask Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this non-normative example, I think I like the simplicity of

if sev < p.Min {
// drop
}

it provides a very reasonable default behavior, which is to drop logs that don't have any severity, and people can always build more complex logic to factor those logs in as needed

return nil
}
return p.Processor.OnEmit(ctx, record)
}

// Enabled returns false if the severity is lower than p.Min.
// Enabled returns false if the severity is lower than p.Min
// and inside the log.SeverityTrace1..log.SeverityFatal4 range.
func (p *SeverityProcessor) Enabled(ctx context.Context, param sdklog.EnabledParameters) bool {
sev := param.Severity
if sev != log.SeverityUndefined && sev < p.Min {
if sev >= log.SeverityTrace1 && sev <= log.SeverityFatal4 && sev < p.Min {
Copy link
Copy Markdown
Member Author

@pellared pellared Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that based on today's SIG discussion this could even be

Suggested change
if sev >= log.SeverityTrace1 && sev <= log.SeverityFatal4 && sev < p.Min {
if sev < p.Min {

return false
}
if fp, ok := p.Processor.(sdklog.FilterProcessor); ok {
Expand Down
Loading