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

Docker: Update condition to enable tracing to avoid annoying warning messages #2541

Merged
merged 1 commit into from
Dec 27, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Dec 27, 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

Fixes #2355
Fixes #2415
Fixes #2423
Fixes #2432
Fixes #2481

By default, SE_ENABLE_TRACING is true to align with CLI option config https://www.selenium.dev/documentation/grid/configuration/cli_options/#logging
It is recommended that the user enable observability for tracing issues if having any. However, by default, it auto config the endpoint to a local OTLP localhost/[0:0:0:0:0:0:0:1]:4317 and raises the warning if endpoint is unreachable.

ERROR (ThrottlingLogger.dolog) Failed to export spans.
  The request could not be executed. Error message: Failed to connect to localhost/[0:0:0:0:0:0:0:1]:4117
  java.net.ConnectException: Failed to connect to localhost/[0:0:0:0:0:0:0:1]:4317 
at okhttp3.internal.connection.RealConnection.connectSocket(RealConnection.kt:297)
at okhttp3.internal.connection. ExchangeFinder.findConnection (Exchangefinder.kt: 226)
at okhttp3.internal.connection.okhttps.internal.connection.RealConnection.connect(RealConnection.kt:207)

This warning might annoy users or cause messed up logs to identify another real issue.

Instead of repetitively asking the user to add env var SE_ENABLE_TRACING=false, updating the condition to enable this feature should be SE_ENABLE_TRACING=true, and users have to input a correct endpoint to env var SE_OTEL_EXPORTER_ENDPOINT. Otherwise, it is disabled.

Motivation and Context

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

  • Enhanced tracing configuration across all Selenium Grid components to prevent unnecessary warning messages
  • Modified tracing condition to require both SE_ENABLE_TRACING=true and SE_OTEL_EXPORTER_ENDPOINT to be set
  • Removed default SE_ENABLE_TRACING=false from all docker-compose files to simplify configuration
  • Updated all component scripts (Hub, Node, Router, etc.) to use consistent tracing condition

Changes walkthrough 📝

Relevant files
Enhancement
start-selenium-grid-distributor.sh
Update tracing condition in distributor script                     

Distributor/start-selenium-grid-distributor.sh

  • Updated tracing condition to require both SE_ENABLE_TRACING=true and
    SE_OTEL_EXPORTER_ENDPOINT to be set
  • +1/-1     
    start-selenium-grid-hub.sh
    Update tracing condition in hub script                                     

    Hub/start-selenium-grid-hub.sh

  • Modified tracing condition to check for both SE_ENABLE_TRACING and
    SE_OTEL_EXPORTER_ENDPOINT
  • +1/-1     
    Configuration changes
    docker-compose-v3.yml
    Remove tracing configuration from compose file                     

    docker-compose-v3.yml

    • Removed SE_ENABLE_TRACING environment variable from all services
    +0/-5     

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

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Validation Check

    The condition for enabling tracing requires both SE_ENABLE_TRACING=true and SE_OTEL_EXPORTER_ENDPOINT to be set. Verify that tracing is properly disabled when either condition is not met.

    if [ "${SE_ENABLE_TRACING}" = "true" ] && [ -n "${SE_OTEL_EXPORTER_ENDPOINT}" ]; then

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Improve error handling by providing clear feedback when configuration is incomplete

    Add error message when tracing is enabled but endpoint is missing, to help users
    understand why tracing isn't working.

    EventBus/start-selenium-grid-eventbus.sh [84]

    -if [ "${SE_ENABLE_TRACING}" = "true" ] && [ -n "${SE_OTEL_EXPORTER_ENDPOINT}" ]; then
    +if [ "${SE_ENABLE_TRACING}" = "true" ]; then
    +  if [ -z "${SE_OTEL_EXPORTER_ENDPOINT}" ]; then
    +    echo "Warning: Tracing enabled but SE_OTEL_EXPORTER_ENDPOINT not set. Tracing will be disabled."
    +  else
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding an error message when SE_OTEL_EXPORTER_ENDPOINT is missing would significantly improve user experience by providing clear feedback about why tracing is not working, helping with troubleshooting.

    8
    Enhance variable existence check to handle special characters and edge cases in shell scripts

    Add quotes around the variable reference in the condition to handle cases where the
    variable might contain spaces or special characters.

    EventBus/start-selenium-grid-eventbus.sh [84]

    -if [ "${SE_ENABLE_TRACING}" = "true" ] && [ -n "${SE_OTEL_EXPORTER_ENDPOINT}" ]; then
    +if [ "${SE_ENABLE_TRACING}" = "true" ] && [ -n "${SE_OTEL_EXPORTER_ENDPOINT:+x}" ]; then
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While the suggestion to use :+x parameter expansion is technically valid for robust shell scripting, the current -n check is sufficient for this use case since the variable is already properly quoted.

    3

    @VietND96 VietND96 merged commit 02ea1a7 into trunk Dec 27, 2024
    29 of 36 checks passed
    @VietND96 VietND96 deleted the tracing-enable branch December 27, 2024 10:42
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment