opentelemetry-specification icon indicating copy to clipboard operation
opentelemetry-specification copied to clipboard

Clarify log record severity comparison

Open pellared opened this issue 5 months ago • 5 comments

Follows:

  • https://github.com/open-telemetry/opentelemetry-specification/pull/4535

Why

Related to https://github.com/open-telemetry/opentelemetry-specification/issues/4540

Supersedes https://github.com/open-telemetry/opentelemetry-specification/pull/4541 per https://github.com/open-telemetry/opentelemetry-specification/pull/4541#discussion_r2138670874 and discussions from the last OTel Specification and Logs SIG meetings.

Some comments that were already expressed as comments.

From https://github.com/open-telemetry/opentelemetry-specification/pull/4541#discussion_r2138670874:

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 https://github.com/open-telemetry/opentelemetry-specification/pull/4541#discussion_r2138960382:

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

https://github.com/open-telemetry/opentelemetry-specification/blob/7e7c0bd12a535b332284ecabd228c4749886455f/specification/logs/data-model.md?plain=1#L323-L329

From https://github.com/open-telemetry/opentelemetry-specification/issues/4540#issue-3115353902:

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 avatar Jun 12 '25 12:06 pellared

Very minor nit, but will help folks search for this PR, title should contain severity not serverty.

bixu avatar Jun 12 '25 13:06 bixu

This applied suggestion makes this PR blocked by:

  • https://github.com/open-telemetry/opentelemetry-specification/pull/4535

pellared avatar Jun 12 '25 14:06 pellared

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 avatar Jun 17 '25 15:06 pellared

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 https://github.com/open-telemetry/opentelemetry-specification/pull/4535 is merged.

pellared avatar Jun 24 '25 11:06 pellared

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 avatar Jun 24 '25 17:06 pellared

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

pellared avatar Jul 01 '25 15:07 pellared

@tigrannajaryan Merging this, FYI

carlosalberto avatar Jul 21 '25 15:07 carlosalberto