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

[java] Added a check for intentionally existing loggers #14754

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

JHeflinger
Copy link

@JHeflinger JHeflinger commented Nov 13, 2024

User description

Description

A check for the existence of the handlers property in the LogManager is added in the configureLogging() function as to ensure that intentionally existing loggers such as ones defined in the logging.properties file are not overwritten

Motivation and Context

This is to allow users to use their own loggers and formatting with the grid program.

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.

Fixes #14160


PR Type

Bug fix


Description

  • Added a check in the configureLogging method to ensure that existing loggers, such as those defined in the logging.properties file, are not overwritten.
  • This change allows users to maintain their custom loggers and formatting when using the grid program.

Changes walkthrough 📝

Relevant files
Bug fix
LoggingOptions.java
Add check to preserve existing loggers in LoggingOptions 

java/src/org/openqa/selenium/grid/log/LoggingOptions.java

  • Added a check for existing loggers in configureLogging.
  • Prevents overwriting of loggers defined in logging.properties.
  • +3/-0     

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

    A check for the handlers property of the LogManager is added
    as to ensure that intentionally existing loggers such as ones
    defined in the logging.properties file are not overwritten
    
    Fixes SeleniumHQ#14160
    @CLAassistant
    Copy link

    CLAassistant commented Nov 13, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Race Condition
    The check for handlers property and early return could create a race condition if other threads are configuring logging simultaneously. Consider adding synchronization.

    Incomplete Check
    Checking only for 'handlers' property may not be sufficient to determine if loggers are properly configured. Consider checking for other essential logging properties.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Enhance logger existence check by verifying actual handler instances rather than a generic property

    Check for specific logger handlers instead of the generic 'handlers' property, as
    individual loggers might have their own handlers configured.

    java/src/org/openqa/selenium/grid/log/LoggingOptions.java [102]

    -if (LogManager.getLogManager().getProperty("handlers") != null) return;
    +if (Arrays.stream(LogManager.getLogManager().getLoggerNames().asIterator().asIterator().toArray())
    +        .anyMatch(name -> LogManager.getLogManager().getLogger(name.toString()).getHandlers().length > 0)) return;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion proposes a more thorough approach to detect existing logger configurations by checking actual handlers on each logger, which is more reliable than checking a single property. This would prevent edge cases where loggers are configured differently.

    7
    Maintainability
    Improve debugging capabilities by logging when custom configurations are detected

    Add logging statement to indicate when custom logging configuration is detected and
    preserved.

    java/src/org/openqa/selenium/grid/log/LoggingOptions.java [102]

    -if (LogManager.getLogManager().getProperty("handlers") != null) return;
    +if (LogManager.getLogManager().getProperty("handlers") != null) {
    +    Logger.getLogger(LoggingOptions.class.getName()).info("Preserving existing logging configuration");
    +    return;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: Adding a log message would help with debugging and observability, making it clearer when custom logging configurations are preserved. However, this is a minor enhancement with moderate impact on maintainability.

    4

    💡 Need additional feedback ? start a PR chat

    @shbenzer
    Copy link
    Contributor

    @JHeflinger Thank you for the PR! Before it can be evaluated, please sign the Contributor License Agreement.

    @shbenzer
    Copy link
    Contributor

    shbenzer commented Nov 14, 2024

    After looking at this PR, could you please add a test to ensure this functionality works as expected.

    @VietND96 VietND96 changed the title Added a check for intentionally existing loggers [java] Added a check for intentionally existing loggers Nov 15, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: [GRID] logging.properties is ignored when trying to remove stack trace errors from Logs
    5 participants