rubytune / perf_check Goto Github PK

PerfCheck currently communicates at the user in two ways: a Logger instance and exceptions. This implementation makes sense in the context of a terminal application with a command-line interface but doesn't work well when communicating through a CI or desktop application.

There are a number of different types of information that PerfCheck needs to output:

1. Status of the run (started, finished, failed, broken, etc)
2. Progress of the run (git: Checking out `master', bundler: Bundling, http: Request /companies 243.3ms, etc)
3. Measurements and results in structured format (request_path: '/companies', latency: 24.3, query_count: 12, body: 'Hi!', diff: "-Hello\n+Hi!")
4. Formatted reports (Your branch is 20% less awesome than master)
5. Errors (source of the error, message, and structured details)
6. Logs (Rails server logs)

We don't want to keep doing this over the logger interface because it restricts the freedom we have in the UI when presenting this information.

Short term future

Because PerfCheck currently runs in the same process space as PerfCheck CI we can easily define some Interface in Ruby to pass this information back.

For example:

perf_check.publish(
  :measurement,
  request_path: '/sessions/new',
  latency: 24.3
)
perf_check.publish(
  :progress,
  "Checking out `#{branch}'
  branch: branch
)
perf_check.publish(
  :error,
  "Failed to check out `#{branch}'",
  branch: branch,
  output: stderr.read
)
perf_check.publish(
  :error,
  "Failed to check out `#{branch}'",
  branch: branch,
  output: stderr.read
)
perf_check.publish(
  :log, output,
  job_uuid: @uuid,
  name: 'Rails server log'
)

On the other end we can subscribe to these publish events and do something with it.

perf_check.on_published(:measurement, ->(**details) {
  perf_check.logger.info(
    PerfCheck::Measurement.table.row(
      *details.slice(:request_path, :latency)
    )
  )
})
perf_check.on_published(:progress, ->(message, **details) {
  perf_check.logger.info(message)
})

perf_check.on_published(:log, ->(output, **attributes) {
  Job.find_by!(uuid: job_uuid).logs.create!(attributes)
})

Long term future

At some point we might switch to a distributed architecture where PerfCheck runs in multiple instances and reports back to PerfCheck CI through HTTP or ZeroMQ or something.

perf_check.on_published(:measurement, ->(**details) {
  perf_check.zero_mq.send_string(JSON.dump(details))
})

When a job is in progress, it should be automatically stopped when a new job is queued with the same branch and the same test URL(s).

We currently have flip flopping specs because the specs sometimes leave the git working directory in a weird state.

In the previous situation (i.e. using perf_check_daemon) we would always start perf_check in the ‘current branch’. This meant we never had to change branches at the start of the run. So:

Basically there are three relevant branches when running Perf Check:

1. The current branch
2. The reference branch (zero measurement for comparison)
3. The experimental branch

• As a user I want to automatically switch to the experimental branch when it's not the current branch when running in deployment mode.
• As a user I want to automatically switch back the current branch at the end of the experiment when running in deployment mode.

It should be possible to run perf_check from a Rails project without using bundle exec.

I think we should always bundle on the ref branch to save the user the pain of finding out they haven't done so since pulling.

My only question is if Gemfile.lock would get modified on the ref branch then? I'm thinking not in 99% of cases.

putting '@PerfCheck' into a github PR causes a perf check to be run.

However, when the database that perf check uses is being backed up, perf-check reports spurious, random errors.

Perf check should either stall while it's database is being restored, and should not process github callbacks until it's actually ready for work or error in a more sane way.

Create one single class to compute summary statistics on a list of profiles.

The PerfCheck::Git class stores the PerfCheck logger in an instance variable when it initializes so it never updates when the logger accessor on PerfCheck is used to change the logger.

Some app changes appear only with envar changes, so it should be possible to set envars on the "control" test and set the same envars on the "test" runs.

Suggested: -m ENVAR=VALUE1 and -m ENVAR=VALUE2

The spec suite will currently issue git commands against the perf_check working directory instead of against the test apps directly. Doing so can break the current git state when running specs on change. A solution is to pack an entire test app with a git directory and run them completely isolated.

Some applications require additional tools to run before a Rails server can be started like yarn install.

If we don't deamonize the Rails server we have far more direct control over the process.

Open3.popen2e(
  'rails',
  'server',
  '--environment', rails_environment,
  '--binding', rails_hostname,
  '--port', rails_port.to_s,
  chdir: app_root
) do |stdin, output, thread|
  stdin.close
  # Do things and then quit the server.
  Process.kill('TERM', thread.pid)
  # After the process stopped we can read the entire server output. This is
  # useful when debugging broken runs and possibly digging performance
  # information out of the request logs.
  logger.debug(output.read)
end

Would like to at least support 302. On the other hand, 302 is a handy indicator of an unauthorized session for some apps (e.g. redirect_to '/account/login'). Maybe (dis)allow it with a command line flag like --consider-302-successful.

Let's check out whether it's a need to force-set cache_classes when code isn't being changed between requests. Basically, is it a consistent amount of overhead? Widely variable?

There is no really good reason to continue trying to perform requests when a git reset fails. We could technically differentiate between fatal errors (e.g. git issues) and acceptable failures (e.g. determining memory usage with ps).

When running perf_check as a CLI it's pretty obvious that you will want to log to disk or one of the standard outputs. In a server environment like PerfCheckCI we want to redirect the PerfCheck logger to a very specific IO so we can separate PerfCheck for framework output easily without having to create files on disk.

• As a developer I want to initialize PerfCheck with a logger object so I have freedom to redirect test output.

• As a developer I want to get all measurement from a PerfCheck so I can store them for later use.

References #61.

In a specific edge case in the PerfCheck CI test suite we built an app that has a blank database when we run PerfCheck.

The following happens:

1. Fetch and hard reset to experiment branch
2. Bundle install
3. Trigger before start callbacks
4. Run migrations
5. (Re)start Rails server etc…

When the before start callbacks attempt to access something introduced by the migrations it will not be able to do so.

One solution would be to trigger the start callbacks after the migrations, but that might introduce other issues.

Potential solutions:

1. Move the callbacks and hope for the best.
2. More fine-grained callbacks (eg. before_all, before_setup, before_each_test_case, etc).
3. Insane callbacks by using event handlers instead of callbacks*

Callbacks

We can have a system where every step in a run can have its own before, around, and after callbacks.

# Configure an around callback so you can decide not to yield for
# a certain step in the process to keep it from happening.
perf_check.steps.around(:start) do |step|
  if perf_check.options.experiment_branch == 'master'
    perf_check.logger.info(
      "Sorry, running benchmarks against master is " \
      "not allowed."
    )
  else
    step.call
  end
end
# Run something afterwards.
perf_check.steps.after(:migrations) do
  puts "Migrations done!"
end
perf_check.steps.around(:request) do |step|
  puts step.call.body
end

class Git
  def checkout(branch)
    perf_check.steps.call(:checkout) do
      git.checkout(branch)
    end
  end
end

When perf_check has a job in progress, it should provide a "Stop" button on the web UI which, when clicked, would do what it says.

When rails s gets run on the master branch, Gemfile.lock is regenerated, and if it differs from the version on the dev branch then git stash pop will bail.

Git stash applying...
error: Your local changes to the following files would be overwritten by merge:
    Gemfile.lock
Please, commit your changes or stash them before you can merge.
Aborting

An object representing a run or perf_check could keep all the profiles. Then perf_check.profiles could return all the measurements. Statistics code and figure out how to interpret depending on the report (ie. compare branches, compare paths, compare responses).

[
  { branch: 'master', request_path: '/companies', latency: 0.23, query_count: 23, status: 200 },
  { branch: 'master', request_path: '/companies', latency: 0.27, query_count: 23, status: 200 },
  { branch: 'slower', request_path: '/companies', latency: 0.56, query_count: 37, status: 200 },
  { branch: 'slower', request_path: '/companies', latency: 0.66, query_count: 37, status: 200 }
]

Most applications use some form of authentication so it's probably useful to add some sort of mechanism to PerfCheck to select a role and/or user for authentication.

I think we want to pass the role as a string so the target application can resolve it in a useful way. In the context of this issue I don't care how the target application does this.

perf_check --user-role admin /manage/users

For selecting a specific user we also want to accept a string.

perf_check --user [email protected] /dashboard
perf_check --user 5543453 /dashboard

We can also limit the API surface by forcing users to encode their user scheme into some sort of serialized form.

perf_check --user company:12,user:5 /dashboard
perf_check --user role=admin /admin/board

Internally we can call this options.user or option.authenticated_user for extreme clarity. Eventually it should be replaced by options.cookie or options.headers.

I've seen some routes return the format.csv block when format.html is also present...

Somehow...I'm thinking

1. We should be like YO 500 ERROR HAPPENED!
2. More explicitly instruct peeps to check out /perf_check_failed_request.html for trace

sudara@juno ⚡️  ~/Sites/alonetone (master) ✗ perf_check /sudara/tracks
Yo, profiling master vs. master isn't too useful, but hey, we'll do it
=============================================================================
PERRRRF CHERRRK! Grab a ☕️  and don't touch your working tree (we automate git)
=============================================================================
starting rails...

Benchmarking /sudara/tracks:
    Request  0: —— FAILURE (HTTP 500): /perf_check_failed_request.html

Using Process.spawn will create a process in the same process group as the parent (i.e. the Sidekiq process) and inherit most of its environment. A contaminated environment causes Bundler, Webpacker, and other tools to bleed over their settings into the new process.

A solution is to use Process.spawn options to decouple the new process as much as possible when creating a new process and re-use this as much as possible for rails server, bundle install, and rails db:migrate.

From the option parser I get the idea that --branch and -b should be able to select the experiment branch.

There is a -branch option which is currently interpreted by OptionParser to be --branchranch. The -b option is configured for both the brief and branch option.

All commands should probably go through a single execute method on PerfCheck to make sure they are all execute on the correct path.

When we get a 3xx response from a server it's not entirely clear if the request was successful. It helps to see where the redirect goes to know if it worked properly.