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

fix: return inner Logger from GetLogger for log.WithContext reuse #3455

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

guangxuewu
Copy link
Contributor

@guangxuewu guangxuewu commented Oct 31, 2024

fix: Ensure thread safety by eliminating lock copy in GetLogger

企业微信截图_d18e997f-a193-4d6d-9dc4-cb1ee9e86e4d 企业微信截图_9782d329-af80-4d24-8f5d-611625c1c40e

Description (what this PR does / why we need it):

Which issue(s) this PR fixes (resolves / be part of):

Other special notes for the reviewers:

fix: Ensure thread safety by eliminating lock copy in GetLogger
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Oct 31, 2024
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Oct 31, 2024
@shenqidebaozi shenqidebaozi changed the title feat: Return inner Logger from GetLogger for log.WithContext reuse fix: Return inner Logger from GetLogger for log.WithContext reuse Oct 31, 2024
@shenqidebaozi shenqidebaozi added the bug Something isn't working label Oct 31, 2024
@dosubot dosubot bot added the LGTM label Oct 31, 2024
@shenqidebaozi
Copy link
Member

This pull request includes changes to improve the thread safety of the global logger and adds a new test to ensure the logger works correctly with context values.

Improvements to thread safety:

  • log/global.go: Changed the lock field in loggerAppliance from sync.Mutex to sync.RWMutex to allow for concurrent read access.
  • log/global.go: Updated the GetLogger function to use a read lock (RLock) when accessing the global logger, ensuring thread-safe read operations.

Enhancements to testing:

  • log/global_test.go: Added a new test function TestContextWithGlobalLog to verify that the global logger correctly logs context values, ensuring the logger's functionality with context-based trace IDs.

@shenqidebaozi shenqidebaozi enabled auto-merge (squash) October 31, 2024 10:01
@shenqidebaozi shenqidebaozi changed the title fix: Return inner Logger from GetLogger for log.WithContext reuse fix: return inner Logger from GetLogger for log.WithContext reuse Oct 31, 2024
@shenqidebaozi shenqidebaozi merged commit f3c4b23 into go-kratos:main Oct 31, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working LGTM size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants