-
Notifications
You must be signed in to change notification settings - Fork 976
Introduction of log SeverityNumber 0 - "UNDEFINED" #4493
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -291,6 +291,7 @@ defines the meaning of `SeverityNumber` value: | |
|
|
||
| SeverityNumber range|Range name|Meaning | ||
| --------------------|----------|------- | ||
| 0 |UNDEFINED |An event with unset severity. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry/src/logs/record.rs#L141-L192
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| 1-4 |TRACE |A fine-grained debugging event. Typically disabled in default configurations. | ||
| 5-8 |DEBUG |A debugging event. | ||
| 9-12 |INFO |An informational event. Indicates that an event happened. | ||
|
|
@@ -301,7 +302,7 @@ SeverityNumber range|Range name|Meaning | |
| Smaller numerical values in each range represent less important (less severe) | ||
| events. Larger numerical values in each range represent more important (more | ||
| severe) events. For example `SeverityNumber=17` describes an error that is less | ||
| critical than an error with `SeverityNumber=20`. | ||
| critical than an error with `SeverityNumber=20`. The exception to this is the zero value (`SeverityNumber=0`), which, since it means UNDEFINED, cannot be ordered after any importance level. | ||
|
|
||
| #### Mapping of `SeverityNumber` | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to the question.
The API says that the
SeverityNumberis optional. It does not define what happens ifSeverityNumberis not supplied in the "Emit a LogRecord" API, so we may want to clarify that when using the OTLP exporter it should result inSEVERITY_NUMBER_UNSPECIFIED.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.