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

ci: add gomu gomu no gatling reports #1501

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

Conversation

isavov
Copy link
Contributor

@isavov isavov commented Mar 7, 2024

Pull Request type

  • Build-related changes

What is the current behavior?

Currently gomu gomu benchmark is run, but the results are just output to console.

Resolves: #NA

What is the new behavior?

Benchmark results are processed and compared with previous runs. Regression in performance of over 20% causes the pipeline to fail and any failed requests to madara during the benchmark cause the pipeline to fail too.

Does this introduce a breaking change?

No

Other information

@tdelabro
Copy link
Collaborator

tdelabro commented May 6, 2024

@apoorvsadana what is our current situation regarding gomugomu? Is this PR still relevant?

@apoorvsadana
Copy link
Collaborator

Ya it's relevant. The last PR created by Limechain on gomu gomu has to be merged. There was a bug before rebase in the PR which is solved now. So need to review it now, once we merge that, we can re run this and ensure everything works fine and finally merge

Copy link
Collaborator

@tdelabro tdelabro left a comment

Choose a reason for hiding this comment

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

I don't have the skills required to review this tbh

Comment on lines 57 to 80
- name: Process results
run: |
set -v -x +e
cp ../gomu-gomu-no-gatling/report.json ./
#### Extract TPS, UOPS, Extrinsics and Steps from the report ####
jq '
.benches[] |= (.name as $benchName | .metrics[] |= (.name = .name + " ("+ $benchName + ")")) # Append benchmark name to each metric name
| .benches[].metrics[].extra = .extra # set extra field for each metric
|.benches # extract only benches
|map(.metrics)|add # merge metrics from different benches
|map(select(.name | test("Average TPS|Average UOPS|Average Extrinsics|Average Steps"))) #filter only the metrics we want
' report.json > processed_report.json
- name: Check for failed requests
run: |
set -v -x +e
#### Extract Failed from the report ####
jq '
.benches[] |= (.name as $benchName | .metrics[] |= (.name = .name + " ("+ $benchName + ")")) # Append benchmark name to each metric name
| .benches[].metrics[].extra = .extra # set extra field for each metric
|.benches # extract only benches
|map(.metrics)|add # merge metrics from different benches
|map(select(.name | test("Failed"))) # filter only the metrics we want
|any(.[]; .value > 0) | if . then halt_error(1) else {} end # fail if there are failed requests
' report.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no idea what I'm reading.

Is it some standard thing you found online, or do you write this yourself all custom?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no problems here, it's just jq processing. I didn't tested them, so don't know if it's the desired output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I had to write it custom, since the benchmark processing action doesn't support our output format. I've commented it extensively in the hopes of making it a bit more readable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add a gh-compatible output format to gomu-gomu?

@tdelabro tdelabro requested a review from d-roak May 6, 2024 08:07
@tdelabro
Copy link
Collaborator

tdelabro commented May 6, 2024

maybe @d-roak and @apoorvsadana will do a better job reviewing this than me

@tdelabro tdelabro requested a review from apoorvsadana May 6, 2024 08:07
.github/workflows/gomu-gomu-tests.yml Outdated Show resolved Hide resolved
Comment on lines 57 to 80
- name: Process results
run: |
set -v -x +e
cp ../gomu-gomu-no-gatling/report.json ./
#### Extract TPS, UOPS, Extrinsics and Steps from the report ####
jq '
.benches[] |= (.name as $benchName | .metrics[] |= (.name = .name + " ("+ $benchName + ")")) # Append benchmark name to each metric name
| .benches[].metrics[].extra = .extra # set extra field for each metric
|.benches # extract only benches
|map(.metrics)|add # merge metrics from different benches
|map(select(.name | test("Average TPS|Average UOPS|Average Extrinsics|Average Steps"))) #filter only the metrics we want
' report.json > processed_report.json
- name: Check for failed requests
run: |
set -v -x +e
#### Extract Failed from the report ####
jq '
.benches[] |= (.name as $benchName | .metrics[] |= (.name = .name + " ("+ $benchName + ")")) # Append benchmark name to each metric name
| .benches[].metrics[].extra = .extra # set extra field for each metric
|.benches # extract only benches
|map(.metrics)|add # merge metrics from different benches
|map(select(.name | test("Failed"))) # filter only the metrics we want
|any(.[]; .value > 0) | if . then halt_error(1) else {} end # fail if there are failed requests
' report.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

no problems here, it's just jq processing. I didn't tested them, so don't know if it's the desired output

.github/workflows/gomu-gomu-tests.yml Outdated Show resolved Hide resolved
.github/workflows/gomu-gomu-tests.yml Outdated Show resolved Hide resolved
@tdelabro
Copy link
Collaborator

@tdelabro
Copy link
Collaborator

@isavov still red 🥲

isavov added 18 commits June 3, 2024 15:19
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
@tdelabro
Copy link
Collaborator

tdelabro commented Jun 5, 2024

@isavov the gomu run fails

@isavov
Copy link
Contributor Author

isavov commented Jun 5, 2024

@isavov the gomu run fails

Yes, it appears the current version of gomu-gomu is not compatible with madara's current version. Will try to debug further in the next week.

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.

None yet

4 participants