-
Notifications
You must be signed in to change notification settings - Fork 8
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
MSPSDS-472: Improve technical setup #246
Conversation
# This is the page that will show for timeouts, currently showing the same as an internal error | ||
match "/503", to: "errors#internal_server_error", via: :all | ||
|
||
mount PgHero::Engine, at: "pghero" |
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.
Do we want this in production? Also, do we need to be running https://github.com/ankane/pghero/blob/master/guides/Rails.md#historical-query-stats?
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.
I think we very much do - otherwise we can't properly evaluate the performance.
And that was one of my comments, do
https://github.com/ankane/pghero/blob/master/guides/Rails.md#historical-query-stats and
https://github.com/ankane/pghero/blob/master/guides/Rails.md#historical-space-stats
seem useful here?
I'm tempted to say yes, but also not sure what we'd want to use for the stats capture scheduling. I've had quick look at https://github.com/ondrejbartas/sidekiq-cron, am i barking up the right tree here?
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.
Finally, there's a recommendation to use a separate user for phHero, but the required approach seems super messy:
https://github.com/ankane/pghero/blob/master/guides/Permissions.md
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.
sidekiq-cron sounds like it might work!
Pull Request Test Coverage Report for Build 1155
💛 - Coveralls |
Damn it, I've written a whole host of comments before putting into review that I've accidentally deleted now :P |
@broder as discussed, I'll want to push this through and leave enabling pghero historical stats to a separate PR |
f525166
to
1a64446
Compare
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.
Looks good, thanks!
The code changes comprise: