test: output JSON for VHD build benchmarks and publish as pipeline artifact #4521
Conversation
zachary-bailey
requested review from
juan-lee,
cameronmeissner,
UtheMan,
ganeshkumarashok,
anujmaheshwari1,
AlisonB319,
Devinwong,
lilypan26,
ShiqianTao,
AbelHu,
junjiezhang1997 and
jason1028kr
as code owners
zachary-bailey
force-pushed
the
zacharybailey/jsonbenchmarks
branch
from
95a0c5f
to
d6f9f68
Compare
Pull Request Test Coverage Report for Build 9520897360Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9520911031Details
💛 - Coveralls |
|
|
||
| local current_time=$(date +%s) | ||
| local end_timestamp=$(date +%H:%M:%S) | ||
| local difference_in_seconds=$((current_time - ${1})) | ||
| if [ "$is_final_section" = true ]; then |
| } | ||
|
|
||
| stop_watch () { | ||
| capture_benchmarks () { |
There was a problem hiding this comment.
This function has 2 purposes, namely, capture benchmarks for a section and for the whole script.
If I were implementing it, I would separate this function into two, e.g., capture_section_benchmarks and capture_script_benchmarks (or capture_final_section_benchmarks if it's more appropriate), instead of using a bool to control the logic.
The reasons behind that are
- Single Responsibility Principle: The idea behind the SRP is that every class, module, or function in a program should have one responsibility/purpose in a program (copied from search engine)
- There isn't too much common logic between when $is_final_section == true vs false. So duplicate codes shouldn't be an issue here.
- Even there were duplicate codes, I would put them into functions so that
capture_section_benchmarksandcapture_script_benchmarkscan reuse.
Just my 2 cents :)
There was a problem hiding this comment.
Thank you for the review! I will look into refactoring the function with this in mind.
| script_object=$(jq -n --arg script_name "$title" --arg script_start_timestamp "$script_start_timestamp" --arg end_timestamp "$end_timestamp" --arg total_time_elapsed "$total_time_elapsed" '{($script_name): {"overall": {"start_time": $script_start_timestamp, "end_time": $end_timestamp, "total_time_elapsed": $total_time_elapsed}}}') | ||
|
|
||
| #iterate over the benchmarks array in order to retrieve data from previous sections and create section objects for each | ||
| for ((i=0; i<${#benchmarks[@]}; i+=4)); do |
There was a problem hiding this comment.
nit: consider using 4 arrays to store the information, namely, section_name, section_start_timestamp, end_timestamp, total_time_elapsed if there is no obvious benefit to use a single array. Just for easier debugging consideration. (Struct is not supported in bash script, otherwise it would be even better)
| echo "$output" | ||
| else | ||
| if [[ $OS == $MARINER_OS_NAME ]]; then | ||
| sudo tdnf install -y jq |
There was a problem hiding this comment.
should we capture the error in case it fails to install?
For example, in install_dependencies.sh it is doing something like this apt_get_install 30 1 600 grub-efi || exit 1
| sudo tdnf install -y jq | ||
| echo "jq was installed: $(jq --version)" | ||
| else | ||
| apt_get_install 5 1 60 jq |
There was a problem hiding this comment.
Same as above. Capture error?
| } | ||
|
|
||
| stop_watch () { | ||
| capture_benchmarks () { |
There was a problem hiding this comment.
I just added a shell script unit test and merged to master branch. Please git pull master branch and take a look at the section shell scripts unit tests in the readme. It will be great if you can add unit tests to your new functions. Examples are listed in the readme too.
zachary-bailey
force-pushed
the
zacharybailey/jsonbenchmarks
branch
from
d6f9f68
to
5d937bb
Compare
Pull Request Test Coverage Report for Build 9586871333Details
💛 - Coveralls |
zachary-bailey
force-pushed
the
zacharybailey/jsonbenchmarks
branch
from
530a56c
to
b475462
Compare
What type of PR is this?
/test
What this PR does / why we need it:
This PR changes the workflow for benchmarking the VHD build process and publishes a build artifact at the conclusion of the pipeline. Changes include removing the start_watch function, adding the installJq function, and outputting benchmarking data in JSON instead of formatted strings. At the end of each script, a JSON object will be output in the terminal with performance data for each section.
The installJq function was necessary because jq is not installed until the installDeps function is called in install-dependencies.sh, but it is needed when the build process starts in pre-install-dependencies.sh.
The pipeline artifact that will be published is a JSON file that will contain benchmarking data for the build process. In later changes, github actions will be used to evaluate this artifact and check for build time regression.
The formatting of the artifact can be checked by going to the PR gate build and checking the artifacts list.
Which issue(s) this PR fixes:
This will prevent vhd build times from incrementally increasing due to code changes.
Note: When reviewing "Files changed", please collapse /pkg/agent/testdata to see all relevant file changes
Requirements: