otel: add options to configure log level and span error status (WithLevel, WithErrorStatusLevel) - #25
Conversation
0ae6429 to
15f4ac3
Compare
042c0c7 to
07d754d
Compare
There was a problem hiding this comment.
Pull request overview
Adds configurability to the otel logrus hook so consumers can (1) filter which log levels become span events and (2) optionally mark spans as error based on log severity, aligning behavior with common logrus/OpenTelemetry integration expectations.
Changes:
- Add
WithLevel(log.Level)to restrict which log levels the hook handles. - Add
WithErrorStatusLevel(log.Level)to set span status tocodes.Errorfor sufficiently severe log entries. - Add tests covering default vs configured hook levels and span error-status behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| otel/log.go | Adds hook options for level filtering and error-status behavior, and updates Levels()/Fire() accordingly |
| otel/log_test.go | Adds unit tests for WithLevel and WithErrorStatusLevel using a minimal test span |
Suppressed comments (1)
otel/log.go:98
- Levels() returns internal slices (either h.levels or the package-level allLevels) directly. Since slices are mutable, callers can accidentally modify the hook's configuration (and currently, modify allLevels globally). Return a defensive copy instead.
if h.levels == nil {
return allLevels
}
return h.levels
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for i, l := range allLevels { | ||
| if l == level { | ||
| h.levels = allLevels[:i+1] | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Yeah, this could be a h.levels = slices.Clone(allLevels[:i+1]), but even that won't prevent Hook.Levels() from returning the slice and it being mutable unless every hook "fire" would return a clone.
This is a wider pattern than just this one; Logrus itself has an even worse case, where AllLevels is an exported var; https://github.com/sirupsen/logrus/blob/6d6a132bc03324d4ceb78e1b927f995d014cda20/logrus.go#L87-L95
// AllLevels exposing all logging levels.
var AllLevels = []Level{
PanicLevel,
FatalLevel,
ErrorLevel,
WarnLevel,
InfoLevel,
DebugLevel,
TraceLevel,
}Which usually is included directly in hooks;
https://github.com/sirupsen/logrus/blob/6d6a132bc03324d4ceb78e1b927f995d014cda20/hooks/syslog/syslog.go#L60-L62
unc (hook *SyslogHook) Levels() []logrus.Level {
return logrus.AllLevels
}
07d754d to
0210ffe
Compare
39586c7 to
9feadfd
Compare
| // Levels returns the logrus levels that this hook is interested in. | ||
| func (h *LogrusHook) Levels() []log.Level { | ||
| return allLevels | ||
| return h.levels | ||
| } |
There was a problem hiding this comment.
Alright, alright, I'll add it back, but 😠 don't come back to comment "you should not return allLevels without cloning!
Add WithLevel to configure the minimum log level handled by the OpenTelemetry hook. This allows callers to limit which log entries are recorded as span events, reducing trace noise and overhead when lower-severity logs are not useful for tracing. By default, the hook continues to handle all log levels, preserving the existing behavior. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Add WithErrorStatusLevel to configure the minimum log level that marks an active span with an error status. This allows callers to reflect sufficiently severe log entries in the span status while keeping the behavior independent from attached error fields. By default, log entries continue to leave the span status unchanged. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
9feadfd to
8a61f63
Compare
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| span := &testSpan{} |
There was a problem hiding this comment.
Ah, you're in nit-picking mode now, right?
It's a test, CoPilot; the test panicking in that case is a feature - it means that a refactor reveals non-implemented things; which is better than silently swallowing by some stub.
otel: add options to configure log level and span error status
These options make the containerd hook flexible enough to replace otellogrus (https://pkg.go.dev/github.com/uptrace/opentelemetry-go-extra/otellogrus) for consumers that currently use it for Logrus/OpenTelemetry integration combined with containerd/log. Callers can preserve their existing log-level filtering and span error-status behavior without needing a second tracing hook implementation.
otel: add option to configure log level
Add WithLevel to configure the minimum log level handled by the
OpenTelemetry hook.
This allows callers to limit which log entries are recorded as span
events, reducing trace noise and overhead when lower-severity logs are
not useful for tracing.
By default, the hook continues to handle all log levels, preserving
the existing behavior.
otel: add option to set span error status
Add WithErrorStatusLevel to configure the minimum log level that marks
an active span with an error status.
This allows callers to reflect sufficiently severe log entries in the
span status while keeping the behavior independent from attached error
fields.
By default, log entries continue to leave the span status unchanged.