-
Notifications
You must be signed in to change notification settings - Fork 1.2k
sdk/log: Fix AddAttributes, SetAttributes, SetBody on Record to not mutate input #7403
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7403 +/- ##
======================================
Coverage 86.2% 86.3%
======================================
Files 294 294
Lines 25844 25987 +143
======================================
+ Hits 22286 22428 +142
- Misses 3185 3186 +1
Partials 373 373
🚀 New features to boost your workflow:
|
CC @varkey98 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a concurrency bug in the OpenTelemetry Go SDK where the AddAttributes
, SetAttributes
, and SetBody
methods on the Record
type were mutating input parameters, causing race conditions when the same input data was used concurrently across multiple goroutines.
- Implements defensive copying to prevent input mutation while maintaining performance optimizations
- Adds comprehensive test coverage for various deduplication and truncation scenarios
- Removes race condition workarounds that were previously skipping tests
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
sdk/log/record.go | Core fix - adds defensive copying logic and optimization checks to prevent input mutation |
sdk/log/record_test.go | Extensive test cases added for deduplication, truncation, and edge cases; removes race condition skip |
sdk/log/race_on_test.go | Removed - no longer needed after fixing the race condition |
sdk/log/race_off_test.go | Removed - no longer needed after fixing the race condition |
CHANGELOG.md | Documents the bug fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅 Quite profound.
Changing to draft as I think it is not fully fixed (sic!). I need to spend more time on it at least to double check. |
This comment was marked as outdated.
This comment was marked as outdated.
@MrAlias, merging the PR as I am out next week and tomorrow I need to focus on other things 😬 If you find any opportunities to improve the performance then please create an issue or PR. If I remember correctly this was the consensus during the SIG meeting. |
Fixes #7364
The allocations happen only when necessary.
This is when deduplication happens or data is truncated because of the limits.