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 memory-profiler and add ability to selectively enable rack-mini-profiler #24022

Merged
merged 3 commits into from Aug 7, 2018

Conversation

aoby
Copy link
Contributor

@aoby aoby commented Jul 31, 2018

on production front ends for admins.

We have had the rack-mini-profiler available in development mode for several years, though I don't think anyone has been using it. We also had a shortcut in development, where adding ?pp=enabled to the query would turn it on for a running server. For simplicity I removed that and consolidated in one code path. It's not a big deal to add that key to locals and restart the server, and it simplifies the production code path.

In order for the profiler to be enabled, the following must be met:

  1. CDO.rack_mini_profiler_enabled (from locals.yml for instance) must be set to true.
  2. development mode OR any other environment only for admins
  3. (always true, part of the mini profiler gem itself) ?pp must be in the query.

Relevant options for profiling memory (source):

  • ?pp=profile-memory to generate a memory report
  • ?pp=profile-gc to report garbage collection stats
  • ?pp=analyze-memory to report ObjectSpace stats

Once deployed, I intend to ssh into a single front end, add the rack_mini_profiler_enabled setting in locals.yml, restart the dashboard service, then hit that front end directly as outlined in this comment to get live memory reports. Initially, I'll take a few samples, just after DTP and after it's been running for a while, look for patterns, and report to the team.

Additional context and reasoning.

  1. Storage: Rack mini profiler provides 4 storage options: MemoryStore, RedisStore, MemcacheStore, and FileStore. I discussed with @wjordan and decided to use the file store so we can share across processes but not across servers, and because we intend this to be used on demand and in low volume on a single front end.
  2. Production performance impact?
    1. The mini profiler is designed with production in mind.
    2. In production it is only enabled for logged-in admins, with the pp= query param, when CDO.rack_mini_profiler_enabled is set.
    3. The mini profiler community page and several blog posts I've found hold that is has negligible performance impact when it's not enabled in a given page view, "equivalent of about 50 if false statements".

See https://github.com/MiniProfiler/rack-mini-profiler for more description and options.

…rofiler on production front ends for admins
@sureshc
Copy link
Contributor

sureshc commented Aug 1, 2018

Unlike adhoc instances, I believe the front end instances don't have public IP addresses. If that's the case, you could use curl or an HTTP client via the ruby console on production-console to request the profile page from the specific front end that has profiling enabled, but it's hard to send an HTTP request via curl that is authenticated as an admin.

Perhaps you could just keep refreshing your browser until the load balancer sends your request to the server with profiling enabled?

@aoby
Copy link
Contributor Author

aoby commented Aug 1, 2018

Hold off. This isn't working :(

On an adhoc, or in development, setting Rack::MiniProfiler.config.authorization_mode = :whitelist, the default mode on environments aside from dev and test (which default to allow_all), that requires explicit authorization per call, causes it never to actually be authorized.

Initial investigation locally shows that the authorization is being reset between when its enabled and when the page is actually rendered. From some initial logging of stack traces, this appears to be coming from (a conflict with?) the newrelic_rpm gem. I need to investigate / debug further...

@wjordan
Copy link
Contributor

wjordan commented Aug 1, 2018

then hit that front end directly via the public ip from EC2 (not through the load balancer)

As Suresh noted, our production-frontend EC2 instances don't have public ip addresses for security purposes. You can reach the private IP from within our VPC, either on production-console or via gateway.

See Debugging Frontend instances on our Wiki for instructions on setting up SSH port-forwarding through gateway to connect to a specific frontend through your local browser.

Copy link
Contributor

@wjordan wjordan left a comment

Choose a reason for hiding this comment

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

LGTM once the authorization setting is figured out (and typo/test is fixed; see comment)

@@ -81,7 +83,7 @@ def reset_session_endpoint

def render_404
respond_to do |format|
format.html {render file: 'public/404.html', layout: 'layouts/application', status: :not_found}
format.html {render file: 'pblic/404.html', layout: 'layouts/application', status: :not_found}
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a typo? (also seems to be the cause of ci test failures)

# setting the environment variable prior to running the server, or by hitting
# a page with a "pp=enabled" query string parameter and then refreshing the page.
Rack::MiniProfiler.config.pre_authorize_cb = lambda {|_env| ENV['RACK_MINI_PROFILER'] == 'on'}
# Only pre-authorize the min profiler when this setting is enabled (in locals.yml).
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: 'mini'

@aoby
Copy link
Contributor Author

aoby commented Aug 7, 2018

Thanks @wjordan for all the help figuring our how to make this work on adhoc (and therefore hopefully production 🤞).

The key was whitelisting the cookie that rack-mini-profiler uses internally to maintain state so it is not stripped.

@aoby aoby merged commit 23c762c into staging Aug 7, 2018
@aoby aoby deleted the mem-profile branch August 7, 2018 20:16
@aoby
Copy link
Contributor Author

aoby commented Aug 7, 2018

Investigate memory leak

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

Successfully merging this pull request may close these issues.

None yet

3 participants