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

add: Env var SE_SUPERVISORD_LOG_LEVEL to set supervisord log level #2317

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jul 21, 2024

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

add: Env var SE_SUPERVISORD_LOG_LEVEL to set supervisord log level

Motivation and Context

Fix: #2301

Based on supervisord log level support http://supervisord.org/logging.html
supervisord.conf file also supports env variable to pass dynamic value for config
Add env var SE_SUPERVISORD_LOG_LEVEL (default value is info) to the Base and video recorder images.

In helm chart, the env var is set via config key serverConfigMap.env.SE_SUPERVISORD_LOG_LEVEL

Accept values: critical, error, warn, info, debug, trace, blather

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, Configuration changes


Description

  • Added SE_SUPERVISORD_LOG_LEVEL environment variable to Base and Video Dockerfiles with a default value of info.
  • Updated supervisord.conf files to use the SE_SUPERVISORD_LOG_LEVEL environment variable for setting log levels.
  • Added SE_SUPERVISORD_LOG_LEVEL to Helm chart values and test configurations.
  • Set SE_SUPERVISORD_LOG_LEVEL to error in various test configurations.

Changes walkthrough 📝

Relevant files
Enhancement
Dockerfile
Add `SE_SUPERVISORD_LOG_LEVEL` environment variable           

Base/Dockerfile

  • Added SE_SUPERVISORD_LOG_LEVEL environment variable with default value
    info.
  • +2/-1     
    Dockerfile
    Add `SE_SUPERVISORD_LOG_LEVEL` environment variable           

    Video/Dockerfile

  • Added SE_SUPERVISORD_LOG_LEVEL environment variable with default value
    info.
  • +2/-1     
    Configuration changes
    supervisord.conf
    Use `SE_SUPERVISORD_LOG_LEVEL` in supervisord configuration

    Base/supervisord.conf

  • Updated loglevel to use SE_SUPERVISORD_LOG_LEVEL environment variable.
  • Added username and password placeholders for unix_http_server.
  • +3/-1     
    supervisord.conf
    Use `SE_SUPERVISORD_LOG_LEVEL` in supervisord configuration

    Video/supervisord.conf

  • Updated loglevel to use SE_SUPERVISORD_LOG_LEVEL environment variable.

  • +1/-1     
    values.yaml
    Add `SE_SUPERVISORD_LOG_LEVEL` to Helm chart values           

    charts/selenium-grid/values.yaml

  • Added SE_SUPERVISORD_LOG_LEVEL to serverConfigMap with default value
    info.
  • +2/-0     
    Tests
    base-auth-ingress-values.yaml
    Set `SE_SUPERVISORD_LOG_LEVEL` in test configuration         

    tests/charts/ci/base-auth-ingress-values.yaml

    • Set SE_SUPERVISORD_LOG_LEVEL to error in test configuration.
    +4/-0     
    docker-compose-v3-test-parallel.yml
    Add `SE_SUPERVISORD_LOG_LEVEL` to docker-compose test configuration

    tests/docker-compose-v3-test-parallel.yml

  • Added SE_SUPERVISORD_LOG_LEVEL with value error to multiple services.
  • +4/-0     
    docker-compose-v3-test-video.yml
    Add `SE_SUPERVISORD_LOG_LEVEL` to video test configuration

    tests/docker-compose-v3-test-video.yml

  • Added SE_SUPERVISORD_LOG_LEVEL with value error to video service.
  • +1/-0     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Configuration Consistency
    The addition of username and password in the supervisord configuration without corresponding environment variables or documentation might lead to configuration errors or security concerns if not handled properly.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add a default value for the supervisord log level environment variable to ensure stability

    Ensure that the environment variable ENV_SE_SUPERVISORD_LOG_LEVEL is always set
    before using it in the supervisord configuration to avoid configuration errors. You
    can provide a default value directly in the supervisord configuration file.

    Base/supervisord.conf [8]

    -loglevel=%(ENV_SE_SUPERVISORD_LOG_LEVEL)s
    +loglevel=%(ENV_SE_SUPERVISORD_LOG_LEVEL|info)s
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding a default value for the environment variable ensures that the configuration does not fail if the variable is not set, improving stability and reliability.

    9
    Enhancement
    Parameterize the supervisord log level to allow flexibility in different environments

    Ensure that the environment variable SE_SUPERVISORD_LOG_LEVEL is set to a valid
    supervisord log level. Currently, it is hardcoded to "info", which might not be
    suitable for all deployment environments.

    Video/Dockerfile [112]

    -SE_SUPERVISORD_LOG_LEVEL="info"
    +SE_SUPERVISORD_LOG_LEVEL=${SUPERVISORD_LOG_LEVEL:-"info"}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Parameterizing the log level allows for greater flexibility and adaptability across different deployment environments, enhancing the configuration's robustness.

    8
    Maintainability
    Standardize the supervisord log level environment variable usage across services

    Use a consistent environment variable for the supervisord log level across different
    services in the Docker Compose file to avoid confusion and potential
    misconfiguration.

    tests/docker-compose-v3-test-parallel.yml [26-80]

    -- SE_SUPERVISORD_LOG_LEVEL=error
    +- SE_SUPERVISORD_LOG_LEVEL=${SUPERVISORD_LOG_LEVEL:-error}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a consistent environment variable across services reduces the risk of misconfiguration and improves maintainability.

    7
    Documentation
    Improve the documentation for supervisord log level settings

    Document the acceptable values for SE_SUPERVISORD_LOG_LEVEL in the comments to guide
    users in setting appropriate log levels.

    charts/selenium-grid/values.yaml [250-251]

    -# Log level of supervisord. Accept values: critical, error, warn, info, debug, trace, blather (http://supervisord.org/logging.html)
    +# Log level of supervisord. Acceptable values: critical, error, warn, info, debug, trace, blather. See more at http://supervisord.org/logging.html
     SE_SUPERVISORD_LOG_LEVEL: "info"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Enhancing the documentation helps users understand the acceptable values for the log level, promoting correct usage and configuration.

    6

    @VietND96 VietND96 merged commit 582fb2c into SeleniumHQ:trunk Jul 21, 2024
    16 checks passed
    @VietND96 VietND96 added this to the 4.23.0 milestone Jul 21, 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]: NodeChrome ignores log level and floods INFO logs
    1 participant