fix(client): Add a new console class that allow us show extra info passed to log #21170
Conversation
WalkthroughThe changes introduce a new logging mechanism in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LoggerFactory
participant ConsoleFormattedStream
Client->>LoggerFactory: Request logger stream
LoggerFactory->>LoggerFactory: Check process.env.DETAILED_LOGS
alt DETAILED_LOGS is true
LoggerFactory->>ConsoleFormattedStream: Use ConsoleFormattedStream
else DETAILED_LOGS is false
LoggerFactory->>ConsoleStream: Use ConsoleStream
end
LoggerFactory-->>Client: Return selected logger stream
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
bigbluebutton-html5/webpack.config.js (1)
9-9: LGTM with a minor suggestion.The addition of the
detailedLogsconstant aligns well with the PR objectives. It correctly uses theDETAILED_LOGSenvironment variable to control the logging detail.Consider using nullish coalescing operator (
??) instead of logical OR (||) to handle cases whereDETAILED_LOGSmight be intentionally set to falsy values like empty string:const detailedLogs = process.env.DETAILED_LOGS ?? false;This ensures that only
nullorundefinedvalues trigger the defaultfalse.bigbluebutton-html5/imports/startup/client/logger/index.ts (2)
41-41: LGTM: Conditional stream selection implemented correctly.The implementation aligns well with the PR objective of introducing an environment variable to enable detailed logs. The condition correctly checks if DETAILED_LOGS is truthy, using the new ConsoleStream when true and falling back to ConsoleFormattedStream otherwise.
For improved clarity, consider using explicit boolean comparison:
- Stream = process.env.DETAILED_LOGS ? ConsoleStream : ConsoleFormattedStream; + Stream = process.env.DETAILED_LOGS === 'true' ? ConsoleStream : ConsoleFormattedStream;This change would make it clear that the environment variable is expected to be a string 'true' rather than a boolean.
ConsoleStream Implementation Incomplete
The
ConsoleStreamclass in./bigbluebutton-html5/imports/startup/client/logger/consoleStream.tsdoes not implement the expectedwritemethod as defined in the verification pattern. This may affect the functionality of toggling detailed logs using theDETAILED_LOGSenvironment variable.
- File:
./bigbluebutton-html5/imports/startup/client/logger/consoleStream.ts- Issue: Missing or incorrectly implemented
writemethod inConsoleStreamclass.Please review and update the
ConsoleStreamimplementation to ensure it includes the necessary methods for proper logging functionality.🔗 Analysis chain
Line range hint
1-124: Overall implementation meets PR objectives with room for minor improvements.The changes successfully implement the desired functionality of using an environment variable (DETAILED_LOGS) to toggle detailed logs. The implementation is correct and aligns well with the PR objectives.
To further improve the code:
- Consider using explicit boolean comparison for the DETAILED_LOGS environment variable.
- Refactor the switch statement to reduce code duplication.
These minor improvements would enhance code clarity and maintainability without changing the core functionality.
To ensure that the new ConsoleStream is properly implemented and imported, run the following verification script:
This script will help verify the proper implementation and usage of the new ConsoleStream and the DETAILED_LOGS environment variable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and import of ConsoleStream # Test 1: Check if ConsoleStream file exists fd --type file --extension ts --exec echo "ConsoleStream file found: {}" consoleStream.ts # Test 2: Verify ConsoleStream implementation ast-grep --lang typescript --pattern $'class ConsoleStream { $$$ write($_) { $$$ } $$$ }' consoleStream.ts # Test 3: Check usage of DETAILED_LOGS environment variable rg --type typescript 'process\.env\.DETAILED_LOGS'Length of output: 45903
Script:
#!/bin/bash # Description: Enhanced verification of ConsoleStream implementation and DETAILED_LOGS usage # Test 1: Find all consoleStream.ts files fd --type file --extension ts --name 'consoleStream.ts' --exec echo "Found ConsoleStream.ts: {}" \; # Test 2: Verify ConsoleStream implementation in each found file fd --type file --extension ts --name 'consoleStream.ts' | while read -r file; do echo "Checking implementation in $file" ast-grep --lang typescript --pattern $'class ConsoleStream {\n $$$\n write($_) {\n $$$\n }\n $$$\n}' "$file" done # Test 3: Check usage of DETAILED_LOGS environment variable rg 'process\.env\.DETAILED_LOGS' --glob '*.ts'Length of output: 1042
Script:
#!/bin/bash # Description: Corrected verification of ConsoleStream implementation and DETAILED_LOGS usage # Test 1: Find all consoleStream.ts files fd --type file --extension ts 'consoleStream.ts' --exec echo "Found ConsoleStream.ts: {}" \; # Test 2: Verify ConsoleStream implementation in each found file fd --type file --extension ts 'consoleStream.ts' | while read -r file; do echo "Checking implementation in $file" ast-grep --lang typescript --pattern 'class ConsoleStream {\n $$$\n write($_) {\n $$$\n }\n $$$\n}' "$file" done # Test 3: Check usage of DETAILED_LOGS environment variable rg 'process\.env\.DETAILED_LOGS' -g '*.ts'Length of output: 967
bigbluebutton-html5/package.json (1)
6-6: LGTM! Consider making the detailed logging configurable.The addition of
DETAILED_LOGS=trueto the start script aligns well with the PR objective to enhance logging capabilities during development. This change ensures that detailed logs are available when running the development server.Consider making this configurable by using a conditional environment variable:
- "start": "NODE_ENV=development DETAILED_LOGS=true webpack serve --open --config webpack.config.js", + "start": "NODE_ENV=development DETAILED_LOGS=${DETAILED_LOGS:-true} webpack serve --open --config webpack.config.js",This change would allow developers to override the default
truevalue if needed, providing more flexibility.bigbluebutton-html5/imports/startup/client/logger/consoleStream.ts (1)
45-45: Typo in comment on line 45The comment contains a typographical error. The word 'spacs' should be corrected to 'spaces' for clarity.
Apply this diff to fix the typo:
- //get level name and pad start with spacs + //get level name and pad start with spaces
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
bigbluebutton-html5/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
- bigbluebutton-html5/imports/startup/client/logger/consoleStream.ts (1 hunks)
- bigbluebutton-html5/imports/startup/client/logger/index.ts (2 hunks)
- bigbluebutton-html5/package.json (2 hunks)
- bigbluebutton-html5/webpack.config.js (2 hunks)
🧰 Additional context used
🪛 Biome
bigbluebutton-html5/imports/startup/client/logger/consoleStream.ts
[error] 99-99: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
bigbluebutton-html5/webpack.config.js (2)
Line range hint
1-146: Overall implementation looks good.The changes to the webpack configuration file successfully implement the detailed logs feature as described in the PR objectives. The
DETAILED_LOGSenvironment variable is correctly defined and made accessible to the application code.A few points to consider:
- Ensure that the application code is updated to utilize
process.env.DETAILED_LOGSfor controlling log verbosity.- Update the project documentation to explain how to use the
DETAILED_LOGSenvironment variable.- Consider adding a test case that verifies the behavior of the application with different
DETAILED_LOGSvalues.Great job on implementing this feature!
42-42: LGTM. Verify the implementation in application code.The addition of
'process.env.DETAILED_LOGS': detailedLogs,to thewebpack.DefinePluginconfiguration correctly makes theDETAILED_LOGSenvironment variable accessible within the application code. This implementation aligns with the PR objectives and follows webpack best practices.To ensure the
DETAILED_LOGSenvironment variable is being used correctly in the application code, please run the following script:This script will help verify that the
DETAILED_LOGSenvironment variable is being utilized appropriately in the codebase.bigbluebutton-html5/imports/startup/client/logger/index.ts (1)
8-8: LGTM: ConsoleStream import added correctly.The import statement for ConsoleStream is correctly placed and follows the existing import style in the file. This import is necessary for the subsequent usage of ConsoleStream in the createStreamForTarget function.
bigbluebutton-html5/package.json (1)
44-44: LGTM! Verify usage of the new dependency.The addition of
@browser-bunyan/levelsaligns with the PR objective to enhance logging capabilities. This package is likely used to define log levels for the browser-bunyan logging framework.To ensure this dependency is properly utilized, please run the following script to check for its usage in the codebase:
This script will help confirm that the newly added dependency is actually being used in the project.
✅ Verification successful
Verification Successful: Dependency Usage Confirmed
The newly added dependency
@browser-bunyan/levelsis being utilized inimports/startup/client/logger/consoleStream.ts, ensuring it is properly integrated into the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of @browser-bunyan/levels in the codebase # Search for import statements or require calls for @browser-bunyan/levels echo "Searching for usage of @browser-bunyan/levels:" rg --type js --type ts "from\s+['\"]@browser-bunyan/levels['\"]|require\(['\"]@browser-bunyan/levels['\"]\)" # If no results are found, it might indicate that the package is not yet used if [ $? -ne 0 ]; then echo "Warning: No usage of @browser-bunyan/levels found. Ensure it's being used in the codebase." fiLength of output: 339
bigbluebutton-html5/imports/startup/client/logger/consoleStream.ts
Outdated
Show resolved
Hide resolved
bigbluebutton-html5/imports/startup/client/logger/consoleStream.ts
Outdated
Show resolved
Hide resolved
…ition using optional chaining
|
antobinary
changed the title
Automated tests Summary✅ All the CI tests have passed! |
What does this PR do?
This PR adds a env variable to show detailed logs on browser-bunyan, also I had to create a new console stream to proccess this new data based on this issue on browser-bunyan repository 104
OBS: I used a env variable because of some limitation on settings being fetched after the logger be loaded
Closes Issue(s)
#21053
How to test
Change the env variable
DETAILED_LOGStotrueand look at browser logs