Fix SeverityProcessor example#4541
Conversation
| 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 { |
There was a problem hiding this comment.
Non-blocking, might be too go-specific: Should severity have an IsValid() or IsComparable() helper? Reminds me of handling zero values for trace context.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I think that based on today's SIG discussion this could even be
| if sev >= log.SeverityTrace1 && sev <= log.SeverityFatal4 && sev < p.Min { | |
| if sev < p.Min { |
There was a problem hiding this comment.
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:
| 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)
There was a problem hiding this comment.
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
| 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 { |
There was a problem hiding this comment.
I think that based on today's SIG discussion this could even be
| if sev >= log.SeverityTrace1 && sev <= log.SeverityFatal4 && sev < p.Min { | |
| if sev < p.Min { |
| 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 { |
There was a problem hiding this comment.
I think that based on today's SIG discussion this could even be
| if sev >= log.SeverityTrace1 && sev <= log.SeverityFatal4 && sev < p.Min { | |
| if sev < p.Min { |
|
Closing per #4552 |
Follows: - #4535 ## Why Related to #4540 Supersedes #4541 per #4541 (comment) and discussions from the last OTel Specification and Logs SIG meetings. **Some comments that were already expressed as comments.** From #4541 (comment): > 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. From #4541 (comment): > 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 This is also inline with https://github.com/open-telemetry/opentelemetry-specification/blob/7e7c0bd12a535b332284ecabd228c4749886455f/specification/logs/data-model.md?plain=1#L323-L329 From #4540 (comment): > Relying on int comparison seems to be the most natural option We agreed that this PR does conflict proposal to allow only allow/support 0..24 SeverityNumber values. ## What Removal of > `SeverityNumber` can be compared to another `SeverityNumber` or to numbers in the 1..24 range (or to the corresponding short names). as I find that this sentence is more confusing than clarifying: 1. why comparing values of different types? 2. why can we compare only with integers between 1..24? 3. what about `SeverityNumber` that are greater than 24 or lesser than 1? 4. are the short names normative? Removal of duplicated description > Smaller numerical values in each range represent less important (less severe) events. Larger numerical values in each range represent more important (more severe) events. Moving the comparison example to a better place where the `SeverityNumber` comparison is described > For example `SeverityNumber=17` describes an error that is less critical than an error with `SeverityNumber=20`. Simplify `SeverityProcessor` example. ## Prototype This is exactly how https://pkg.go.dev/go.opentelemetry.io/contrib/processors/minsev currently works. --------- Co-authored-by: Reiley Yang <reyang@microsoft.com> Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com>
Why
opentelemetry-specification/specification/logs/data-model.md
Lines 418 to 423 in bdcf53d
Currently the spec only describes how to compare values between 1 and 24.
All other values have undefined meaning.
People are free to do with them whatever they seem appropriate but at this point of time the spec shouldn't give any recommendations for them for the sake of future compatibility.
What
Fix the
SeverityProcessorexample implementation so that it filters out only the records withSeverityNumberin the 1..24 range.Extracted from #4535
Prototype
I do not think it is needed but maybe it would help the reviewers: