Skip to content
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

otellogrus: Fix logrus.Level conversion #6191

Merged
merged 10 commits into from
Oct 10, 2024

Conversation

vkuptcov
Copy link
Contributor

@vkuptcov vkuptcov commented Oct 4, 2024

Logrus severity transformation is broken right now as all severity levels are counterintuitively transformed to various trace levels.
Logrus has the next levels from 0 (the highest) to 6 (lowest):

const (
	// PanicLevel level, highest level of severity. Logs and then calls panic with the
	// message passed to Debug, Info, ...
	PanicLevel Level = iota
	// FatalLevel level. Logs and then calls `logger.Exit(1)`. It will exit even if the
	// logging level is set to Panic.
	FatalLevel
	// ErrorLevel level. Logs. Used for errors that should definitely be noted.
	// Commonly used for hooks to send errors to an error tracking service.
	ErrorLevel
	// WarnLevel level. Non-critical entries that deserve eyes.
	WarnLevel
	// InfoLevel level. General operational entries about what's going on inside the
	// application.
	InfoLevel
	// DebugLevel level. Usually only enabled when debugging. Very verbose logging.
	DebugLevel
	// TraceLevel level. Designates finer-grained informational events than the Debug.
	TraceLevel
)

while otel has a huge number of levels from 0 (the lowest) to 24 (the highest):

const (
	// SeverityUndefined represents an unset Severity.
	SeverityUndefined Severity = 0 // UNDEFINED

	// A fine-grained debugging log record. Typically disabled in default
	// configurations.
	SeverityTrace1 Severity = 1 // TRACE
	SeverityTrace2 Severity = 2 // TRACE2
	SeverityTrace3 Severity = 3 // TRACE3
	SeverityTrace4 Severity = 4 // TRACE4

	// A debugging log record.
	SeverityDebug1 Severity = 5 // DEBUG
	SeverityDebug2 Severity = 6 // DEBUG2
	SeverityDebug3 Severity = 7 // DEBUG3
	SeverityDebug4 Severity = 8 // DEBUG4

	// An informational log record. Indicates that an event happened.
	SeverityInfo1 Severity = 9  // INFO
	SeverityInfo2 Severity = 10 // INFO2
	SeverityInfo3 Severity = 11 // INFO3
	SeverityInfo4 Severity = 12 // INFO4

	// A warning log record. Not an error but is likely more important than an
	// informational event.
	SeverityWarn1 Severity = 13 // WARN
	SeverityWarn2 Severity = 14 // WARN2
	SeverityWarn3 Severity = 15 // WARN3
	SeverityWarn4 Severity = 16 // WARN4

	// An error log record. Something went wrong.
	SeverityError1 Severity = 17 // ERROR
	SeverityError2 Severity = 18 // ERROR2
	SeverityError3 Severity = 19 // ERROR3
	SeverityError4 Severity = 20 // ERROR4

	// A fatal log record such as application or system crash.
	SeverityFatal1 Severity = 21 // FATAL
	SeverityFatal2 Severity = 22 // FATAL2
	SeverityFatal3 Severity = 23 // FATAL3
	SeverityFatal4 Severity = 24 // FATAL4

	// Convenience definitions for the base severity of each level.
	SeverityTrace = SeverityTrace1
	SeverityDebug = SeverityDebug1
	SeverityInfo  = SeverityInfo1
	SeverityWarn  = SeverityWarn1
	SeverityError = SeverityError1
	SeverityFatal = SeverityFatal1
)

Right now the conversion is done via offset

const sevOffset = logrus.Level(log.SeverityDebug) - logrus.DebugLevel
record.SetSeverity(log.Severity(e.Level + sevOffset))

and produces pretty strange and unexpected results, when all logrus levels become debug or trace

@vkuptcov vkuptcov requested review from dmathieu, pellared and a team as code owners October 4, 2024 07:34
Copy link

linux-foundation-easycla bot commented Oct 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmathieu
Copy link
Member

dmathieu commented Oct 4, 2024

Could you sign the CLA?

@dmathieu dmathieu added the blocked: CLA Waiting on CLA to be signed before progress can be made label Oct 4, 2024
@vkuptcov
Copy link
Contributor Author

vkuptcov commented Oct 4, 2024

Could you sign the CLA?

done

@dmathieu dmathieu removed the blocked: CLA Waiting on CLA to be signed before progress can be made label Oct 4, 2024
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.0%. Comparing base (fb4466f) to head (259bd67).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
bridges/otellogrus/hook.go 89.4% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6191   +/-   ##
=====================================
  Coverage   67.0%   67.0%           
=====================================
  Files        194     194           
  Lines      12692   12709   +17     
=====================================
+ Hits        8510    8525   +15     
- Misses      3903    3905    +2     
  Partials     279     279           
Files with missing lines Coverage Δ
bridges/otellogrus/hook.go 97.5% <89.4%> (-1.5%) ⬇️

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add unit tests for severity level transformations and add a changelog entry?

@vkuptcov
Copy link
Contributor Author

vkuptcov commented Oct 4, 2024

@pellared done. All stages are fine, except the check-links stage but fails also in the main branch.

bridges/otellogrus/hook.go Outdated Show resolved Hide resolved
@vkuptcov vkuptcov requested review from dmathieu and pellared October 4, 2024 09:43
@pellared pellared changed the title Fix logrus severity conversion otellogrus: Fix logrus.Level conversion Oct 4, 2024
bridges/otellogrus/hook.go Outdated Show resolved Hide resolved
@vkuptcov vkuptcov requested a review from pellared October 4, 2024 10:07
@pellared pellared added this to the v1.31.0 milestone Oct 4, 2024
@dmathieu dmathieu merged commit 8ae2168 into open-telemetry:main Oct 10, 2024
23 of 24 checks passed
MrAlias added a commit that referenced this pull request Oct 14, 2024
### Added

- The `Severitier` and `SeverityVar` types are added to
`go.opentelemetry.io/contrib/processors/minsev` allowing dynamic
configuration of the severity used by the `LogProcessor`. (#6116)
- Move examples from `go.opentelemetry.io/otel` to this repository under
`examples` directory. (#6158)
- Support yaml/json struct tags for generated code in
`go.opentelemetry.io/contrib/config`. (#5433)
- Add support for parsing YAML configuration via `ParseYAML` in
`go.opentelemetry.io/contrib/config`. (#5433)
- Add support for temporality preference configuration in
`go.opentelemetry.io/contrib/config`. (#5860)

### Changed

- The function signature of `NewLogProcessor` in
`go.opentelemetry.io/contrib/processors/minsev` has changed to accept
the added `Severitier` interface instead of a `log.Severity`. (#6116)
- Updated `go.opentelemetry.io/contrib/config` to use the
[v0.3.0](https://github.com/open-telemetry/opentelemetry-configuration/releases/tag/v0.3.0)
release of schema which includes backwards incompatible changes. (#6126)
- `NewSDK` in `go.opentelemetry.io/contrib/config` now returns a no-op
SDK if `disabled` is set to `true`. (#6185)
- The deprecated
`go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`
package has found a Code Owner. The package is no longer deprecated.
(#6207)

### Fixed

- Possible nil dereference panic in
`go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`.
(#5965)
- `logrus.Level` transformed to appropriate `log.Severity` in
`go.opentelemetry.io/contrib/bridges/otellogrus`. (#6191)

### Removed

- The `Minimum` field of the `LogProcessor` in
`go.opentelemetry.io/contrib/processors/minsev` is removed.
  Use `NewLogProcessor` to configure this setting. (#6116)
- The deprecated
`go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron`
package is removed. (#6186)
- The deprecated `go.opentelemetry.io/contrib/samplers/aws/xray` package
is removed. (#6187)

---------

Co-authored-by: David Ashpole <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants