Skip to content

fix: use global logger for threadWatcher#41395

Closed
HurSungYun wants to merge 1 commit intomilvus-io:masterfrom
HurSungYun:threadwatcher_use_global_logger
Closed

fix: use global logger for threadWatcher#41395
HurSungYun wants to merge 1 commit intomilvus-io:masterfrom
HurSungYun:threadwatcher_use_global_logger

Conversation

@HurSungYun
Copy link
Contributor

@HurSungYun HurSungYun commented Apr 18, 2025

Currently, thread watcher observe thread num log

log.Debug("thread watcher observe thread num", zap.Int32("threadNum", threadNum))

is not affected by log level config.

I believe this is because of the initialization order.

  1. Threadwatcher initialized and started to run first ->

    milvus/cmd/roles/roles.go

    Lines 313 to 316 in c43f8f7

    // start milvus thread watcher to update actual thread number metrics
    thw := internalmetrics.NewThreadWatcher()
    thw.Start()
    defer thw.Stop()
  2. and then, logger is initialized after Threadwatcher starts to run ->
    mr.setupLogger()

In my case, thread watcher observe thread num log (it's debug level) is written even if I set my log level to WARN. It's kinda annoying for who operates Milvus.

스크린샷 2025-04-18 오후 2 07 55

Thus, I would like to suggest to use the global logger for Threadwatcher instead of a local one, to be affected by the config.

Signed-off-by: lambert <lambert@daangn.com>
@sre-ci-robot sre-ci-robot added the size/XS Denotes a PR that changes 0-9 lines. label Apr 18, 2025
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: HurSungYun
To complete the pull request process, please assign tedxu after the PR has been reviewed.
You can assign the PR to them by writing /assign @tedxu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mergify mergify bot added dco-passed DCO check passed. kind/bug Issues or changes related a bug labels Apr 18, 2025
@mergify
Copy link
Contributor

mergify bot commented Apr 18, 2025

@HurSungYun Please associate the related issue to the body of your Pull Request. (eg. “issue: #”)

@codecov
Copy link

codecov bot commented Apr 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.63%. Comparing base (c43f8f7) to head (068332f).
⚠️ Report is 698 commits behind head on master.

❌ Your project status has failed because the head coverage (72.63%) is below the target coverage (77.00%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (c43f8f7) and HEAD (068332f). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (c43f8f7) HEAD (068332f)
2 1
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #41395       +/-   ##
===========================================
- Coverage   80.60%   72.63%    -7.97%     
===========================================
  Files        1478      312     -1166     
  Lines      210192    29045   -181147     
===========================================
- Hits       169426    21098   -148328     
+ Misses      34631     7947    -26684     
+ Partials     6135        0     -6135     
Components Coverage Δ
Client ∅ <ø> (∅)
Core 72.63% <ø> (ø)
Go ∅ <ø> (∅)
see 1166 files with indirect coverage changes
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stale
Copy link

stale bot commented May 18, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label May 18, 2025
@HurSungYun
Copy link
Contributor Author

@yanliang567 @zwd1208 can you please review this PR? Thank you in advance.

@stale stale bot removed the stale indicates no udpates for 30 days label May 19, 2025
@yanliang567
Copy link
Contributor

/assign @LoveEachDay
please help to reivew the changes

@zwd1208 zwd1208 removed their request for review May 30, 2025 09:25
@stale
Copy link

stale bot commented Jun 29, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label Jun 29, 2025
@HurSungYun
Copy link
Contributor Author

@LoveEachDay can you please help me whenever you have some time?

@stale stale bot removed the stale indicates no udpates for 30 days label Jun 30, 2025
@stale
Copy link

stale bot commented Jul 30, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label Jul 30, 2025
@stale stale bot closed this Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-passed DCO check passed. do-not-merge/missing-related-issue kind/bug Issues or changes related a bug size/XS Denotes a PR that changes 0-9 lines. stale indicates no udpates for 30 days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants