Skip to content

Push benchmark results to GitHub pages#193

Open
simonmarty wants to merge 6 commits intomainfrom
bench-baseline
Open

Push benchmark results to GitHub pages#193
simonmarty wants to merge 6 commits intomainfrom
bench-baseline

Conversation

@simonmarty
Copy link
Copy Markdown
Contributor

Description

Why is this change being made?

  1. Allows us to generate graphs of the agent's benchmarks.

What is changing?

  1. Added a separate workflow with write permissions that runs on push to create benchmark charts.
  2. Modified the workflow that runs on PRs so that it automatically fetches the latest bench baseline from GH pages.

We keep the two separate because they have different levels of permissions.

Related Links

  • Issue #, if available:

Testing

Tested on fork https://simonmarty.github.io/aws-secretsmanager-agent/dev/bench/
Made a mock PR https://github.com/simonmarty/aws-secretsmanager-agent/actions/runs/24591788832

How was this tested?

  1. Tested on fork https://simonmarty.github.io/aws-secretsmanager-agent/dev/bench/
  2. Made a mock PR https://github.com/simonmarty/aws-secretsmanager-agent/actions/runs/24591788832

When testing locally, provide testing artifact(s):


Reviewee Checklist

Update the checklist after submitting the PR

  • I have reviewed, tested and understand all changes
    If not, why:
  • I have filled out the Description and Testing sections above
    If not, why:
  • Build and Unit tests are passing
    If not, why:
  • Unit test coverage check is passing
    If not, why:
  • Integration tests pass locally
    If not, why:
  • I have updated integration tests (if needed)
    If not, why: No change
  • I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
    If not, why:
  • I have added explanatory comments for complex logic, new classes/methods and new tests
    If not, why:
  • I have updated README/documentation (if needed)
    If not, why: No change
  • I have clearly called out breaking changes (if any)
    If not, why: No breaking changes.

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

  • Reviewee checklist has been accurately filled out
  • Code changes align with stated purpose in description
  • Test coverage adequately validates the changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@simonmarty simonmarty requested a review from a team as a code owner April 18, 2026 00:09
@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Apr 18, 2026
@simonmarty simonmarty requested a review from reyhankoyun April 18, 2026 00:10
@github-actions github-actions Bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Apr 18, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.30%. Comparing base (4b1852d) to head (88237fd).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #193   +/-   ##
=======================================
  Coverage   92.30%   92.30%           
=======================================
  Files          15       15           
  Lines        2626     2626           
  Branches     2626     2626           
=======================================
  Hits         2424     2424           
  Misses        152      152           
  Partials       50       50           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Simon Marty <martysi@amazon.com>
@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Apr 18, 2026
@github-actions github-actions Bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Apr 18, 2026
Comment thread .github/workflows/bench_baseline.yml
Comment thread .github/workflows/bench_baseline.yml Outdated
Comment thread .github/workflows/bench_baseline.yml
@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Apr 22, 2026
@github-actions github-actions Bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Apr 22, 2026
reyhankoyun
reyhankoyun previously approved these changes Apr 22, 2026
@simonmarty simonmarty enabled auto-merge (squash) April 22, 2026 23:29
working-directory: aws_secretsmanager_caching
run: cargo bench --bench benchmark -- --output-format bencher | tee ../bench-result.txt

- name: Download previous benchmark data
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.

for my edification: why is this being removed? Does benchmark-action/github-action-benchmark@v1 do this for you?

Copy link
Copy Markdown
Contributor Author

@simonmarty simonmarty Apr 23, 2026

Choose a reason for hiding this comment

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

Sorry it took a minute to respond, I figured I could show you on my fork.

Here's a run from a push to main on the fork: https://github.com/simonmarty/aws-secretsmanager-agent/actions/runs/24808395936/job/72763846297

Here's a run against a no-op pull request I created on my fork, it correctly retrieves the test run results simonmarty#61 https://github.com/simonmarty/aws-secretsmanager-agent/actions/runs/24855062991/job/72765608319?pr=61

This does make me realize that there must be some variance in the performance of GitHub actions hosted runners. It's failing the regression check despite there not being any code changes. I'm going to set the workflow to be non-blocking and just post comments. Maybe up the tolerance threshold to the default of 200%.

Signed-off-by: Simon Marty <martysi@amazon.com>
Signed-off-by: Simon Marty <martysi@amazon.com>
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