[BCF-3490] Remove AtomicCore logger #20571
Conversation
|
I see you updated files related to
|
|
There was a problem hiding this comment.
Pull request overview
This PR rolls back the AtomicCore logger implementation due to OOM issues during AtomicCore allocation. The changes remove the thread-safe core swapping functionality and revert to the simpler logger initialization approach.
Key Changes:
- Removed AtomicCore implementation and associated cleanup mechanisms
- Reverted logger initialization to use direct
New()instead ofNewWithCores() - Removed OTel core swapping logic in shell initialization
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/logger/zap.go | Removed entire AtomicCore implementation including cleanup logic, WithCore type, and thread-safe core swapping; reverted interface{} type usage |
| core/logger/zap_test.go | Removed runtime-based caller line testing and AtomicCore cleanup test; reverted to hardcoded line number assertion |
| core/logger/logger_test.go | Removed AtomicCore swap test that verified core replacement functionality |
| core/cmd/app.go | Simplified logger initialization to use New() instead of NewWithCores() and removed AtomicCore lifecycle management |
| core/cmd/shell.go | Removed SetOtelCore field from Shell struct |
| core/cmd/shell_local.go | Removed OTel core swapping logic for log streaming |
| core/logger/zap_disk_logging.go | Reordered import statements (cosmetic change) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lines := strings.Split(logs, "\n") | ||
|
|
||
| require.Contains(t, lines[0], fmt.Sprintf("logger/zap_test.go:%d\ttest message with caller", lineCall-1)) | ||
| require.Contains(t, lines[0], "logger/zap_test.go:246") |
There was a problem hiding this comment.
Hardcoding the line number 246 creates a brittle test that will break if any code is added or removed above this line. Consider using a more robust approach, such as capturing the caller information at the point of logging or using a unique log message that can be matched without relying on line numbers.
| require.Contains(t, lines[0], "logger/zap_test.go:246") | |
| require.Contains(t, lines[0], "logger/zap_test.go") | |
| require.Contains(t, lines[0], "test message with caller") |
|
Approved under the assumption we will check for side effects through a soaking process. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* Minor. * Fix lint. * Update shell.
…beta1-changes [BCF-3490] Remove AtomicCore logger (#20571)
This reverts commit 9cf3f4f.
This reverts commit 9cf3f4f.
This reverts commit 9cf3f4f.
This reverts commit 9cf3f4f.
* Minor. * Fix lint. * Update shell.
This reverts commit 9cf3f4f.
This reverts commit 9cf3f4f.
3 participants
This PR effectively roll backs
#19089 and #20019
until we figure out the exact OOM culprit (except that it happens in AtomicCore allocation).