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

Create summary.json of results from K6 run #8

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

waleedqk
Copy link

@waleedqk waleedqk commented May 2, 2022

When a K6 perf test is being run, individual files are created that hold the result for each test that is part of the submitted set.
The scrapper script consolidates the results from each of the output files and joins them into a single summary.json file.

@kserve-oss-bot kserve-oss-bot requested review from aluu317 and Tomcli May 2, 2022 12:22
@waleedqk waleedqk marked this pull request as draft May 2, 2022 12:24
@tedhtchang tedhtchang marked this pull request as ready for review May 11, 2022 14:06
Signed-off-by: waleedqk <[email protected]>

On branch wqk_prom_scrape
Changes to be committed:
	modified:   perf_test/runHowitzer.sh
	new file:   perf_test/scripts/scraper.py
@@ -66,8 +66,9 @@ for TEST in render/*.js; do
emphasize "Intermediate test results for: ${TEST}"
cat $RESULT_FILE

# emphasize "Generating intermediate Markdown for K6 tests..."
# python3 -m src.k6.scraper -r results -s summary -p persistent-results -c $CONFIG_FILE -k configs/kingdom.dict
emphasize "Generating intermediate Markdown for K6 tests..."
Copy link
Contributor

Choose a reason for hiding this comment

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

There's another python3 -m perf_test.scripts.scraper -r results -s summary -c $CONFIG_FILE down below that basically does the same command. Is there a difference here? Maybe we only need one?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, so testing it out locally and understanding the code as well, I think we can just remove the invocation in the loop and just running it at the end will suffice
Testing it on the perf cluster to confirm no change in behaviour

@waleedqk waleedqk changed the title First draft for adding scraper to create summary.json of results Create summary.json of results from K6 run May 24, 2022
# emphasize "K6 TESTS ARE COMPLETE. Use the following command to copy to your CWD."
# echo "kubectl cp ${POD_NAME}:output.md ./output.md && kubectl cp ${POD_NAME}:summary.json ./summary.json"

# Add a sleep at the end of the job to give time to exec into pods if need be
sleep 120
Copy link
Contributor

Choose a reason for hiding this comment

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

do you still want to keep this sleep here?

@@ -16,7 +16,8 @@ The core design principle beneath Howitzer lies in "exploding" K6 parameter iter
When Howitzer is run, the bash script will create directories `render`, `results` and `summary`:
- `render` stores the "exploded" k6 tests using the k6 template specified in the test config for each k6_opts combination.
- Then the script will run each individual k6 test in this `render` directory, storing the human readable results as text in `results` directory.
- It will also store the corresponding json file of the result into `summary` directory that we can later use for MLFlow.
- It will also store the corresponding json file of the result into `summary` directory.
- The scraper script then consolidates the results from the K6 test, the input test conditions etc. into a single `summary.json` file for the tests that were run.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think since now we have multiple scripts, let's be explicit and say the perf_test.scripts.scraper script, instead of just scraper script.

@@ -16,7 +16,8 @@ The core design principle beneath Howitzer lies in "exploding" K6 parameter iter
When Howitzer is run, the bash script will create directories `render`, `results` and `summary`:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change this to say

the bash script `perf_test/runHowitzer.sh`

so we can be specific

Copy link
Contributor

@aluu317 aluu317 left a comment

Choose a reason for hiding this comment

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

This looks good! We should also add the unit test for scraper.py in the unit_test folder just to be complete

@aluu317
Copy link
Contributor

aluu317 commented Jun 9, 2022

/lgtm

@aluu317
Copy link
Contributor

aluu317 commented Jun 9, 2022

We need to fix the issue with github not being able to install python in the github action.

@waleedqk Please also change the .github/workflows file to run python3 -m unittest discover unit_test/k6 (last line) so the test includes the scraper unit test

@kserve-oss-bot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: waleedqk
To complete the pull request process, please ask for approval from aluu317 after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@aluu317
Copy link
Contributor

aluu317 commented Jun 27, 2022

@waleedqk Can you do me a favor and try with version 3.9.13 or 3.10.5 on line https://github.com/kserve/modelmesh-performance/blob/main/.github/workflows/unit-test.yml#L21 . That's the error I see from the test run:
Screen Shot 2022-06-27 at 4 21 26 PM

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