Fix nil panic caused by Logrus not being initialized due to init execution order #235
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a nil panic that occurs during application startup when using Go 1.24 and go-agent v0.6, caused by initialization order issues where Logrus components are accessed before being fully initialized.
- Added nil checks in logger context retrieval and Logrus logger update functions
- Protected against accessing uninitialized operator and logger instances during init execution
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/go-agent/instrument/logger/frameworks/logrus_adapt.go | Added nil check for logger parameter and ChangeLogger callback |
| tools/go-agent/instrument/logger/context.go | Added nil check for operator before accessing LogReporter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if l == nil { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The empty line after the return statement is unnecessary and inconsistent with the formatting of the rest of the function. Consider removing it for better code consistency.
Copilot uses AI. Check for mistakes.
| if operator == nil { | ||
| return nil | ||
| } | ||
| report, ok := operator.LogReporter().(LogReporter) | ||
| if !ok || report == nil { | ||
| return nil | ||
| } | ||
| return report.GetLogContext(withEndpoint) |
There was a problem hiding this comment.
Inconsistent indentation: line 83 uses spaces while line 82 uses tabs. The indentation should be consistent throughout the function.
Copilot uses AI. Check for mistakes.
|
Thanks for your report. Please fix the CI first, then add a plugin test case to verify whether logrus and go are working correctly. |
.github/workflows/plugin-tests.yaml
Outdated
| push: | ||
| branches: | ||
| - '**' | ||
| pull_request: | ||
| branches: | ||
| - '**' | ||
| # pull_request: |
There was a problem hiding this comment.
This should not be added. Please run these tests locally. And PR will trigger CI automatically.
There was a problem hiding this comment.
Got it, thanks! I’m testing the workflow to meet the CI requirements. I’ll create a new branch to avoid affecting this PR.
| if l == nil { | ||
| return | ||
| } | ||
|
|
||
| if LogTracingContextEnable { | ||
| if _, wrapperd := l.Formatter.(*WrapFormat); !wrapperd { | ||
| l.Formatter = Wrap(l.Formatter, LogTracingContextKey) | ||
| } | ||
| } | ||
| if ChangeLogger != nil { | ||
| ChangeLogger(NewLogrusAdapter(l)) | ||
| } |
There was a problem hiding this comment.
Why these changes? It seems code style change, please revert and follow the upstream pattern.
There was a problem hiding this comment.
Got it, thanks! I’m testing the workflow to meet the CI requirements. I’ll create a new branch to avoid affecting this PR.
Just a side note: the link to the Go agent v0.6 documentation actually points to v0.5, and opening it results in a 404 Not Found. Could this be fixed? |
|
Should be fixed by apache/skywalking-website@18edf0f |
3 participants
Changes
Added nil check in
Reason for Change
Our project worked normally with Go 1.21 + go-agent v0.4.
After upgrading to Go 1.24 + go-agent v0.6, it compiled successfully but crashed with a nil panic at startup.
Investigation showed that the panic occurred because Logrus was accessed during initialization (init), while the underlying WrapFormat called GetLogContext before Logrus was fully initialized.
Full investigation details are documented on my blog: https://lymboy.com/archives/go-skywalking-agent-logrus-nil-panic