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 log timestamp in custom scripts align with supervisord #2441

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Oct 23, 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

Synchronize logs timestamp between additional scripts and pid logs from supervisord

Elapsed time:         0.0s
2024-10-23T06:36:02UTC [video.recorder] - Ready to shutdown the recorder
2024-10-23 06:36:02,488 INFO exited: video-recording (exit status 0; expected)
2024-10-23 06:36:02,488 WARN received SIGTERM indicating exit request
2024-10-23 06:36:02,489 INFO waiting for video-ready, video-upload to die
2024-10-23T06:36:02UTC [video.uploader] - Trapped SIGTERM/SIGINT/x so shutting down uploader
2024-10-23T06:36:02UTC [video.uploader] - Start consuming pipe file to upload

After

Elapsed time:         0.0s
2024-10-23 10:06:43,737 [video.recorder] - Ready to shutdown the recorder
2024-10-23 10:06:43,738 INFO exited: video-recording (exit status 0; expected)
2024-10-23 10:06:43,739 WARN received SIGTERM indicating exit request
2024-10-23 10:06:43,739 INFO waiting for video-ready, video-upload to die
2024-10-23 10:06:43,833 [video.uploader] - Trapped SIGTERM/SIGINT/x so shutting down uploader
2024-10-23 10:06:43,843 [video.uploader] - Start consuming pipe file to upload

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


Description

  • Introduced a new timestamp format variable ts_format across multiple scripts to standardize log timestamps.
  • Updated log statements in various Bash scripts to use the new timestamp format.
  • Added a new environment variable SE_LOG_TIMESTAMP_FORMAT in the Dockerfile for log timestamp configuration.
  • Updated Kubernetes versions in CircleCI and GitHub Actions workflows for various test strategies.

Changes walkthrough 📝

Relevant files
Enhancement
7 files
upload.sh
Synchronize log timestamps in upload script                           

