Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 31, 2024
Merged

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

merged 3 commits into from
Oct 31, 2024

Conversation

guangxuewu
Copy link
Contributor

@guangxuewu guangxuewu commented Oct 31, 2024 •
edited by shenqidebaozi
Loading

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:

feat: Return inner Logger from GetLogger for log.WithContext reuse
84e9a4a 
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
@shenqidebaozi shenqidebaozi mentioned this pull request Oct 31, 2024
Closed
chaosue pushed a commit to chaosue/kratos that referenced this pull request Dec 18, 2024
@guangxuewu
fix: Return inner Logger from GetLogger for log.WithContext reuse (go…
805de62 
…-kratos#3455)

* feat: Return inner Logger from GetLogger for log.WithContext reuse
fix: Ensure thread safety by eliminating lock copy in GetLogger

* feat: Add TestContextWithGlobalLog, WithContext from GlobalLog, print kv from context

* fix: lint

---------

Co-authored-by: guangxue.wu <guangxue.wu@yunzhanghu.com>
@shenqidebaozi shenqidebaozi mentioned this pull request Dec 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shenqidebaozi shenqidebaozi shenqidebaozi approved these changes

@songzhibin97 songzhibin97 songzhibin97 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working LGTM size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guangxuewu @shenqidebaozi @songzhibin97