-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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') |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
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.