Add query blocker tracking based on pg_blocking_pids() #3
Conversation
jeffdoering
requested review from
tedcheng,
montanalow,
mmontagna and
christoudias
| end | ||
|
|
||
| def server_version_num | ||
| @server_version_num ||= select_one("SHOW server_version_num").to_i | ||
| @server_version_num ||= select_one('SHOW server_version_num').to_i | ||
| end |
There was a problem hiding this comment.
Any functional reason why you're updating all the quote types?
There was a problem hiding this comment.
Rubocop was complaining about double-quoted strings that didn't require interpolation. It made sense so I updated files I was working on anyway to suppress the warning and make the distinction clear.
Now that you asked it looks like the Ruby style guide leaves this open-ended and the Rubocop setting is configurable in my IDE (RubyMine). I hadn't checked that context and which (if any) model Instacart uses standard.
I can revert the unnecessary changed lines now that I see it's not an agreed Ruby standard:
There was a problem hiding this comment.
String quote style in ruby is a vi/emacs style holy war.
|
This is looking great! I'm excited for this feature. How as this tested? Asking cause it's a large change and I'm also curious how to develop on this app. |
Good question. I developed and did primary testing in a standalone Rails app I created locally and pulled PgHero into. I manually tested via the UI on the local environment. I also added test cases for the blocker functionality and added some extra test cases for existing functionality. The cases are not deep in terms of verifying output data but they ensure that the code executes successfully and appears to return something useful. They did catch many small bugs during my refactoring. But now that I've dug deeper; the same is possible by running Blazer locally. I have that 95% working but didn't do the full Blazer setup around sign-on - still need to try that part. |
jeffdoering
force-pushed
the
jeffdoering/blocker_tracking
branch
from
ea1d41b
to
fda3767
Compare
| require "pghero/version" | ||
|
|
||
| module PgHero | ||
| autoload :Connection, "pghero/connection" | ||
| autoload :QueryStats, "pghero/query_stats" |
There was a problem hiding this comment.
Is there a reason to disable autoloading, without requiring these? Are these effectively dead code now?
| end | ||
|
|
||
| def server_version_num | ||
| @server_version_num ||= select_one("SHOW server_version_num").to_i | ||
| @server_version_num ||= select_one('SHOW server_version_num').to_i | ||
| end |
There was a problem hiding this comment.
String quote style in ruby is a vi/emacs style holy war.
The feature is modeled after other historical data tracking in PgHero (e.g. query stats). The tracked stats are not shown in PgHero UI yet (it still shows the older live blockers information with no history) but will be visible via direct table queries. Separate PR will enable rendering of query blockers.
Each sample creates a row in pghero_blocker_samples even if there are no blockers; this is to ensure that historical data can be accurately distinguished from missing data. Actual blockers and blockees are stored in pghero_blocker_sample_sessions.
The PR also restructures the base methods and objects to cleanly separate the monitored DB from the respository used to store historical data and allow reuse of methods for both cases. It also includes minor cosmetic changes to files that were touched to accomplish the restructuring (e.g. RuboCop suggestions on strings, etc).