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

update: Tracing is enabled by default #2347

Merged
merged 1 commit into from
Aug 10, 2024
Merged

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Aug 10, 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

Motivation and Context

Following default value of option --tracing is true - mentioned in https://www.selenium.dev/documentation/grid/configuration/cli_options/#logging
Env var SE_ENABLE_TRACING is set to true - it also can be disabled when set to false
Adding env var SE_HTTP_LOGS for option --http-logs with default value is false. Via helm chart, the config key is global.seleniumGrid.httpLogs

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, Documentation


Description

  • Added support for SE_HTTP_LOGS environment variable across multiple startup scripts.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • Updated Helm chart configurations and documentation to include global.seleniumGrid.httpLogs.
  • Added default environment variables for logging and tracing in Dockerfile.

Changes walkthrough 📝

Relevant files
Enhancement
13 files
start-selenium-grid-distributor.sh
Add HTTP logs support and disable tracing by default         

Distributor/start-selenium-grid-distributor.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-grid-eventbus.sh
    Add HTTP logs support and disable tracing by default         

    EventBus/start-selenium-grid-eventbus.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-grid-hub.sh
    Add HTTP logs support and disable tracing by default         

    Hub/start-selenium-grid-hub.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-node.sh
    Add HTTP logs support and disable tracing by default         

    NodeBase/start-selenium-node.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-grid-docker.sh
    Add HTTP logs support and disable tracing by default         

    NodeDocker/start-selenium-grid-docker.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-grid-router.sh
    Add HTTP logs support and disable tracing by default         

    Router/start-selenium-grid-router.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-grid-session-queue.sh
    Add HTTP logs support and disable tracing by default         

    SessionQueue/start-selenium-grid-session-queue.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-grid-sessions.sh
    Add HTTP logs support and disable tracing by default         

    Sessions/start-selenium-grid-sessions.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-standalone.sh
    Add HTTP logs support and disable tracing by default         

    Standalone/start-selenium-standalone.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    start-selenium-grid-docker.sh
    Add HTTP logs support and disable tracing by default         

    StandaloneDocker/start-selenium-grid-docker.sh

  • Added support for SE_HTTP_LOGS environment variable.
  • Disabled tracing by default if SE_ENABLE_TRACING is set to false.
  • +7/-0     
    Dockerfile
    Add default environment variables for logging and tracing

    Base/Dockerfile

  • Added default environment variables for SE_LOG_LEVEL, SE_HTTP_LOGS,
    SE_STRUCTURED_LOGS, and SE_ENABLE_TRACING.
  • +4/-0     
    logging-configmap.yaml
    Add HTTP logs to logging configuration map                             

    charts/selenium-grid/templates/logging-configmap.yaml

    • Added SE_HTTP_LOGS to the logging configuration map.
    +3/-2     
    values.yaml
    Add HTTP logs configuration to Helm chart values                 

    charts/selenium-grid/values.yaml

    • Added httpLogs configuration under global.seleniumGrid.
    +2/-0     
    Tests
    1 files
    chart_test.sh
    Add HTTP logs configuration to Helm chart tests                   

    tests/charts/make/chart_test.sh

  • Added global.seleniumGrid.httpLogs configuration to Helm chart tests.
  • +1/-0     
    Documentation
    1 files
    README.md
    Update documentation for HTTP logs configuration                 

    charts/selenium-grid/README.md

  • Updated documentation to include global.seleniumGrid.httpLogs
    configuration.
  • +17/-14 

    💡 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: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Configuration Clarity
    The comment "Tracing should be enabled to log http logs." might be confusing as it suggests a dependency between HTTP logs and tracing, but the implementation does not enforce this dependency. Consider clarifying or enforcing this dependency in the code.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a validation check for SE_HTTP_LOGS to ensure it contains valid data

    Consider checking the value of SE_HTTP_LOGS before appending it to SE_OPTS to ensure
    it contains valid data that won't break the command line arguments.

    Distributor/start-selenium-grid-distributor.sh [62-64]

    -if [ ! -z "$SE_HTTP_LOGS" ]; then
    +if [ ! -z "$SE_HTTP_LOGS" ] && [ "$SE_HTTP_LOGS" != "false" ]; then
       echo "Appending Selenium options: --http-logs ${SE_HTTP_LOGS}"
       SE_OPTS="$SE_OPTS --http-logs ${SE_HTTP_LOGS}"
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a validation check for SE_HTTP_LOGS ensures that invalid values do not disrupt the command line arguments, enhancing script robustness.

    8
    Ensure SE_HTTP_LOGS is valid before using it in command options

    To avoid potential script errors, ensure that SE_HTTP_LOGS is not empty or set to
    'false' before appending it to SE_OPTS.

    EventBus/start-selenium-grid-eventbus.sh [27-29]

    -if [ ! -z "$SE_HTTP_LOGS" ]; then
    +if [ ! -z "$SE_HTTP_LOGS" ] && [ "$SE_HTTP_LOGS" != "false" ]; then
       echo "Appending Selenium options: --http-logs ${SE_HTTP_LOGS}"
       SE_OPTS="$SE_OPTS --http-logs ${SE_HTTP_LOGS}"
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This validation prevents potential script errors by ensuring SE_HTTP_LOGS contains a valid value, improving the reliability of the script.

    8
    Check SE_HTTP_LOGS for valid content before appending to Selenium options

    Validate the SE_HTTP_LOGS variable to ensure it does not disrupt the Selenium
    options if set to an inappropriate value.

    Hub/start-selenium-grid-hub.sh [30-32]

    -if [ ! -z "$SE_HTTP_LOGS" ]; then
    +if [ ! -z "$SE_HTTP_LOGS" ] && [ "$SE_HTTP_LOGS" != "false" ]; then
       echo "Appending Selenium options: --http-logs ${SE_HTTP_LOGS}"
       SE_OPTS="$SE_OPTS --http-logs ${SE_HTTP_LOGS}"
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Checking SE_HTTP_LOGS for valid content before appending it to Selenium options prevents potential disruptions, enhancing script stability.

    8
    Ensure SE_HTTP_LOGS is appropriately checked before use

    Add a condition to check if SE_HTTP_LOGS is not 'false' before appending it to the
    Selenium options to prevent command errors.

    NodeBase/start-selenium-node.sh [69-71]

    -if [ ! -z "$SE_HTTP_LOGS" ]; then
    +if [ ! -z "$SE_HTTP_LOGS" ] && [ "$SE_HTTP_LOGS" != "false" ]; then
       echo "Appending Selenium options: --http-logs ${SE_HTTP_LOGS}"
       SE_OPTS="$SE_OPTS --http-logs ${SE_HTTP_LOGS}"
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Ensuring SE_HTTP_LOGS is appropriately checked before use prevents command errors, improving the robustness and reliability of the script.

    8

    @VietND96 VietND96 merged commit b49b13b into SeleniumHQ:trunk Aug 10, 2024
    14 of 16 checks passed
    @VietND96 VietND96 added this to the 4.23.1 milestone Aug 10, 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.

    1 participant