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 PGHero to the demo app #1294

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

mec
Copy link
Contributor

@mec mec commented Mar 21, 2024

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:

  • are we happy with PGHero being publiclally accessible on the demo app, it's usually not something you would ever want open, but the demo app is a pretty unusual context?
  • I wasn't sure about adding the histrorical stats as the docs say "The query stats table can grow large over time" which might be a pain on the demo app - we can always enable them later.

@bensheldon
Copy link
Owner

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.

@mec
Copy link
Contributor Author

mec commented Mar 21, 2024

Yep, basic auth seems good - let me give it a whirl and see on here.

I am assuming we can trigger the PgHero.capture_query_stats with GoodJob as well - let me have a look.

@bensheldon
Copy link
Owner

bensheldon commented Mar 21, 2024

Thank you!!! 🙇🏻

I am assuming we can trigger the PgHero.capture_query_stats with GoodJob as well

Yep, it's in the job example I copy pasted (in addition to the cleanup)

@mec mec marked this pull request as draft March 21, 2024 18:19
mec added 3 commits March 21, 2024 18:23
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
@mec
Copy link
Contributor Author

mec commented Mar 22, 2024

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 RAILS_ENV=development without PgHero you can see that lots is happening, whereas once PgHero is just available the behaviour is completely different, it just sits there doing nothing (as best I can tell) - this only seems to be the case with RAILS_ENV=development as RAILS_ENV=demo looks to be working as expected.

The following spec fails:

context 'when development async' do
because the jobs never launch like they do usually, so 'Enqueued ExampleJob' is not present, again the 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 👂

@bensheldon
Copy link
Owner

@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.

it 'fails without the basic auth credentials' do
visit pg_hero_path

expect(status_code).to eq 401
Copy link
Owner

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:

https://github.com/ankane/pghero/blob/6d1c688a0917a4a240c0599b5aecb9b4b8c0c15a/app/controllers/pg_hero/home_controller.rb#L7

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.

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

Successfully merging this pull request may close these issues.

Install PgHero on the Demo app
2 participants