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

malloc: fix log level #16509

Merged
merged 2 commits into from
May 31, 2024
Merged

malloc: fix log level #16509

merged 2 commits into from
May 31, 2024

Conversation

reusee
Copy link
Contributor

@reusee reusee commented May 30, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue # https://github.com/matrixorigin/MO-Cloud/issues/3369

What this PR does / why we need it:

less verbose logging


PR Type

Enhancement


Description

  • Adjusted the log level from Info to Debug in pkg/common/malloc/malloc.go to reduce verbosity in logging.

Changes walkthrough 📝

Relevant files
Enhancement
malloc.go
Adjust log level from `Info` to `Debug` in malloc.go         

pkg/common/malloc/malloc.go

  • Changed log level from Info to Debug for less verbose logging.
+1/-1     

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

Copy link

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Review 🔍

⏱️ Estimated effort to review [1-5]

1, because the PR involves a simple one-line change in the logging level, which is straightforward to review.

🧪 Relevant tests

No

⚡ Possible issues

No

🔒 Security concerns

No

@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label May 30, 2024
Copy link

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Performance
Add a condition to check if debug logging is enabled to avoid unnecessary computation

To avoid potential performance issues, consider wrapping the logging statement in a
condition that checks if the debug level is enabled. This prevents unnecessary computation
of log arguments when the debug level is not enabled.

pkg/common/malloc/malloc.go [42-45]

-logutil.Debug("malloc",
-    zap.Any("max buffer size", maxBufferSize),
-    zap.Any("num shards", numShards),
-    zap.Any("classes", len(classSizes)),
+if logutil.IsDebugEnabled() {
+    logutil.Debug("malloc",
+        zap.Any("max buffer size", maxBufferSize),
+        zap.Any("num shards", numShards),
+        zap.Any("classes", len(classSizes)),
+    )
+}
 
Suggestion importance[1-10]: 8

Why: This is a good suggestion for performance optimization, especially in production environments where debug logging might be turned off. It prevents unnecessary computation of log arguments.

8
Enhancement
Add conditional logging to log at different levels based on the value of n

Consider adding a condition to log at different levels based on the value of n. For
example, log at Debug level if n is below a certain threshold and at Info level otherwise.
This will provide more granular logging and help in debugging.

pkg/common/malloc/malloc.go [42-45]

-logutil.Debug("malloc",
-    zap.Any("max buffer size", maxBufferSize),
-    zap.Any("num shards", numShards),
-    zap.Any("classes", len(classSizes)),
+if n < threshold {
+    logutil.Debug("malloc",
+        zap.Any("max buffer size", maxBufferSize),
+        zap.Any("num shards", numShards),
+        zap.Any("classes", len(classSizes)),
+    )
+} else {
+    logutil.Info("malloc",
+        zap.Any("max buffer size", maxBufferSize),
+        zap.Any("num shards", numShards),
+        zap.Any("classes", len(classSizes)),
+    )
+}
 
Suggestion importance[1-10]: 7

Why: This suggestion is valid as it introduces a dynamic logging level based on the value of n, which can be useful for more effective debugging. However, it's not crucial for functionality, hence the score.

7
Maintainability
Reduce log verbosity by removing unnecessary fields from the debug log

Ensure that the zap.Any fields are necessary for debugging purposes. If some fields are
not critical for debugging, consider removing them to reduce log verbosity.

pkg/common/malloc/malloc.go [42-45]

 logutil.Debug("malloc",
     zap.Any("max buffer size", maxBufferSize),
     zap.Any("num shards", numShards),
-    zap.Any("classes", len(classSizes)),
+)
 
Suggestion importance[1-10]: 5

Why: While reducing log verbosity can help in maintaining cleaner logs, the suggestion does not provide evidence that the fields are indeed unnecessary. More context is needed to validate the removal of these fields.

5

@mergify mergify bot requested a review from sukki37 May 30, 2024 07:18
@mergify mergify bot merged commit 7c951a4 into matrixorigin:1.2-dev May 31, 2024
17 of 18 checks passed
@aylei aylei mentioned this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants