-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
feat: improvement to logger #2396
Conversation
@odilitime this has the ability to add child loggers for agentId's and/or plugins, but I will not implement it at this time. But it has opened the door for it. |
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive logging enhancement across the project. By integrating the Changes
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@coderabbitai try again |
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/runtime.ts (1)
473-473
: Simplify unnecessary ternary operatorThe ternary operator
${this.character.settings.ragKnowledge ? true : false}
is unnecessary. You can directly use${this.character.settings.ragKnowledge}
.Apply this diff to simplify the expression:
- `[RAG Check] RAG Knowledge enabled: ${this.character.settings.ragKnowledge ? true : false}` + `[RAG Check] RAG Knowledge enabled: ${this.character.settings.ragKnowledge}`🧰 Tools
🪛 Biome (1.9.4)
[error] 473-473: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
.env.example (1)
22-22
: Consider specifying the default value in the comment.While the comment mentions "false by default", consider making it more visible by adding it directly after the variable:
-LOG_JSON_FORMAT= # Print everything in logger as json; false by default +LOG_JSON_FORMAT=false # Print everything in logger as json
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
.env.example
(1 hunks)packages/core/package.json
(2 hunks)packages/core/src/logger.ts
(1 hunks)packages/core/src/runtime.ts
(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/runtime.ts
[error] 473-473: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/core/src/logger.ts
[error] 43-43: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 58-58: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: smoke-tests
- GitHub Check: integration-tests
🔇 Additional comments (3)
packages/core/package.json (1)
74-74
: Dependencies updated appropriatelyThe addition of
pino
andpino-pretty
, and the removal of@tavily/core
, align with the new logging mechanism.Also applies to: 89-90
.env.example (2)
21-22
: LGTM! Clear and well-documented logging configuration.The new logging section is well-placed and the comment clearly explains the purpose of
LOG_JSON_FORMAT
.
21-23
: Verify logging configuration documentation.Let's check if there's any additional documentation about the logging configuration in the repository.
✅ Verification successful
LOG_JSON_FORMAT is properly documented
The variable is well documented in .env.example with a clear description and default value (false). It's part of the core logging configuration system alongside other logging-related settings like LOG_LEVEL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for logging configuration documentation rg -i "log.*json.*format|json.*log.*format" README.md docs/Length of output: 333
Script:
#!/bin/bash # Search for logging-related documentation and configuration rg -i "logging|log_" README.md docs/ -A 2 fd -e yaml -e yml -e json | xargs rg -i "logging|log_" rg -i "env.*example|environment.*variable" README.md docs/ -A 2Length of output: 46620
@wtfsayo the changes look great! the logger implementation is now much cleaner and more maintainable with the following improvements:
the implementation is now more robust and follows industry best practices. 🔗 analysis chain 🏁 scripts executed the following scripts were executed for the analysis: script:
length of output: 21075 ✏️ learnings added
✅ Actions performedComments resolved. |
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.
lgtm, tested
@JoeyKhd with those changes it looks like the agent is only showing the |
@snobbee i have tested all log levels, will have another look |
The current implementation of the elizaLogger is very chaotic and messy.
This is a better and more visible way of displaying logs:
With the ability to print raw log messages which could be plugged into a tool like Grafana. This has to be enabled in the .env.
With LOG_JSON_FORMAT set to true in .env: