Skip to content

Unset log record severity is not INFO #4509

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
wants to merge 16 commits into from
+8 −8
Closed

Unset log record severity is not INFO #4509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
03e9737
Unset log record severity is not INFO
pellared May 13, 2025
81f64da
Update changelog entry
pellared May 13, 2025
1a08dda
SeverityNumber zero value exception
pellared May 13, 2025
69fd343
Refine
pellared May 13, 2025
7ef0c06
Update CHANGELOG.md
pellared May 13, 2025
12c6820
Merge branch 'main' into rem-interpet
pellared May 14, 2025
2634f7b
Update CHANGELOG.md
pellared May 14, 2025
bc37f7e
Update data-model.md
pellared May 14, 2025
28fd201
Update supplementary-guidelines.md
pellared May 14, 2025
c1d0f94
Replace with better guidelines
pellared May 15, 2025
1f9e36b
Format
pellared May 15, 2025
63f183a
Refine
pellared May 15, 2025
3ab4568
Merge branch 'main' into rem-interpet
pellared May 20, 2025
297ab80
Merge branch 'main' into rem-interpet
pellared May 22, 2025
a8ffe4b
Merge branch 'main' into rem-interpet
pellared May 23, 2025
cf91cc0
Merge branch 'main' into rem-interpet
pellared May 27, 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 @@ -15,6 +15,10 @@ release.

### Logs

- Remove the suggestion that backends or UI may represent log records with unset severity as INFO severity.
Source formats that do not define a concept of severity or log level SHOULD emit INFO.
([#4509](https://github.com/open-telemetry/opentelemetry-specification/pull/4509))

### Baggage

### Resource
Expand Down
7 changes: 2 additions & 5 deletions specification/logs/data-model.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,11 +328,8 @@ For example if the source format has an "Informational" log level and no other
log levels with similar meaning then it is recommended to use
`SeverityNumber=9` for "Informational".

Source formats that do not define a concept of severity or log level MAY omit
`SeverityNumber` and `SeverityText` fields. Backend and UI may represent log
records with missing severity information distinctly or may interpret log
records with missing `SeverityNumber` and `SeverityText` fields as if the
`SeverityNumber` was set equal to INFO (numeric value of 9).
Source formats that do not define a concept of severity or log level SHOULD
set `SeverityNumber=9`.
Copy link
Member

Choose a reason for hiding this comment

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

Previously:

  • Sources that do not define a concept of severity MAY omit severity
  • UI may represent missing severity as INFO

Proposed:

  • Sources that do not define a concept of severity SHOULD set severity as INFO
  • Guidance for UIs is removed, presumably meaning if a UI does receive an unset severity, they should not represent it as INFO?

I get the motivation of this PR based on the title: UIs should not have guidance to represent unset severity as INFO, since its misleading.

But why change / remove this guidance?

Source formats that do not define a concept of severity or log level MAY omit
SeverityNumber and SeverityText fields.

Why do we want sources formats without a severity concept to represent it as INFO instead of omitting it?

Copy link
Contributor

@lmolkova lmolkova May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Omitted severity translates into 0 on OTLP layer, which is not info and should behave similarly to Log4J OFF - never record log with this level. It's quite confusing to have different behavior on the SDK log-filtering side and also special-case 0 on the consumer side, but in a different way

So I believe we want everything to set severity explicitly.

It likely means that we'll need to define a constant for 'always log' - (max int) that would be used to record something like business-telemetry event that should always be recorded, but this is not currently in scope of this PR.

there is some context here: #4509 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

which is not info and should behave similarly to Log4J OFF

Why?

At the proto level, we say that the severity_number field is optional, where inset (really SEVERITY_NUMBER_UNSPECIFIED) is the default value.

It seems like this PR (in its current form) is trying to change the semantics of severity from optional to required with a default value of optional.

I get that unset has a value of 0, and thus has a sort of strange interplay with someone trying to write a severity threshold. But to me this is something you need to consider when setting up a severity threshold. A user needs to consider what the threshold should be when severity is set, and separately decide what to do when severity is unset.

Copy link
Contributor

@lmolkova lmolkova May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why?

int comparison and simplicity for everyone.

Let's say user writes an event and does not set severity:

  • their in-process log processors maybe smart enough to not filter out logs with unset severity
  • collector-based log processors doing threshold comparison would not be able to differentiate unset from 0 and/or would need to special-case 0 to treat it as unset.
  • backends need to special-case it again. Update: e.g. you want to order logs by severity, now you need to special-case 0 to be bigger than everything

Now what if someone configures log level threshold to be 0? What would it mean? All logs or no logs?

Life would have been much easier if we just relied on int comparison and let it do the thing.
SDK can record unset severity as max_int, because treating it as 0 makes no sense (you don't write logs that should never be logged).

Copy link
Contributor

@lmolkova lmolkova May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like this PR (in its current form) is trying to change the semantics of severity from optional to required with a default value of optional.

I'd say the OTLP definition of it as optional is somewhat weird - it's always there for consumers. You cannot not set it because it's enum.

Copy link
Member

Choose a reason for hiding this comment

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

collector-based log processors doing threshold comparison would not be able to differentiate unset from 0 and/or would need to special-case 0 to treat it as unset.

I don't understand this - there is no difference. 0 is equivalent to unset. Why does a processor need to differentiate?

backends need to special-case it again. Update: e.g. you want to order logs by severity, now you need to special-case 0 to be bigger than everything

I'd argue a backend receiving a severity of unset / 0 should store a null / empty value. Then from an ordering standpoint, you need to specify whether nulls are first or last. I.e. in PostgreSQL, this might look like ORDER BY severity NULLS FIRST.

I'd say the OTLP definition of it as optional is somewhat weird

The field has been defined in a way that seems semantically equivalent to the using the protobuf optional keyword. The protobuf spec says that all enums MUST have a zero value which is the default. I can't tell if its idiomatic to use the optional keyword with an enum. The argument against doing this is that because enums require a unspecified 0 value, the optional keyword presents 2 empty states:

  • present and unspecified
  • not present

Copy link
Member

Choose a reason for hiding this comment

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

The argument is a good one: having an optional severity (regardless of whether that is accomplished with the optional keyword, or with a comment indicating [Optional] and the _UNSPECIFIED enum option) makes processing based on the notion of a severity threshold more difficult.

But can we go back and make a field required that is currently optional? Strictly speaking, no.

But this isn't trying to change the proto - instead, its trying to change the guidance to recommend never leaving the field unset. This is an allowed change, but since we can't ever fully get rid of the possibility of the field being unset, processors will always have to be aware of this case and probably provide utilities to solve it. I think the only way to avoid this would have been to model severity as something different than an enum, since all protobuf enums require a default _UNSPECIFIED value.

Copy link
Member Author

@pellared pellared May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is only a recommendation and I think it is a good one. Notice that log records with severity level 0 (_UNSPECIFIED) would be filtered by most severity processor implementations given the existing doc that I find better to be keep:

Smaller numerical values correspond to
less severe events (such as debug events), larger numerical values correspond to
more severe events (such as errors and critical events)

But can we go back and make a field required that is currently optional? Strictly speaking, no

Agreed and we are not changing it.

I think this PR proposes better recommendation without changing the model.

we can't ever fully get rid of the possibility of the field being unset

True, but I think that reducing the chance of having this case would be beneficial.

Copy link
Member

Choose a reason for hiding this comment

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

I'm supportive of this proposal. Typically when I've seen sysout/syserr piped into a standard logger (e.g. log4j), they've been tagged with INFO/ERROR respectively.

Copy link
Member Author

@pellared pellared May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

An alternative could be that the SDK always sets the the severity level to INFO for those that log record that have been emitted with unset severity level.


#### Reverse Mapping

Expand Down
5 changes: 2 additions & 3 deletions specification/logs/supplementary-guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,16 +198,15 @@ type SeverityProcessor struct {
// if the record's severity is greater than or equal to p.Min.
// 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 {
if record.Severity() < p.Min {
return nil
}
return p.Processor.OnEmit(ctx, record)
}

// Enabled returns false if the severity is lower than p.Min.
func (p *SeverityProcessor) Enabled(ctx context.Context, param sdklog.EnabledParameters) bool {
sev := param.Severity
if sev != log.SeverityUndefined && sev < p.Min {
if param.Severity < p.Min {
return false
}
if fp, ok := p.Processor.(sdklog.FilterProcessor); ok {
Expand Down
Loading