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

Add RoadRunner integration #3

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

Conversation

richardwang1124
Copy link

@richardwang1124 richardwang1124 commented Mar 5, 2025

Issue #, if available:

Description of changes:
Generates a RoadRunner compatible results.json for performance testing.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

Good start. My comments are assuming we still want to convert old to new. We should consider just modifying our benchmark report to be the new format instead. We can consider using this transformation logic to backfill historical data.

benchmark.rake Outdated
require 'json'
require_relative 'roadrunner'

if File.exist?('benchmark_report.json')
Copy link
Contributor

Choose a reason for hiding this comment

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

Guard clauses make this more readable. If the file doesn't exist, print and exit, then the rest of the logic that follows does not need to be inside an if statement.

Copy link

@jterapin jterapin left a comment

Choose a reason for hiding this comment

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

Nice! Agree with Matt on seeing if we can streamline the benchmark tools. Good opportunity to clean up our dashboard too.

Also - there's some rubocop failures - run rubocop locally to see what they are. Also rubocop -A to autocorrect if it makes sense to do so.

@richardwang1124 richardwang1124 changed the title Add conversion task for RoadRunner integration Add RoadRunner integration Mar 10, 2025
Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

Huge improvement! Nice work!

benchmark/gem.rb Outdated
module Benchmark
# Abstract base class for benchmarking an SDK Gem.
# Implementors must define the `gem_name`, `client_klass`, and the
# `operation_benchmarks` methods.
# rubocop:disable Metrics/ClassLength
Copy link
Contributor

Choose a reason for hiding this comment

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

For class length, the rubocop default is pretty low IMO. I'd just change it to 200 globally and you won't need this one.

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