-
-
Notifications
You must be signed in to change notification settings - Fork 204
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 PGHero to the demo app #1294
Conversation
Let's enable query stats and add a job to clean them up. I copied this from another app: # demo/app/jobs/pg_hero_capture_query_stats_job.rb
# frozen_string_literal: true
class PgHeroCaptureQueryStatsJob < ApplicationJob
good_job_control_concurrency_with(
key: "pg_hero_capture_query_stats",
total_limit: 1
)
discard_on StandardError
def perform
PgHero.capture_query_stats
PgHero.clean_query_stats
end
end
# demo/initializers/good_job.rb
# in the cron config...
pg_hero_capture_query_stats: {
cron: "*/10 * * * *", # Every 10 minutes
class: "PgHeroCaptureQueryStatsJob",
description: "Runs PG Hero maintenance",
}, #... It looks like I can configure BasicAuth on the production demo site: https://github.com/ankane/pghero/blob/master/guides/Rails.md#authentication It would be rad if you could add a simple system test that stubbed ENV to enable it, and then verified that basic auth is triggered. But that also sounds like it could be complicated to get working, so not required of this PR. |
Yep, basic auth seems good - let me give it a whirl and see on here. I am assuming we can trigger the |
Thank you!!! 🙇🏻
Yep, it's in the job example I copy pasted (in addition to the cleanup) |
We want to show any helpful performance data in the GoodJob demo app so we are installing PGHero: https://github.com/ankane/pghero We have to add Sprockets to the Rails app so the PGHero assets get loaded correctly, whilst we could have used a more modern asset pipeline, Sprockets is well supported and a simple drop in to get PGHero running. PGHero is available at `/pghero`. We have not enabled: - [Suggested indexes](https://github.com/ankane/pghero/blob/master/guides/Rails.md#suggested-indexes) - [Historical query stats](https://github.com/ankane/pghero/blob/master/guides/Rails.md#query-stats) - [Historical space stats](https://github.com/ankane/pghero/blob/master/guides/Rails.md#historical-space-stats) As these may not be suitable for the demo application and can always be added later.
We want to be sure PgHero is behind basic auth. This spec validates that once the correct environment variables are set, PgHero will require basic authentication. We have to use the rack_test driver so we can use `basic_authorize` to set the headers, rack_test is the default in RSpec/Capybara, but we have explicitly set the driver just in case.
This does the work to enable historical query stats in PgHero, but the Postgres instance backing the service must also be configured, see: https://github.com/ankane/pghero/blob/master/guides/Rails.md#query-stats We are: - adding the tables to store stats - setting up a GoodJob cron job to collect the stats and clean old ones (the demo app generates a lot of queries) For reference, the recommended settings for `postgres.conf`: ``` shared_preload_libraries = 'pg_stat_statements' pg_stat_statements.track = all pg_stat_statements.max = 10000 track_activity_query_size = 2048 ``` See: https://github.com/ankane/pghero/blob/master/guides/Query-Stats.md
So as best I can tell, as soon as we load the PgHero gem it does something that GoodJob does not like at all! If you watch the demo app started with The following spec fails: good_job/spec/integration/server_spec.rb Line 11 in 1acb721
RAILS_ENV has been set to development in this spec. The new PgHero specs also fails when not run in isolation returning 500?
Right now I don't have enough context to understand what is happening in development that is so different to cause this, I'll keep looking, but if you have any ideas I am all ears 👂 |
@mec thank you for getting so far. I can take a look at it. I have PgHero running successfully on other applications so I imagine there's just something odd somewhere. |
spec/system/pghero_spec.rb
Outdated
it 'fails without the basic auth credentials' do | ||
visit pg_hero_path | ||
|
||
expect(status_code).to eq 401 |
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.
Unfortunately it looks like this isn't testable because of how basic auth is implemented in PGHero:
Another option would be to enforce a password in all environments except Development (which I think can be done through pghero's config), but I'm also ok just operating on trust and removing this test.
# Conflicts: # Gemfile # Gemfile.lock # demo/db/schema.rb
We want to show any helpful performance data in the GoodJob demo app so
we are installing PGHero:
https://github.com/ankane/pghero
We have to add Sprockets to the Rails app so the PGHero assets get loaded
correctly, whilst we could have used a more modern asset pipeline,
Sprockets is well supported and a simple drop in to get PGHero running.
PGHero is available at
/pghero
.We have not enabled:
As these may not be suitable for the demo application and can always be
added later.
This work will begin to resolve #1166.
@bensheldon couple of questions: