Unset log record severity is not INFO

[pellared]

Related to #4478

What

Remove the possibility 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.

Why

This changes the a possibility (MAY) to a recommendation (SHOULD).
Therefore, I think it is an acceptable change for a Stable document.

I feel that this change can be considered as a bugfix given the specification already says:

opentelemetry-specification/specification/logs/data-model.md

Lines 287 to 289 in aff95e5

	`SeverityNumber` is an integer number. 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). The following table

The "unset" value is 0 which is e.g. less severe than TRACE=1. Interpreting "unset" 0 as INFO=9 is against the statement above.

As the Logs SIG, we may also think that there may be use cases where the unspecified log record severity may be important. For instance, events may need separate more domain-specific severity levels.

We can't ever fully get rid of the possibility of the severity level being unset. However, reducing the chance of having this case would be beneficial and make processing easier. One can always add a processor which changes the severity level from unset to INFO.

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. However, this closes the door of explicitly taking advantage of an unset severity level. People can always add a processor that sets the severity level to INFO if it was not set.

[trask]

would be great to get @tigrannajaryan, @jack-berg, or other original Log SIG members to review

[jack-berg]

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?

[lmolkova]

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)

[jack-berg]

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.

[lmolkova]

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).

[lmolkova]

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.

[jack-berg]

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

[jack-berg]

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.

[pellared]

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.

[trask]

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.

[pellared]

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.

[tigrannajaryan]

An indirect hint that SeverityNumber has a limited range is in the spec:

In the contexts where severity participates in less-than / greater-than comparisons SeverityNumber field should be used. SeverityNumber can be compared to another SeverityNumber or to numbers in the 1..24 range (or to the corresponding short names).

[tigrannajaryan]

0 value is also specifically called out as a value that must not be used in OTLP:

// UNSPECIFIED is the default SeverityNumber, it MUST NOT be used.

This means no producer is allowed to set this value. The reason for that is to reserve 0 as an indicator of missing value. If we make 0 a valid value in the data model in any way, then we will have a problem with mapping the value in the SDK exporter to OTLP.

[pellared]

SIG meeting notes:
This looks is against the original intend of the Logs spec authors.
The authors also say that the intend was that 1..24 should be the only allowed values (and unset) from the API perspective.

[pellared]

@tigrannajaryan, for Collector (pdata) unspecified is repesented by 0:
https://github.com/open-telemetry/opentelemetry-collector/blob/392b705719bc8796659805e05c64ce227a7eff1d/pdata/plog/severity_number.go#L14

The same is probably done in many languages. Shouldn't we say somewhere in specification that 0 is a "reserved" value as it may be used to model unspecified?

[tigrannajaryan]

The same is probably done in many languages. Shouldn't we say somewhere in specification that 0 is a "reserved" value as it may be used to model unspecified?

Yes, I think we can. We can go a step further and explicitly say values outside 1..24 range must not be used.

[pellared]

The same is probably done in many languages. Shouldn't we say somewhere in specification that 0 is a "reserved" value as it may be used to model unspecified?

Yes, I think we can. We can go a step further and explicitly say values outside 1..24 range must not be used.

Thanks. I will create a separate PR that does it to move the issue forward.

Making this one as a draft.

Thanks a lot for feedback.

[lmolkova]

Yes, I think we can. We can go a step further and explicitly say values outside 1..24 range must not be used.

I would like to push back on this. This won't solve the problem that this PR tries to address:

• how to treat logs without severity set in the in-process processors
• how to treat logs without severity set in collector/on backends.

This PR put responsibility on producers to set severity according to their best knowledge. There is definitely more for us to figure out.

[pellared]

One can always add a processor which changes the severity level from unset to INFO.

[tigrannajaryan]

I would like to push back on this. This won't solve the problem that this PR tries to address:

I am probably missing something, but I am not sure I understand what the problem is.

• how to treat logs without severity set in the in-process processors

Why do the processors need to treat it in any way? Why isn't "do nothing" an option?

• how to treat logs without severity set in collector/on backends.

Why does the Collector need to do anything? The unset severity can remain unset in the Collector.

Backends have a few options available:

1. Don't show SeverityNumber as a field on the log record.
2. Show SeverityNumber as a special "unset" value.
3. Show SeverityNumber "INFO".

My preference if I was designing a backend would be 1 or 2, but our spec also allows 3.

[pellared]

I created:

• SeverityNumber=0 MAY be used to represent an unspecified value #4535

Closing this PR.

[pellared]

Reopening and making as draft PR per discussion from Specification SIG meeting.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.