Skip to content

Timestamps + Ansicolor based segregation - #518

Open
vipbhardwaj wants to merge 3 commits into
mainfrom
CBG-5787
Open

Timestamps + Ansicolor based segregation#518
vipbhardwaj wants to merge 3 commits into
mainfrom
CBG-5787

Conversation

@vipbhardwaj

@vipbhardwaj vipbhardwaj commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

CBG-5787

Jenkinsfile is modified now to use timestamps so as to print time-taken per stage, in the overall RUN STAGE of Jenkinsfile right now.
It is also using Ansicoloring so to just print stuff better since that is making it a lot easier travel across the logs via naked eye.

This is a first step segregation while changes related to CBG-5727 are on hold / being re-considered.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new EXIT trap in shared/config.sh overwrites existing EXIT traps in other pipeline scripts, breaking their cleanup/upload behavior unless the trap is chained.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR enhances Jenkins pipeline log readability and timing visibility for the QE/sgw job by enabling Jenkins-level timestamps and ANSI coloring, and by adding reusable “section” banners in the shared pipeline shell config.

Changes:

  • Add section_start / timed section banner support (with ANSI colors) to jenkins/pipelines/shared/config.sh.
  • Replace a couple of plain echo milestones in QE/sgw scripts with colored/timed section banners.
  • Enable ansiColor('xterm') and timestamps() in the QE/sgw Jenkins declarative pipeline.
File summaries
File Description
jenkins/pipelines/shared/config.sh Adds ANSI-colored section banner helpers and duration reporting for Jenkins console output.
jenkins/pipelines/QE/sgw/test.sh Uses section_start to mark and time “INFRA SETUP” and “RUN TESTS”.
jenkins/pipelines/QE/sgw/teardown.sh Uses section_start to mark and time “TEARDOWN”.
jenkins/pipelines/QE/sgw/Jenkinsfile Enables Jenkins ansiColor + timestamps options so banner coloring and timestamps render in console logs.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread jenkins/pipelines/shared/config.sh Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Jenkins pipeline/logging changes can impact CI behavior and should be validated in an actual Jenkins run environment before approval.

Review details

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

jenkins/pipelines/shared/config.sh:97

  • The color constants are stored as literal backslash sequences (e.g. "\033"); this forces callers to rely on echo -e to interpret escapes, which is less predictable across environments and can accidentally interpret backslash sequences from labels. Prefer storing actual ESC characters via Bash $'…' quoting so they print correctly with printf/echo without -e.
readonly COLOR_RESET='\033[0m'
readonly COLOR_CYAN='\033[1;36m'
readonly COLOR_YELLOW='\033[1;33m'
readonly COLOR_GREEN='\033[1;32m'

jenkins/pipelines/shared/config.sh:125

  • Using echo -e is shell-dependent and can mis-handle backslash sequences; printf is more deterministic for emitting the section banners (especially once the color variables contain real ESC characters).

This issue also appears on line 134 of the same file.

  echo -e "${_section_color}=== ${_section_label} END (took $(_format_duration $((end_ts - _section_start_ts)))) ===${COLOR_RESET}"

jenkins/pipelines/shared/config.sh:132

  • section_start assumes two positional args; with set -u (enabled in these scripts) missing args will fail with a generic "unbound variable" message. Adding explicit parameter checks gives a clearer failure mode for callers.
  _section_color="$1"
  _section_label="$2"

jenkins/pipelines/shared/config.sh:134

  • Same as above: prefer printf over echo -e for deterministic output.
  echo -e "${_section_color}=== ${_section_label} START ===${COLOR_RESET}"
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread jenkins/pipelines/shared/config.sh Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new EXIT-trap append logic in config.sh is brittle and can break existing traps due to unsafe parsing/re-installation of trap -p EXIT output.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

jenkins/pipelines/shared/config.sh:147

  • The EXIT-trap append logic parses trap -p EXIT output with awk -F"'" '{print $2}', which is brittle: it can mis-handle traps printed as $'...' (newlines/escapes) or traps containing embedded single quotes, and can silently change or drop the existing trap body when re-installing it. Consider extracting the raw trap command in a way that preserves Bash's quoting and only then appending _section_close.
_existing_exit_trap=$(trap -p EXIT | awk -F"'" '{print $2}')
if [ -n "$_existing_exit_trap" ]; then
  if [[ "$_existing_exit_trap" != *"_section_close"* ]]; then
    trap "${_existing_exit_trap}; _section_close" EXIT
  fi
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment on lines +89 to +93
# Colored, timed section banners for Jenkins console output (rendered by the
# ansiColor('xterm') pipeline option; the raw escape codes are harmless on a
# plain terminal too). Usage: section_start "$COLOR_CYAN" "INFRA SETUP" ...
# work... ; the next section_start call (or script exit, via the EXIT trap
# below) closes the previous section and prints how long it took.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc the explicit pairing instead of the implicit "next section_start closes the previous one" + EXIT trap.

Suggested change
# Colored, timed section banners for Jenkins console output (rendered by the
# ansiColor('xterm') pipeline option; the raw escape codes are harmless on a
# plain terminal too). Usage: section_start "$COLOR_CYAN" "INFRA SETUP" ...
# work... ; the next section_start call (or script exit, via the EXIT trap
# below) closes the previous section and prints how long it took.
# Colored, timed section banners for Jenkins console output (rendered by the
# ansiColor('xterm') pipeline option; the raw escape codes are harmless on a
# plain terminal too). Sections are explicitly paired:
# section_start "$COLOR_CYAN" "INFRA SETUP"
# ...work...
# section_end
# section_end prints how long the section took and clears the state, so a
# script that exits mid-section (a failed test, a Jenkins timeout) simply
# leaves that banner unprinted rather than needing an EXIT trap here.

Comment on lines +117 to +135
# Closes whichever section is currently open (no-op if none is). Registered
# as an EXIT trap so the closing banner -- and the duration -- still prints
# even if the section's work fails (set -e / the caller's ERR trap exits the
# script from inside the section), not just on a clean finish.
_section_close() {
[ -z "$_section_label" ] && return 0
local end_ts
end_ts=$(date +%s)
echo -e "${_section_color}=== ${_section_label} END (took $(_format_duration $((end_ts - _section_start_ts)))) ===${COLOR_RESET}"
_section_label=""
}

section_start() {
_section_close
_section_color="$1"
_section_label="$2"
_section_start_ts=$(date +%s)
echo -e "${_section_color}=== ${_section_label} START ===${COLOR_RESET}"
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promote _section_close to a public section_end that clears all three state vars, and drop the implicit close from section_start so there's one mechanism rather than two.

Suggested change
# Closes whichever section is currently open (no-op if none is). Registered
# as an EXIT trap so the closing banner -- and the duration -- still prints
# even if the section's work fails (set -e / the caller's ERR trap exits the
# script from inside the section), not just on a clean finish.
_section_close() {
[ -z "$_section_label" ] && return 0
local end_ts
end_ts=$(date +%s)
echo -e "${_section_color}=== ${_section_label} END (took $(_format_duration $((end_ts - _section_start_ts)))) ===${COLOR_RESET}"
_section_label=""
}
section_start() {
_section_close
_section_color="$1"
_section_label="$2"
_section_start_ts=$(date +%s)
echo -e "${_section_color}=== ${_section_label} START ===${COLOR_RESET}"
}
section_start() {
_section_color="$1"
_section_label="$2"
_section_start_ts=$(date +%s)
echo -e "${_section_color}=== ${_section_label} START ===${COLOR_RESET}"
}
# No-op if no section is open, so a stray call can't emit a bogus banner.
section_end() {
[ -z "$_section_label" ] && return 0
local end_ts
end_ts=$(date +%s)
echo -e "${_section_color}=== ${_section_label} END (took $(_format_duration $((end_ts - _section_start_ts)))) ===${COLOR_RESET}"
_section_color=""
_section_label=""
_section_start_ts=""
}

Comment on lines +136 to +147

# Append to any existing EXIT trap instead of overwriting it (some pipeline scripts
# install their own EXIT traps before sourcing this file).
_existing_exit_trap=$(trap -p EXIT | awk -F"'" '{print $2}')
if [ -n "$_existing_exit_trap" ]; then
if [[ "$_existing_exit_trap" != *"_section_close"* ]]; then
trap "${_existing_exit_trap}; _section_close" EXIT
fi
else
trap _section_close EXIT
fi
unset _existing_exit_trap

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop setting a trap and go for an explicit session_start / session_end. config.sh is sourced by many files. With an explicit section_end none of it is needed.

Suggested change
# Append to any existing EXIT trap instead of overwriting it (some pipeline scripts
# install their own EXIT traps before sourcing this file).
_existing_exit_trap=$(trap -p EXIT | awk -F"'" '{print $2}')
if [ -n "$_existing_exit_trap" ]; then
if [[ "$_existing_exit_trap" != *"_section_close"* ]]; then
trap "${_existing_exit_trap}; _section_close" EXIT
fi
else
trap _section_close EXIT
fi
unset _existing_exit_trap

SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd)
source $SCRIPT_DIR/../../shared/config.sh

section_start "$COLOR_GREEN" "TEARDOWN"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth keeping this one — teardown runs under timeout(time: 5, unit: 'MINUTES') and does the two things that blow that budget (move_artifacts gzipping a ~100MB session.log, then stop_backend.py destroying EC2), so its duration against that ceiling is the most useful number in the script.

It needs a matching close, which is outside the diff — add section_end as the last line of the file, after the popd on line 18:

uv run ./stop_backend.py --topology topology_setup/topology.json
popd
section_end

uv run pytest -v --no-header --config QE/config.json \
--ignore=dev_e2e/test_replication_xdcr.py \
--sgcollect-on-test-failure
fi

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fi
section_end

Comment on lines +10 to +13
options {
ansiColor('xterm')
timestamps()
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add these things to all Jenkinsfiles -

having ansiColor is mostly a speculative fix and it might pick up some terraform coloring. Typically most code is not going to use it but having it present just makes log files either.

That said, I wouldn't bother with shell script code but I think this is fine modulo my comment about trap code that I would remove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants