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

NH-95676: init commit for ruby benchmark #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

xuan-cao-swi
Copy link
Contributor

@xuan-cao-swi xuan-cao-swi commented Jan 3, 2025

Large chunk of file is rails app code. It can be ignored.
The more interesting part are locust file (how avg response time is calculated) and docker compose file (include how services are setup and configured)

@xuan-cao-swi xuan-cao-swi requested a review from a team January 3, 2025 22:05
Copy link

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

Wow yes there are a lot of files for the test app, many of what's under the config directory make me concerned, chiefly:

  • there are solarwinds initializers containing the service key--we should NEVER commit these even if they are for testing accounts, if these are valid please rotate them immediately.
  • config/application.rb has a lot of customization that i'm not sure is actually needed.

You could do a thorough pass through the test application code to clean things out, but i'd prefer the following:

Let's replace the learn-rails app with a freshly generated one. From quick look, learn-rails code is pretty old and not receiving updates, so unless there's some particular reason to use it, this would be preferable:

rails new test-app --minimal --skip-bundle --skip-ci --skip-docker --skip-git

Let's simplify the directory structure so instead of

|_ benchmark
   |_ learn-rails

We have

|_ test-app

and can we remove the README / ab_test* regarding memory overhead test, i don't think we need these anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants