Introduction of log SeverityNumber 0 - "UNDEFINED"#4493
Introduction of log SeverityNumber 0 - "UNDEFINED"#4493peffis wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
…0 in the specification with the meaning "UNDEFINED"
|
|
||
| SeverityNumber range|Range name|Meaning | ||
| --------------------|----------|------- | ||
| 0 |UNDEFINED |An event with unset severity. |
There was a problem hiding this comment.
since SeverityNumber is optional in the data model, what is the difference between not setting this at all, vs setting it to 0/UNDEFINED?
There was a problem hiding this comment.
+1 to the question.
The API says that the SeverityNumber is optional. It does not define what happens if SeverityNumber is not supplied in the "Emit a LogRecord" API, so we may want to clarify that when using the OTLP exporter it should result in SEVERITY_NUMBER_UNSPECIFIED.
There was a problem hiding this comment.
since SeverityNumber is optional in the data model, what is the difference between not setting this at all, vs setting it to 0/UNDEFINED?
Semantically there is no difference. This is more a question about clarity of- and completeness in the definition about what different levels mean. 0 would be the null value in many languages (such as Golang) if you do not assign a value and this provides clarity as to what that 0 actually means. We have also found, when doing tests in CI/CD, that, always being able to assign a value (where the value could be 0 UNDEFINED if no level was provided) helped with making our tests clearer and more straight forward.
But it is true, semantically it makes no difference. We also did not consider the fact that this could cause breaking changes in the client libraries (as in the discussion about Rust) as this was mostly to provide some clarity to our opentelemetry collector CI/CD tests, so if this is undesirable due to its implications in the client libraries we understand.
|
|
||
| SeverityNumber range|Range name|Meaning | ||
| --------------------|----------|------- | ||
| 0 |UNDEFINED |An event with unset severity. |
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry/src/logs/record.rs#L141-L192
from my quick look, we cannot make this change in Otel Rust, as adding new variant to an Enum is considered breaking change.
(@lalitb could you also recheck this for Rust and C++ too)
There was a problem hiding this comment.
I do not think it has to be modeled as an enum variant. I was just thinking about having some "wording"/"definition"in the spec when the log record has the severity level not set (a name for the default value). The SDK still needs to handle somehow such cases. E.g. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/supplementary-guidelines.md#filtering
|
Log SIG discussion 5/6:
|
|
I created a second approach without explicitly defining This may be less controversial. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
5 participants
This PR introduces a SeverityNumber 0 with the meaning "UNDEFINED", for the cases where there is no severity defined for the log record.
Fixes #4478
Changes
A zero row was added to the table. The accompanying following paragraph was also added a text to highlight that the previous ordering of log events by importance according to SeverityNumber is no longer strictly valid in the case of the zero SeverityNumber. The zero severity number, as indicated in the issue #4478, does not mean that it is of less importance than, say, a DEBUG or INFO message. It just means that the severity was not defined.