Skip to content

[dotnet] Treat SM's logs always as Trace to avoid SM writing at Info level#14667

Merged
nvborisenko merged 1 commit intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-sm-log-output
Oct 28, 2024
Merged

[dotnet] Treat SM's logs always as Trace to avoid SM writing at Info level#14667
nvborisenko merged 1 commit intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-sm-log-output

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

@nvborisenko nvborisenko commented Oct 28, 2024 •
edited
Loading

User description

Description

I ran my console program and saw unexpected console output from Selenium Manager.

20:03:03.469  INFO SeleniumManager: Driver path: C:\Users\Nick\.cache\selenium\chromedriver\win64\130.0.6723.69\chromedriver.exe
20:03:03.481  INFO SeleniumManager: Browser path: C:\Program Files\Google\Chrome\Application\chrome.exe
Starting ChromeDriver 130.0.6723.69 (3ec172b971b9478c515a58f591112c5c23fa4965-refs/branch-heads/6723@{#1452}) on port 52914
Only local connections are allowed.
Please see https://chromedriver.chromium.org/security-considerations for suggestions on keeping ChromeDriver safe.
ChromeDriver was started successfully on port 52914.

Motivation and Context

Nothing should go to the user's console if not enabled explicitly.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement

Description

  • Updated the Selenium Manager logging mechanism to treat all logs as Trace level, ensuring no logs are written at the Info level.
  • Removed the conditional checks for different log levels (WARN, DEBUG, INFO) to streamline the logging process.
  • This change addresses the issue of unexpected console output by ensuring that logs are only recorded if the Trace level is enabled.

Changes walkthrough 📝

Relevant files
Enhancement
SeleniumManager.cs
Simplify log handling by treating all logs as Trace           

dotnet/src/webdriver/SeleniumManager.cs

  • Changed log handling to treat all Selenium Manager logs as Trace
    level.
  • Removed conditional checks for different log levels (WARN, DEBUG,
    INFO).
  • Simplified logging by using a single Trace log entry format.
    		•  +4/-20   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 28, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    1452 - Not compliant

    Not compliant requirements:

    • Reduce the number of TCP ports held by WebDriver
    • Address resource problems caused by thousands of sockets stuck in TIME_WAIT
    • Improve performance of concurrent Selenium tests
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Information Loss
    The change treats all log levels as Trace, which might lead to loss of important information that was previously logged at higher levels (WARN, INFO).

    Performance Concern
    The new implementation still iterates through all log entries, potentially impacting performance if there are many log entries.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 28, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Performance
    Optimize log level checking by moving it outside the loop and combining it with the null check

    Consider moving the log level check outside the loop to improve performance by
    avoiding repeated checks for each log entry.

    dotnet/src/webdriver/SeleniumManager.cs [199-205]

    -if (_logger.IsEnabled(LogEventLevel.Trace))
+if (_logger.IsEnabled(LogEventLevel.Trace) && jsonResponse.Logs != null)
 {
     foreach (var entry in jsonResponse.Logs)
     {
         _logger.Trace($"{entry.Level} {entry.Message}");
     }
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves performance by reducing redundant checks within the loop, which is a beneficial optimization, especially if the log entries are numerous.

    		7
    Enhancement
    Enhance log message formatting using string interpolation for better readability

    Consider using string interpolation for the log message to improve readability and
    potentially performance.

    dotnet/src/webdriver/SeleniumManager.cs [203]

    -_logger.Trace($"{entry.Level} {entry.Message}");
+_logger.Trace($"{entry.Level}: {entry.Message}");
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion slightly improves readability by adding a colon between the log level and message, but the impact on performance is negligible.

    		5

    💡 Need additional feedback ? start a PR chat

    @nvborisenko
    Copy link
    Copy Markdown
    Member Author

    nvborisenko commented Oct 28, 2024

    Perfect, now I don't unexpected logs in my console output.

    @nvborisenko nvborisenko merged commit b2702ca into SeleniumHQ:trunk Oct 28, 2024
    @nvborisenko nvborisenko deleted the dotnet-sm-log-output branch February 6, 2025 18:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @nvborisenko