Video/upload.sh

  • Introduced a new timestamp format variable ts_format.
  • Updated log statements to use the new timestamp format.
  • +11/-10 
    validate_endpoint.sh
    Update log timestamps in endpoint validation script           

    Video/validate_endpoint.sh

  • Added a new timestamp format variable ts_format.
  • Updated log statements to use the new timestamp format.
  • +4/-3     
    video.sh
    Standardize log timestamps in video recording script         

    Video/video.sh

  • Added a new timestamp format variable ts_format.
  • Updated multiple log statements to use the new timestamp format.
  • +23/-22 
    distributorProbe.sh
    Update timestamp format in distributor probe script           

    charts/selenium-grid/configs/distributor/distributorProbe.sh

  • Changed timestamp format to a new standard.
  • Updated log statements to use the new timestamp format.
  • +6/-6     
    nodePreStop.sh
    Standardize log timestamps in node pre-stop script             

    charts/selenium-grid/configs/node/nodePreStop.sh

  • Introduced a new timestamp format variable ts_format.
  • Updated log statements to use the new timestamp format.
  • +14/-13 
    nodeProbe.sh
    Synchronize log timestamps in node probe script                   

    charts/selenium-grid/configs/node/nodeProbe.sh

  • Added a new timestamp format variable ts_format.
  • Updated log statements to use the new timestamp format.
  • +11/-10 
    routerProbe.sh
    Update timestamp format in router probe script                     

    charts/selenium-grid/configs/router/routerProbe.sh

  • Changed timestamp format to a new standard.
  • Updated log statements to use the new timestamp format.
  • +4/-4     
    Configuration changes
    3 files
    config.yml
    Update Kubernetes versions in CircleCI configuration         

    .circleci/config.yml

    • Updated Kubernetes versions for various test workflows.
    +3/-3     
    helm-chart-test.yml
    Update Kubernetes versions in GitHub Actions workflow       

    .github/workflows/helm-chart-test.yml

    • Updated Kubernetes versions for Helm chart test workflows.
    +3/-3     
    Dockerfile
    Add log timestamp format environment variable                       

    Base/Dockerfile

    • Added a new environment variable SE_LOG_TIMESTAMP_FORMAT.
    +1/-0     

    💡 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: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Bug
    The new timestamp format may not be compatible with all systems or tools that parse these logs. Verify that the new format works correctly across all intended use cases.

    Performance Concern
    The script is using multiple curl commands which could potentially slow down the probe process. Consider combining these requests or using a more efficient method to check the status.

    Error Handling
    The script doesn't handle potential errors when making curl requests or processing JSON responses. Consider adding error handling to improve robustness.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add error handling for the rclone command to improve robustness and logging

    Consider adding error handling for the rclone command. Check the exit status of
    rclone and log any errors that occur during the upload process.

    Video/upload.sh [60]

    -rclone --config ${UPLOAD_CONFIG_DIRECTORY}/${UPLOAD_CONFIG_FILE_NAME} ${UPLOAD_COMMAND} ${UPLOAD_OPTS} "${source}" "${target}" &
    +if ! rclone --config ${UPLOAD_CONFIG_DIRECTORY}/${UPLOAD_CONFIG_FILE_NAME} ${UPLOAD_COMMAND} ${UPLOAD_OPTS} "${source}" "${target}"; then
    +  printf "%s [%s] - Error uploading %s to %s\n" "$(date -u +"${ts_format}")" "${process_name}" "${source}" "${target}" >&2
    +fi
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the rclone command significantly enhances the script's robustness by ensuring errors are logged, which is crucial for debugging and maintaining reliable uploads.

    8
    Use parameter expansion with double quotes to handle potential spaces in the timestamp format

    Use parameter expansion with a default value for SE_LOG_TIMESTAMP_FORMAT to ensure
    it always has a value, even if the variable is unset.

    Video/validate_endpoint.sh [6]

    -ts_format=${SE_LOG_TIMESTAMP_FORMAT:-"%Y-%m-%d %H:%M:%S,%3N"}
    +ts_format="${SE_LOG_TIMESTAMP_FORMAT:-%Y-%m-%d %H:%M:%S,%3N}"
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential issue with spaces in the timestamp format and proposes a solution that improves robustness by ensuring the format is always correctly interpreted.

    8
    Use a JSON parsing tool for more reliable extraction of values from the GraphQL response

    Consider using a more robust method to parse the JSON response from the GraphQL
    query. The current approach using grep and cut might break if the response format
    changes. Consider using a JSON parsing tool like jq for more reliable extraction of
    values.

    charts/selenium-grid/configs/distributor/distributorProbe.sh [26-28]

    -SESSION_QUEUE_SIZE=$(echo "${GRAPHQL_RESPONSE}" | grep -oP '(?<="sessionQueueSize":)\d+')
    -SESSION_COUNT=$(echo "${GRAPHQL_RESPONSE}" | grep -oP '(?<="sessionCount":)\d+')
    -MAX_SESSION=$(echo "${GRAPHQL_RESPONSE}" | grep -oP '(?<="maxSession":)\d+')
    +SESSION_QUEUE_SIZE=$(echo "${GRAPHQL_RESPONSE}" | jq -r '.data.grid.sessionQueueSize')
    +SESSION_COUNT=$(echo "${GRAPHQL_RESPONSE}" | jq -r '.data.grid.sessionCount')
    +MAX_SESSION=$(echo "${GRAPHQL_RESPONSE}" | jq -r '.data.grid.maxSession')
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Switching to a JSON parsing tool like jq improves the reliability of data extraction from JSON responses, reducing the risk of errors if the response format changes, thus enhancing maintainability.

    7
    Enhance error logging with more detailed information when the GraphQL endpoint is unreachable

    Consider adding more detailed error logging when the GraphQL endpoint is not
    reachable. Include the specific error message or response body to aid in
    troubleshooting.

    charts/selenium-grid/configs/router/routerProbe.sh [20]

    -echo "$(date -u +"${ts_format}") DEBUG [${probe_name}] - GraphQL endpoint is not reachable. Status code: ${GRAPHQL_PRE_CHECK}."
    +error_message=$(curl --noproxy "*" -m ${max_time} -k -X POST -H "Authorization: Basic ${BASIC_AUTH}" -H "Content-Type: application/json" --data '{"query":"{ grid { sessionCount } }"}' -s ${GRID_GRAPHQL_URL})
    +echo "$(date -u +"${ts_format}") DEBUG [${probe_name}] - GraphQL endpoint is not reachable. Status code: ${GRAPHQL_PRE_CHECK}. Error: ${error_message}"
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Providing more detailed error logging when the GraphQL endpoint is unreachable aids in troubleshooting by offering additional context, which is valuable for diagnosing connectivity issues.

    7
    Best practice
    Replace echo with printf for improved portability and consistency in output formatting

    Consider using printf instead of echo for more consistent behavior across different
    environments. The -e flag in echo is not portable and might not work as expected in
    all shells.

    Video/validate_endpoint.sh [22]

    -echo "$(date -u +"${ts_format}") [${process_name}] - Endpoint ${endpoint} is not found - status code: ${endpoint_checks}"
    +printf "%s [%s] - Endpoint %s is not found - status code: %s\n" "$(date -u +"${ts_format}")" "${process_name}" "${endpoint}" "${endpoint_checks}"
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use printf instead of echo enhances portability and consistency across different environments, addressing potential issues with echo's behavior in various shells.

    7
    Replace echo with printf for more consistent and portable output formatting

    Consider using the printf command instead of echo for more consistent output across
    different systems. The printf command provides better control over formatting and is
    more portable.

    Video/video.sh [36]

    -echo "$(date -u +"${ts_format}") [${process_name}] - Video folder exists: ${VIDEO_FOLDER}"
    +printf "%s [%s] - Video folder exists: %s\n" "$(date -u +"${ts_format}")" "${process_name}" "${VIDEO_FOLDER}"
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using printf instead of echo can improve the consistency and portability of output formatting, but the impact on functionality is minimal since echo is commonly used and generally reliable in this context.

    5
    Maintainability
    Use YAML anchors to define Kubernetes versions once and reference them throughout the configuration

    Consider using a variable or anchor for the Kubernetes versions to make it easier to
    update all occurrences at once in the future.

    .circleci/config.yml [40-60]

    -k8s-version: 'v1.28.15'
    +k8s_versions:
    +  &k8s_v1_28 'v1.28.15'
    +  &k8s_v1_29 'v1.29.10'
    +  &k8s_v1_30 'v1.30.6'
     ...
    -k8s-version: 'v1.29.10'
    +k8s-version: *k8s_v1_28
     ...
    -k8s-version: 'v1.30.6'
    +k8s-version: *k8s_v1_29
    +...
    +k8s-version: *k8s_v1_30
    Suggestion importance[1-10]: 6

    Why: Using YAML anchors for Kubernetes versions can improve maintainability by centralizing version updates, although it requires understanding YAML anchors, which might not be familiar to all developers.

    6

    💡 Need additional feedback ? start a PR chat

    Copy link

    codiumai-pr-agent-pro bot commented Oct 23, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit 323c05a)

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed because the GitHub CLI authentication process encountered an error:

  • The token used for authentication is missing the required scope read:org.
  • This missing scope is necessary for the gh auth login command to validate the token successfully.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    28:  SecurityEvents: write
    29:  Statuses: write
    30:  ##[endgroup]
    31:  Secret source: Actions
    32:  Prepare workflow directory
    33:  Prepare all required actions
    34:  Getting action download info
    35:  Download action repository 'actions/checkout@main' (SHA:163217dfcd28294438ea1c1c149cfaf66eec283e)
    36:  Complete job name: Rerun workflow when failure
    ...
    
    48:  show-progress: true
    49:  lfs: false
    50:  submodules: false
    51:  set-safe-directory: true
    52:  env:
    53:  GH_CLI_TOKEN: ***
    54:  GH_CLI_TOKEN_PR: ***
    55:  RUN_ID: 11479176724
    56:  RERUN_FAILED_ONLY: true
    ...
    
    119:  ##[group]Run sudo apt update
    120:  �[36;1msudo apt update�[0m
    121:  �[36;1msudo apt install gh�[0m
    122:  shell: /usr/bin/bash -e {0}
    123:  env:
    124:  GH_CLI_TOKEN: ***
    125:  GH_CLI_TOKEN_PR: ***
    126:  RUN_ID: 11479176724
    127:  RERUN_FAILED_ONLY: true
    ...
    
    168:  0 upgraded, 0 newly installed, 0 to remove and 26 not upgraded.
    169:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    170:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    171:  shell: /usr/bin/bash -e {0}
    172:  env:
    173:  GH_CLI_TOKEN: ***
    174:  GH_CLI_TOKEN_PR: ***
    175:  RUN_ID: 11479176724
    176:  RERUN_FAILED_ONLY: true
    177:  RUN_ATTEMPT: 1
    178:  ##[endgroup]
    179:  error validating token: missing required scope 'read:org'
    180:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @VietND96 VietND96 merged commit 42a7ecd into trunk Oct 23, 2024
    50 of 53 checks passed
    @VietND96 VietND96 deleted the log-timestamp branch October 23, 2024 14:21
    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