search

open-telemetry / opentelemetry-log-collection

OpenTelemetry log collection library
https://opentelemetry.io
Apache License 2.0
92 stars 41 forks source link

Severity labels do not match to severity numbers #456

Closed politician closed 2 years ago

politician commented 2 years ago

I was debugging why the severity of my logs was not parsed (I was getting SEVERITY_NUMBER_UNSPECIFIED on severity parsing)

Does not work as expected:

severity:
  parse_from: severity_field

Works as expected:

severity:
  preset: none
  parse_from: severity_field
  mapping:
    traces: 1
    trace2: 2
    trace3: 3
    trace4: 4
    debug: 5
    debug2: 6
    debug3: 7
    debug4: 8
    info: 9
    info2: 10
    info3: 11
    info4: 12
    warn: 13
    warn2: 14
    warn3: 15
    warn4: 16
    error: 17
    error2: 18
    error3: 19
    error4: 20
    fatal: 21
    fatal2: 22
    fatal3: 23
    fatal4: 24

And I think the reason is that there is no association between severity numbers and severity labels in the default preset.

What is the point of the following lines? Why not map to the numbers as specified in the spec? This would save a lot of boilerplate code as shown above.

https://github.com/open-telemetry/opentelemetry-log-collection/blob/87e9ee2d69330fc65123548d9f8a823812ac7b13/entry/severity.go#L65-L91

djaglowski commented 2 years ago

I'd be curious to understand your use case in more detail, but on the surface I understand that you are consuming logs that use OTel's numeric severity system.

This library was actually not designed with that use case in mind. Generally, the purpose of the library is to parse logs produced by legacy software. That said, you're right, we should have native support for "parsing" OTel severity numbers.

As I mentioned, this library expects that most users are parsing from non-otel formats. This means that the numbers 1-24 do not have an implicit association with OTel's severity scale, nor should they be automatically interpreted as such. However, I think it makes sense to provide a preset option that does exactly this. It could interpret both OTel's numeric and string representations. The configuration would be:

severity:
  preset: otel
  parse_from: severity_field

and would be equivalent to:

severity:
  preset: none
  parse_from: severity_field
  mapping:
    trace: [ 1, "trace" ]
    trace2: [ 2, "trace2" ]
    ...

What is the point of the following lines?

To address this question directly - This map makes the String() function more efficient.

politician commented 2 years ago

I'd be curious to understand your use case in more detail, but on the surface I understand that you are consuming logs that use OTel's numeric severity system.

I am using Pino and I thought it would be a good idea to use the levels as specified in the Otel spec to be more futureproof. Is it not?

Here's my app code

I am still working on the helm chart with the Otel configuration, so it's not live yet

This library was actually not designed with that use case in mind. Generally, the purpose of the library is to parse logs produced by legacy software. That said, you're right, we should have native support for "parsing" OTel severity numbers.

If I use the Otel spec levels, should I be doing something differently or am I on the right path?

However, I think it makes sense to provide a preset option that does exactly this

Would be happy if you were to put that in your pipeline.

djaglowski commented 2 years ago

Thanks for sharing more details @politician.

I think using the OTel severity levels rather than inventing yet another scale is a great idea. The OTel collector should support it.