Skip to content

Add benchmarks for rally runner components#2042

Open
gareth-ellis wants to merge 5 commits into
elastic:masterfrom
gareth-ellis:runner-benchmarks
Open

Add benchmarks for rally runner components#2042
gareth-ellis wants to merge 5 commits into
elastic:masterfrom
gareth-ellis:runner-benchmarks

Conversation

@gareth-ellis

Copy link
Copy Markdown
Member

Currently we don't track the impact of our changes to rally, risking making a change that could impact performance of rally that could then be attributed to an Elasticsearch change.

The aim of this PR is to add a github action that will run (we can perhaps change this so it runs optionally via a label?). and compare the performance of the current branch to that of master.

We have some benchmarks that existed already, that I have included, and also added some for tracking BulkIndex performance.

@gareth-ellis gareth-ellis marked this pull request as ready for review February 23, 2026 09:18
@gareth-ellis gareth-ellis requested a review from a team February 23, 2026 09:18
Comment on lines +54 to +59
- name: Compare results
id: compare
working-directory: ${{ github.workspace }}
run: |
python3 - <<'PYEOF'
import json, os, sys

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.

In rally tracks backporting we went for a script stored separately. Would it make sense to do the same here? Editing Python code inside YAML is not a great experience.

only_base = sorted(set(base_benchmarks) - set(pr_benchmarks))
only_pr = sorted(set(pr_benchmarks) - set(base_benchmarks))

THRESHOLD = 5 # percent

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.

This feels arbitrary. I would make it clear in the comment if that's the case.

@gbanasiak gbanasiak 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.

LGTM, great idea. I've left some non-blocking comments.

I'm assuming it will be tested against #1868.

Comment on lines +169 to +170
- name: Post or update PR comment
if: always() && !cancelled() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && (steps.compare.outcome == 'success' || steps.compare.outcome == 'failure')

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.

Can we avoid the repeat and do the comment in one workflow only?

Comment on lines +129 to +130
def return_raw_response(self):
pass

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.

Nit: Avoidable with inheritance.

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.

2 participants