Skip to content

Conversation

ausias-armesto
Copy link
Contributor

This pipeline fixes the benchmarks and publishes the results in https://bencher.dev/console/projects/hoprnet/plots

Copy link
Contributor

coderabbitai bot commented Apr 10, 2025

Warning

Rate limit exceeded

@ausias-armesto has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 20 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 528d177 and d2c0589.

📒 Files selected for processing (2)
  • .github/workflows/bencher.yaml (2 hunks)
  • crypto/packet/benches/packet_benches.rs (6 hunks)
📝 Walkthrough

Walkthrough

This set of changes introduces a comprehensive overhaul of the project's benchmarking infrastructure. The GitHub Actions workflow for benchmarks is renamed and enhanced with Nix and Cachix setup steps. Benchmark execution is refactored to use environment variables and Nix shell invocation, with updated threshold parameters. The Nix flake and package definitions are extended to support a benchmarking build variant. Several benchmarking Rust source files are updated to standardize benchmark IDs and naming conventions, primarily replacing spaces with underscores. Additional dependencies and tools, such as bytesize and gnuplot, are added to support benchmarking and result visualization. A new nightly benchmark workflow is added to run scheduled benchmarks on multiple branches.

Changes

File(s) Change Summary
.github/workflows/bencher.yaml Workflow renamed to "Benchmarks"; adds Nix installation and Cachix authentication steps; refactors benchmark execution to use environment variables and run via nix develop -c bencher run; changes threshold parameters (measure from latency to throughput, adjusts upper and adds lower boundaries); removes some CLI flags; sets environment variables GITHUB_ACTIONS and BENCHER_ADAPTER; adds dynamic BENCHER_TESTBED environment variable based on event type and branch; increases job timeout to 150 minutes.
.github/workflows/bencher-nightly.yaml New GitHub Actions workflow added to run nightly benchmarks on master and release/singapore branches with concurrency control and dependency between jobs; reuses bencher.yaml workflow with appropriate inputs.
.github/workflows/merge.yaml Updated invocation of bencher.yaml reusable workflow to include new required input type with value merge.
flake.nix Adds new hoprd-bench package for benchmarking by extending Rust package build with runBench = true; includes it in exported packages and platform-specific inherit lists.
nix/cargo-bench.nix New Nix expression defining a function that creates Cargo benchmark derivations running cargo bench --locked; appends -bench suffix to package name; supports optional native build inputs.
nix/rust-package.nix Adds optional runBench parameter (default false); defines mkBench builder using cargo-bench.nix; conditionally uses mkBench as builder when runBench is true, enabling benchmarking builds.
nix/shell.nix Adds gnuplot package to the development shell environment.
common/network-types/Cargo.toml Adds bytesize crate as a workspace dependency.
common/network-types/benches/session.rs Modifies benchmark ID generation to convert byte sizes into human-readable strings with underscores replacing spaces, improving readability and consistency.
transport/mixer/benches/mixer_throughput.rs Standardizes benchmark ID and description formatting by replacing spaces with underscores; converts byte sizes to underscore-separated strings for benchmark IDs.
transport/protocol/benches/protocol_throughput_emulated.rs Updates benchmark ID formatting to use underscores and human-readable byte size strings without spaces; modifies public key cloning in peer mapping.
crypto/packet/benches/packet_benches.rs Replaces spaces with underscores in all hop count labels and benchmark IDs for consistency; renames benchmark functions accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant GitHub Actions
    participant Nix Installer
    participant Cachix Auth
    participant Nix Shell
    participant Bencher

    GitHub Actions->>Nix Installer: Install Nix via cachix/install-nix-action
    GitHub Actions->>Cachix Auth: Authenticate with Cachix via cachix/cachix-action
    GitHub Actions->>Nix Shell: Enter shell with nix develop
    Nix Shell->>Bencher: Run bencher with environment variables
    Bencher->>Bencher: Execute benchmarks and report results
Loading

Possibly related PRs

Suggested labels

crate:hoprd, crate:hoprd-keypair

Suggested reviewers

  • Teebor-Choka
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the toolchain Developer and product happiness label Apr 10, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
flake.nix (1)

532-554: Consider finalizing the commented-out benchmark app

The commented-out run-bench application contains environment setup that would be valuable for running benchmarks directly. Consider finalizing this implementation to provide a convenient way to execute benchmarks.

You could also add documentation about how to run benchmarks using the new hoprd-bench package until this app is finalized.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f06d060 and 9277b7b.

📒 Files selected for processing (5)
  • .github/workflows/bencher.yaml (3 hunks)
  • flake.nix (4 hunks)
  • nix/cargo-bench.nix (1 hunks)
  • nix/rust-package.nix (2 hunks)
  • nix/shell.nix (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: hoprd / docker
  • GitHub Check: hopli / docker
  • GitHub Check: Docs / Rust docs
🔇 Additional comments (13)
nix/shell.nix (1)

33-33: LGTM: Added gnuplot package to support data visualization

The addition of gnuplot to the development shell is a good choice to support visualization of benchmark results. This aligns well with the PR objective of publishing benchmarks.

nix/rust-package.nix (3)

23-23: Good addition to support benchmarking

Adding the optional runBench parameter follows the existing pattern for other build modes such as runTests and runClippy.


125-127: Well-structured benchmarking implementation

The implementation of the mkBench variable that imports from the new cargo-bench.nix file is clean and consistent with the project's architecture.


132-132: LGTM: Proper integration with existing build options

The additional condition in the builder section is correctly implemented to use the benchmark builder when runBench is true.

nix/cargo-bench.nix (1)

1-21: Excellent benchmark derivation implementation

This new file creates a specialized Cargo derivation for benchmarking, with:

  1. Proper inheritance of artifacts
  2. Appropriate naming with -bench suffix
  3. Correct build command for benchmarking with testing features

The implementation is aligned with the project's approach to other build types and effectively supports the benchmarking workflow.

flake.nix (3)

148-148: LGTM: New benchmark package definition

The addition of hoprd-bench package leverages the benchmarking capabilities added to the rust-package.nix file, enabling benchmark execution through the Nix build system.


677-677: Consistent with commented-out app implementation

This line properly comments out the bench app reference since the implementation is not yet active.


693-693: Well-integrated new package export

The hoprd-bench package is correctly added to the exported packages, making it available for users to build and run benchmarks.

.github/workflows/bencher.yaml (5)

5-5: Workflow Name Update: "Benchmarks"
The workflow name has been updated from "Bencher" to "Benchmarks" which clearly aligns with the PR objective of publishing benchmark results. Ensure that any downstream documentation or references to this workflow are updated accordingly.


54-58: Nix Installation Step Added
The new step to install Nix using the cachix/install-nix-action is correctly implemented, including the secure usage of the GitHub access token from secrets. The pinned commit hash provides stability; just confirm that it meets your update policy.


59-65: Cachix Authentication Step
The addition of the Cachix authentication step using cachix/cachix-action is properly configured with the token and sets the USER environment variable to "runner". This ensures that cached dependencies are correctly managed.


69-72: Addition of "Execute benchmarks" Step
The new step that executes benchmarks via the command nix develop -c cargo bench -F testing is in line with publishing the benchmark results. Ensure that the environment and dependencies in the development shell support this command and that the benchmarks produce results at the expected location.


73-90: Enhanced Benchmark Tracking Configuration
In the "Track base branch benchmarks with Bencher" step, the modifications include:

  • The addition of --dir target/criterion (line 82) to explicitly designate the directory containing benchmark outputs.
  • The inclusion of the --github-actions '${{ secrets.GITHUB_TOKEN }}' flag (line 84) which helps integrate Bencher with the GitHub Actions environment.

These changes should improve benchmark result tracking. Verify that the specified directory exists and contains the expected outputs, and that the Bencher tool correctly processes the GitHub token.

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Apr 13, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
.github/workflows/bencher-nightly.yaml (1)

3-3: Remove trailing whitespace
Line 3 ends with an extra space, which triggers YAMLlint’s trailing-spaces error. Please trim the trailing space after “results”.

🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 3-3: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a406c4 and d8b810b.

📒 Files selected for processing (3)
  • .github/workflows/bencher-nightly.yaml (1 hunks)
  • .github/workflows/bencher.yaml (3 hunks)
  • .github/workflows/merge.yaml (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/bencher-nightly.yaml

[error] 3-3: trailing spaces

(trailing-spaces)

⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Bencher
  • GitHub Check: hoprd / docker
  • GitHub Check: hopli / docker
  • GitHub Check: Docs / Rust docs
  • GitHub Check: tests-unit-nightly
  • GitHub Check: tests-unit
  • GitHub Check: tests-smoke-websocket
  • GitHub Check: zizmor
  • GitHub Check: Linter
🔇 Additional comments (11)
.github/workflows/merge.yaml (1)

294-295:

❓ Verification inconclusive

Ensure all callers supply the new type input
You’ve added the required type: merge parameter to the reusable bencher.yaml invocation. Please verify that every other workflow invoking bencher.yaml (e.g., manual dispatch, scheduled) passes a valid type (merge, scheduled, or manual) so the conditional logic in bencher.yaml doesn’t exit early.

Here’s a snippet you can run to list all invocations and inspect their inputs:


🏁 Script executed:

#!/bin/bash
rg -n "uses: \.\/\.github\/workflows\/bencher.yaml" -A2

Length of output: 175


#!/bin/bash
# List all workflow invocations of bencher.yaml and show 2 lines of context
rg -F "uses: ./.github/workflows/bencher.yaml" -n .github/workflows -A2

#!/bin/bash
# List all workflow invocations of bencher.yaml with surrounding context to check for 'type'
rg -F "uses: ./.github/workflows/bencher.yaml" -n .github/workflows -A5

Confirm all bencher.yaml calls include the new type input
I searched through .github/workflows and only found the reusable-workflow invocation in merge.yaml. Please double-check any other workflows (e.g. manual dispatch or scheduled runs) that call bencher.yaml and ensure they pass a valid type (merge, scheduled, or manual) so its conditional logic doesn’t exit early.

.github/workflows/bencher-nightly.yaml (2)

15-17: Concurrency scope is correct
Using group: ${{ github.ref }}-bencher-nightly effectively prevents overlapping nightly runs per branch. This matches the intended design.


20-28: Nightly workflow invocations look good
Both bencher-nightly-master and bencher-nightly-release-singapore reuse the updated bencher.yaml with type: scheduled. Confirm that release/singapore aligns with your branch naming policy or parameterize additional release branches as needed.

.github/workflows/bencher.yaml (8)

5-5: Workflow renamed for clarity
Renaming to Benchmarks accurately reflects its role as a reusable benchmarking pipeline.


11-19: Added required type input
You’ve extended the workflow_call block to include a mandatory type parameter. This is essential for distinguishing merge, scheduled, and manual contexts. Ensure all callers are updated accordingly.


29-30: Increased timeout for long‐running benchmarks
Benchmark runs can be lengthy; bumping to 150 minutes seems reasonable. Please confirm that this upper limit covers your worst-case runs without unnecessarily blocking runner capacity.


40-48: Sanitize branch name safely
The Set environment variables step guards against template injection and normalizes slashes to dashes. This is a solid approach for subsequent environment and Nix derivations.


53-57: Pinned checkout of target branch
Switching to actions/checkout@v4 and using ref: ${{ steps.vars.outputs.BRANCH_NAME }} is correct. Verify that the checkout still has access to any required submodules or credentials if needed.


59-63: Nix installation step is sound
Installing Nix via the Cachix action sets up your reproducible environment. Ensure the GitHub token used here has minimal privileges.


64-68: Authenticate with Cachix
Authenticating to Cachix with secrets.CACHIX_AUTH_TOKEN looks good. Confirm that the token scope is correct and doesn’t grant more access than necessary.


71-93: Dynamic testbed selection is robust
Your Set Bencher Testbed logic cleanly handles merge, scheduled, and manual triggers and skips other branches. This ensures benchmarks run only where intended.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
.github/workflows/bencher.yaml (2)

100-104: ⚠️ Potential issue

Avoid overriding GITHUB_ACTIONS env var: You export GITHUB_ACTIONS=${GITHUB_ACTIONS}, setting it to the secret token. Overwriting the built-in flag can break runner detection and leak credentials. Use GITHUB_TOKEN instead:

- export GITHUB_ACTIONS=${GITHUB_ACTIONS}
+ export GITHUB_TOKEN=${GITHUB_TOKEN}

114-118: ⚠️ Potential issue

Correct environment variable for GitHub token: In the step’s env block, replace GITHUB_ACTIONS: ${{ secrets.GITHUB_TOKEN }} with:

- GITHUB_ACTIONS: ${{ secrets.GITHUB_TOKEN }}
+ GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

This aligns with the export change above.

🧹 Nitpick comments (1)
.github/workflows/bencher.yaml (1)

46-49: Quote $GITHUB_OUTPUT for consistency: For robustness, consider quoting all expansions of $GITHUB_OUTPUT:

- echo "FORMATTED_BRANCH=$FORMATTED_BRANCH" >> $GITHUB_OUTPUT
+ echo "FORMATTED_BRANCH=$FORMATTED_BRANCH" >> "$GITHUB_OUTPUT"
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d8b810b and 363ae2e.

📒 Files selected for processing (2)
  • .github/workflows/bencher.yaml (2 hunks)
  • crypto/packet/benches/packet_benches.rs (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • crypto/packet/benches/packet_benches.rs
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/bencher.yaml

28-28: label "self-hosted-hoprnet-bigger" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

⏰ Context from checks skipped due to timeout of 90000ms (10)
  • GitHub Check: hoprd / docker
  • GitHub Check: hopli / docker
  • GitHub Check: Docs / Rust docs
  • GitHub Check: tests-smoke-websocket
  • GitHub Check: tests-unit-nightly
  • GitHub Check: tests-smart-contracts
  • GitHub Check: Linter
  • GitHub Check: zizmor
  • GitHub Check: tests-unit
  • GitHub Check: Cargo Audit
🔇 Additional comments (10)
.github/workflows/bencher.yaml (10)

5-5: Rename workflow for clarity: The workflow name has been updated to "Benchmarks" to better reflect its purpose.


11-18: Ensure callers supply the new type input: You’ve added a required type input for workflow_call. Verify that all invoking workflows (e.g., merge.yaml, bencher-nightly.yaml) pass this parameter correctly.


22-23: Review concurrency settings: Disabling cancel-in-progress will queue all runs under the "bencher" group. Ensure this aligns with your expectations, as it may lead to concurrent long-running jobs piling up.


29-29: Increase job timeout: The timeout-minutes has been extended to 150 to accommodate lengthy benchmarks. LGTM if your slowest benchmarks fit within this window.


40-45: Secure branch name extraction: Good use of INPUT_BRANCH fallback to avoid template injection via refs. This ensures safe branch detection across workflow_call and workflow_dispatch events.


51-53: Environment variable mapping: Mapping INPUT_BRANCH and GITHUB_REF_NAME into the script environment looks correct.


57-57: Checkout by sanitized ref: Using steps.vars.outputs.BRANCH_NAME ensures you checkout the intended branch.


60-64: Install Nix securely: Leveraging cachix/install-nix-action with a GitHub token is correct for multi-user environments.


65-71: Authenticate Cachix: The cachix/cachix-action configuration (with USER: runner) is valid for populating the Nix binary cache. Confirm that CACHIX_AUTH_TOKEN has appropriate scopes.


73-79: Validate type-based testbed logic: You derive IS_MERGE and IS_SCHEDULED from inputs and IS_MANUAL from dispatch. Confirm that no other event types slip through and that callers only pass merge or scheduled.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
.github/workflows/bencher.yaml (1)

102-108: ⚠️ Potential issue

Correct environment variable export for GitHub token
You’re currently exporting GITHUB_ACTIONS=${GITHUB_TOKEN}, which overrides a built-in flag and leads to confusing behavior. Update to:

-export GITHUB_ACTIONS=${GITHUB_TOKEN}
+export GITHUB_TOKEN=${GITHUB_TOKEN}
🧹 Nitpick comments (3)
.github/workflows/bencher-nightly.yaml (2)

3-3: Remove trailing whitespace in header comment
The comment line has trailing spaces that may trigger lint errors.

🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 3-3: trailing spaces

(trailing-spaces)


15-16: Use sanitized branch name for concurrency group
github.ref includes the full ref (e.g. refs/heads/master), which can fragment your concurrency keys. Consider using ${{ github.ref_name }} for a cleaner group:

-group: ${{ github.ref }}-bencher-nightly
+group: ${{ github.ref_name }}-bencher-nightly
.github/workflows/bencher.yaml (1)

11-19: Consider renaming the type input
Using type may conflict with YAML or be confusing. A more descriptive key like trigger_type or run_type would improve clarity.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 363ae2e and 048bbba.

📒 Files selected for processing (2)
  • .github/workflows/bencher-nightly.yaml (1 hunks)
  • .github/workflows/bencher.yaml (2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/bencher.yaml

28-28: label "self-hosted-hoprnet-bigger" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

🪛 YAMLlint (1.35.1)
.github/workflows/bencher-nightly.yaml

[error] 3-3: trailing spaces

(trailing-spaces)

⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: tests-smoke-websocket
  • GitHub Check: tests-unit
  • GitHub Check: tests-unit-nightly
  • GitHub Check: tests-smart-contracts
  • GitHub Check: hoprd / docker
  • GitHub Check: hopli / docker
  • GitHub Check: Docs / Rust docs
  • GitHub Check: Linter
  • GitHub Check: zizmor
🔇 Additional comments (13)
.github/workflows/bencher-nightly.yaml (2)

19-25: Master nightly job configuration looks good
Reusing the bencher.yaml workflow with branch: master and type: scheduled is correct, and secrets: inherit ensures credentials are passed.


26-33: Release Singapore nightly job configuration looks good
Chaining needs: bencher-nightly-master to avoid overload and reusing the workflow is a solid approach.

.github/workflows/bencher.yaml (11)

5-5: Workflow renamed appropriately
Updating the name to “Benchmarks” aligns with the multi-branch and nightly usage.


22-23: Concurrency settings simplified correctly
Group bencher with cancel-in-progress: false fits the new schedule—multiple runs won’t cancel each other.


29-29: Verify custom runner label
self-hosted-hoprnet-bigger isn’t in the default GitHub hosted runners. Confirm this label exists on your self-hosted pool or update your actionlint config to allow it.


30-30: Extended timeout is appropriate
Increasing timeout-minutes to 150 accommodates longer benchmark runs.


40-49: Environment extraction step is solid
Safely obtaining branch, formatting it, and capturing COMMIT_HASH avoids template injection and preserves downstream logic.


51-52: Input environment variables mapping is correct
Mapping ${{ inputs.branch }} and ${{ github.ref_name }} to shell variables ensures consistency.


57-57: Checkout ref matches configured branch
Using ref: ${{ steps.vars.outputs.BRANCH_NAME }} is the right approach to replay on the triggered branch.


60-64: Installing Nix with Cachix token is correct
Auth setup via secrets.GITHUB_TOKEN and subsequent Cachix steps follow best practices.


65-71: Cachix action usage looks good
Caching the Nix store ensures faster subsequent runs, and USER: runner is a valid workaround if needed.


72-95: Confirm exit codes in testbed setup
The branch filter exit 0 for unsupported branches is correct, but exit 1 in the “else” of allowed branches will fail the workflow on benign trigger types. If the goal is to skip benchmarks (not error), consider exit 0 instead.


115-118: Passed-in environment variables for benchmarks are correct
Mapping secrets and outputs to BENCHER_API_TOKEN, GITHUB_TOKEN, BRANCH_NAME, COMMIT_HASH, and BENCHER_TESTBED is consistent with earlier steps.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
.github/workflows/bencher.yaml (1)

98-109: ⚠️ Potential issue

Critical security fix: Do not override GITHUB_ACTIONS
On line 104 you’re exporting GITHUB_ACTIONS=${GITHUB_TOKEN}, which overrides the built-in flag and risks leaking the token. It should be:

-export GITHUB_ACTIONS=${GITHUB_TOKEN}
+export GITHUB_TOKEN=${GITHUB_TOKEN}

Please correct this immediately.

🧹 Nitpick comments (4)
.github/workflows/bencher.yaml (4)

35-35: Track TODO: Egress policy adjustment
The comment at line 35 indicates changing egress-policy from audit to block after a few runs. It’s best to schedule this as a tracked task (e.g., GitHub issue) or remove the TODO once complete to avoid overlooking it.


5-5: Inconsistent naming: Workflow vs. job
The workflow is named Benchmarks but the job block below uses name: Bencher. For clarity and consistency, consider renaming the job to Benchmarks (or a similarly aligned name).


22-23: Consider branch-specific concurrency
Current group: bencher will serialize all benchmark runs globally. If you want independent concurrency per branch, include branch context (e.g., ${{ inputs.branch }} or ${{ github.ref }}) in the group name.


71-93: Testbed selection logic
The logic to derive BENCHER_TESTBED across merge, scheduled, and manual runs on different branch patterns is comprehensive. Consider adding a guard (e.g., an explicit warning or exit 1) if inputs.type is outside the expected set to catch misconfigurations early.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 048bbba and 528d177.

📒 Files selected for processing (1)
  • .github/workflows/bencher.yaml (2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/bencher.yaml

28-28: label "self-hosted-hoprnet-bigger" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: tests-smoke-websocket
  • GitHub Check: tests-unit-nightly
  • GitHub Check: tests-unit
  • GitHub Check: hoprd / docker
  • GitHub Check: Linter
  • GitHub Check: zizmor
🔇 Additional comments (8)
.github/workflows/bencher.yaml (8)

11-18: Verify workflow inputs usage
You’ve added a required type input. Please confirm that all callers (e.g., .github/workflows/merge.yaml, .github/workflows/bencher-nightly.yaml) invoke this workflow with the correct type values (merge, scheduled, etc.).


29-29: Extended job timeout
The job timeout has been increased to 150 minutes. Ensure this aligns with your longest benchmark runs and isn’t set excessively high.


40-46: Branch name sanitization
Good use of conditional checks and sed to escape slashes and prevent template injection when forming branch-based outputs.


50-52: Explicit step environment
Passing INPUT_BRANCH and GITHUB_REF_NAME explicitly into the step ensures reproducible behavior. Nicely done.


56-57: Checkout branch by computed output
Using the BRANCH_NAME output from the vars step to drive actions/checkout ensures you pull the correct ref.


59-63: Secure Nix installation
Pinning cachix/install-nix-action to a commit SHA and securely passing secrets.GITHUB_TOKEN aligns with best practices.


64-70: Cachix action configuration
You’ve set USER: runner in the Cachix action’s environment. Please verify this is required for your self-hosted runner context and doesn’t override other important env vars.


111-116: Benchmark step environment
Explicitly mapping secrets and outputs into the environment for the benchmark run is correct and clear.

@ausias-armesto ausias-armesto merged commit 9aa0671 into master Apr 25, 2025
25 of 32 checks passed
@ausias-armesto ausias-armesto deleted the ausias/fix-benchmarks branch April 25, 2025 12:20
@coderabbitai coderabbitai bot mentioned this pull request Apr 25, 2025
@github-project-automation github-project-automation bot moved this to To triage in hoprnet Jul 4, 2025
@ausias-armesto ausias-armesto added this to the 3.0.0 milestone Jul 4, 2025
@tolbrino tolbrino removed this from hoprnet Jul 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate:hopr-crypto-packet crate:hopr-transport-protocol dependencies Pull requests that update a dependency file toolchain Developer and product happiness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The benchmarks performed on various pieces of the code should be stored and regularly analyzed in CI
2 participants