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

REQUEST: Repository maintenance on opentelemetry-rust #2616

Open
scottgerring opened this issue Mar 14, 2025 · 7 comments
Open

REQUEST: Repository maintenance on opentelemetry-rust #2616

scottgerring opened this issue Mar 14, 2025 · 7 comments
Assignees
Labels
area/repo-maintenance Maintenance of repos in the open-telemetry org

Comments

@scottgerring
Copy link

Affected Repository

https://github.com/open-telemetry/opentelemetry-rust

Requested changes

Please grant access of https://github.com/organizations/open-telemetry/settings/actions/runner-groups/8 (Benchmark Bare Metal Runners) to this repository, as was done for opentelemetry-go in #2266. We're running our benchmarks in the regular pools and the results are slow and variable.

Related: open-telemetry/opentelemetry-rust#2706

Purpose

Enable bare metal runners to run benchmarks.

Runner group name: 'Benchmark Bare Metal Runners'

Expected Duration

permanently

Repository Maintainers

* [@open-telemetry/rust-maintainers](https://github.com/orgs/open-telemetry/teams/rust-maintainers)

Adding @cijothomas for visibility

@trask trask self-assigned this Mar 14, 2025
@trask
Copy link
Member

trask commented Mar 14, 2025

done! please check and close the issue once you've confirmed it's working, thanks

@svrnm svrnm added the area/repo-maintenance Maintenance of repos in the open-telemetry org label Mar 17, 2025
@scottgerring
Copy link
Author

@trask thanks! I'll test it out in the next couple days and loop back to you.

@scottgerring
Copy link
Author

Hey @trask , our job doesn't seem to be picked up by the runners. I'm not sure if there's some other configuration we need - I think I saw something, somewhere, about whitelisting particular workflows?

Here's a blocked run and here's the config - but really I've just lifted the runs-on from opentelemetry-go:

jobs:
  runBenchmark:
    name: run benchmark
    permissions:
      pull-requests: write
    runs-on: self-hosted

@trask
Copy link
Member

trask commented Mar 17, 2025

I think I saw something, somewhere, about whitelisting particular workflows?

oh, you're right!

here's the current allowed workflows:

open-telemetry/opentelemetry-collector-contrib/.github/workflows/load-tests.yml@refs/heads/main,
open-telemetry/opentelemetry-go/.github/workflows/benchmark.yml@refs/heads/main,
open-telemetry/opentelemetry-java/.github/workflows/benchmark.yml@refs/heads/main,
open-telemetry/opentelemetry-java/.github/workflows/benchmark-tags.yml@refs/heads/main,
open-telemetry/opentelemetry-js/.github/workflows/benchmark.yml@refs/heads/main,
open-telemetry/opentelemetry-python/.github/workflows/benchmarks.yml@refs/heads/main

it looks like we're only running these on main in order to conserve the limited bare metal runners, can you do something similar in rust repo?

@scottgerring
Copy link
Author

We're new to this in Rust-land, so this might not be the best idea, but we've made it contingent on setting a label on the PR so it runs:

https://github.com/open-telemetry/opentelemetry-rust/blob/18e87185531c723dc76bd0dd404889535b22b10e/.github/workflows/pr_criterion.yaml#L9 - this way we can look at a particular PR and say "what's the impact of this thing we're suspicious of on performance", through the label on, and opt into the benchmarks:

    if: ${{ contains(github.event.pull_request.labels.*.name, 'performance') }}

Do you think we could get away with a wildcard to support this, or are we going a bit off the rails?

@trask
Copy link
Member

trask commented Mar 17, 2025

hm, it looks like the entries in that list "must be pinned to a ref, tag, or full SHA".

so I could add

open-telemetry/opentelemetry-rust/.github/workflows/pr_criterion.yaml@refs/pull/2813/merge

but not something that matches any PR ref branch

I'm guessing the concern with removing this restriction is that any PR could add a workflow that uses the bare metal runner

cc @jpkrohling @open-telemetry/technical-committee as admin for the bare metal runners in case they have some background on this

@scottgerring
Copy link
Author

scottgerring commented Mar 18, 2025

Hey @trask thanks for the info!

I'll leave it using shared workers for our opt-in PR perf regressions then, and setup the job to do HEAD vs HEAD~1 perf diff on main using this pool for a "higher quality comparison".

I figure this way we get the benefit of the bare metal runner for high-fidelity testing on main and follow the same pattern as the other projects.

What do you think?

I'll see if I can structure this within the same workflow file nicely, or not, and ping back on this thread with the full reference to add to the entries list 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/repo-maintenance Maintenance of repos in the open-telemetry org
Projects
Status: No status
Development

No branches or pull requests

3 participants