10000 Clarify log record severity comparison by pellared · Pull Request #4552 · open-telemetry/opentelemetry-specification · GitHub
[go: up one dir, main page]
Skip to content

Clarify log record severity comparison #4552

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

Jump to bottom
Merged
merged 13 commits into from
Jul 21, 2025
Merged

Clarify log record severity comparison #4552

merged 13 commits into from
Jul 21, 2025

Conversation

pellared
Copy link
Member
@pellared pellared commented Jun 12, 2025 •
edited
Loading

Follows:

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

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

Lines 323 to 329 in 7e7c0bd

If the source format has only a single severity that matches the meaning of the
range then it is recommended to assign that severity the smallest value of the
range.
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".

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.

@pellared pellared mentioned this pull request Jun 12, 2025
Closed
@pellared pellared moved this to In progress in Logs SIG Jun 12, 2025
@pellared pellared self-assigned this Jun 12, 2025
@pellared pellared added spec:logs Related to the specification/logs directory area:data-model For issues related to data model clarification clarify ambiguity in specification labels Jun 12, 2025
@pellared pellared marked this pull request as ready for review June 12, 2025 13:01
@pellared pellared requested review from a team June 12, 2025 13:01
@bixu

This comment was marked as resolved.

Sign in to view
@trask trask changed the title Clarify log record serverty comparison Clarify log record severity comparison Jun 12, 2025
trask
trask reviewed Jun 12, 2025
View reviewed changes
@pellared
Copy link
Member Author
pellared commented Jun 12, 2025 •
edited
Loading

This applied suggestion makes this PR blocked by:

@pellared pellared requested a review from trask June 12, 2025 14:17
@pellared
Copy link
Member Author
pellared commented Jun 17, 2025 •
edited
Loading

SIG meeting notes:
Because of e.g. https://protobuf.dev/programming-guides/enum/#java it is safer to allow only 0..24 values.
I am going to propose a separate issues/PRs to address it.

@pellared pellared marked this pull request as draft June 17, 2025 15:35
@pellared pellared marked this pull request as draft June 17, 2025 15:35
@pellared pellared mentioned this pull request Jun 24, 2025
Merged
@pellared
Copy link
Member Author
pellared commented Jun 24, 2025 •
edited
Loading

Because of e.g. https://protobuf.dev/programming-guides/enum/#java it is safer to allow only 0..24 values.
I am going to propose a separate issues/PRs to address it.

I feel that this PR does not contradict with the proposal to allow only 0..24 when exporting via OTLP.
Therefore, I am planing to reopen this PR once #4535 is merged.

@pellared
Copy link
Member Author
pellared commented Jun 24, 2025 •
edited
Loading

Logs SIG meeting:
We agreed that this PR does conflict proposal to allow only allow/support 0..24 SeverityNumber values and that this PR polishes and clarifies the specification.

We also agreed that, due to the current state of Logs language implementations and the OTLP proto definition, we can only RECOMMEND using values between 0 and 24.

@pellared pellared marked this pull request as ready for review June 24, 2025 17:57
carlosalberto
carlosalberto reviewed Jun 27, 2025
View reviewed changes
@pellared
Copy link
Member Author
pellared commented Jul 1, 2025

SIG meeting notes:
Waiting a little more to give a chance for more people to review (most notably @tigrannajaryan).

@pellared pellared requested a review from carlosalberto July 1, 2025 16:41
This was referenced Jul 10, 2025
Merged
Closed
@carlosalberto
Copy link
Contributor

@tigrannajaryan Merging this, FYI

@carlosalberto carlosalberto added this pull request to the merge queue Jul 21, 2025
Merged via the queue into open-telemetry:main with commit 742facc Jul 21, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in Logs SIG Jul 21, 2025
@svrnm svrnm mentioned this pull request Jul 28, 2025
Open
@carlosalberto carlosalberto mentioned this pull request Aug 11, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Aug 13, 2025
@carlosalberto @arminru
Release 1.48.0 (#4620)
5342fef 
August 2025 release.

### Logs

- Improve concurrency safety description of `LogRecordProcessor.OnEmit`.

([#4578](#4578))
- Clarify that `SeverityNumber` values are used when comparing
severities.

([#4552](#4552))

### Entities

- Mention entity references in the stability guarantees.

([#4593](#4593))

### OpenTelemetry Protocol

- Clarify protocol defaults on specification.

([#4585](#4585))

### Compatibility

- Flexibilie escaping of characters that are discouraged by Prometheus
Conventions
  in Prometheus exporters.

([#4533](#4533))
- Flexibilize addition of unit/type related suffixes in Prometheus
exporters.

([#4533](#4533))
- Define the configuration option "Translation Strategies" for
Prometheus exporters.

([#4533](#4533))
- Define conversion of Prometheus native histograms to OpenTelemetry
exponential histograms.

([#4561](#4561))
- Clarify what to do when scope attribute conflicts with name, version
and schema URL.

([#4599](#4599))

### SDK Configuration

- Enum values provided via environment variables SHOULD be interpreted
in a case-insensitive manner.

([#4576](#4576))

Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trask trask trask approved these changes

@lmolkova lmolkova lmolkova approved these changes

@cijothomas cijothomas cijothomas approved these changes

@reyang reyang reyang approved these changes

@jack-berg jack-berg jack-berg approved these changes

@carlosalberto carlosalberto Awaiting requested review from carlosalberto

Assignees

@pellared pellared

Labels
area:data-model For issues related to data model clarification clarify ambiguity in specification spec:logs Related to the specification/logs directory
Projects
Logs SIG
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@pellared @bixu @carlosalberto @trask @lmolkova @cijothomas @reyang @jack-berg
0