[postgres] Collect StatIO per table & revamp test #1415
Conversation
|
@remh I still have to update the test for it. |
LeoCavaille
force-pushed
the
alq666/pg-statio
branch
2 times, most recently
from
89d1b81
to
f52c827
Compare
degemer
changed the title
| @@ -177,7 +177,7 @@ class PostgreSql(AgentCheck): | |||
| ('schemaname', 'schema') | |||
| ], | |||
| 'metrics': { | |||
| 'pg_stat_user_tables': ('postgresql.total_tables', GAUGE), | |||
| 'pg_stat_user_tables': ('postgresql.table.count', GAUGE), | |||
There was a problem hiding this comment.
This might break existing user's screens, alerts, etc. Renaming a metric that's out there could be considered a breaking change.
There was a problem hiding this comment.
Agreed. I checked and no such screens and monitors exist right now. We can recheck before we release.
There was a problem hiding this comment.
@alq666 @miketheman should have added a note.
That's normal b/c this is a new metric post 5.2 so it hasn't been released to the crowds yet. That's why I had no mercy renaming it.
See : bee8eec
There was a problem hiding this comment.
Notes are good! :) Thanks for the explain @LeoCavaille .
| } | ||
| ] | ||
|
|
||
| self.run_check(dict(instances=instances)) |
There was a problem hiding this comment.
Let's use run_check_twice instead ?
There was a problem hiding this comment.
aha nice, of course. That didn't even existed when I submitted the PR ⏪
|
Looks great besides the tiny nitpicks. It was definitely not an easy refactoring. |
We read this new PG table to get by relation metrics. See http://www.postgresql.org/docs/9.2/static/monitoring-stats.html#PG-STATIO-ALL-TABLES-VIEW
Some better metric names avoiding ugly abbreviations. + we make these new metrics relation-scoped meaning that we'll only collect these metrics for the list of relation that has been input by the user in the configuration file. This avoids a possible unbounded (in number of relations) number of metrics.
To be more consistent with the database count `postgresql.db.count` the table count has been renamed from `postgresql.total_tables` to 'postgresql.table.count`
* Otherwise it will fail at formatting the query * Removed unused `warning_func` def * Renamed the scope for more consistency
Regression introduced by 70374b4. Basically returning an empty dict as metrics generates a bad query which then fails the whole transaction for this other database. Instead we return None and skip the collection proeperly.
Fix regression introduced by #1395. The idea is that because the pre-processing function `_process_customer_metrics` was modifying "in-place" instance attributes, and for instance converting the string "GAUGE" to the method "AgentCheck.gauge" at the first pass, the second time it would expect to find a string and fail when it encounters a method. Instead we cache the pre-preprocessed dict the first time, and hit this dictionnary in the next runs if it is present.
* Use the `AgentCheckTest` class which makes the code much more readable and enjoyable for edge cases. * Factorize the SQL commands into SQL files that are input in psql commands * Minor logging facelift of the common test class
`test_postgresql` was the only file named postgresql instead of postgres (ci/postgres.rb, checks.d/postgres.py), also breaking the implicit convention that integration test of checks.d/my_test.py should be called test_my_test.py
+ refactor a bit the code with the new handy methods that were added to AgentCheckTest + added a bunch of FIXMEs in the code where me mutate caps-variables which are supposed to be constant. Not doing it now b/c it involves a further refactoring of the whole check logic
LeoCavaille
force-pushed
the
alq666/pg-statio
branch
from
33b5372
to
ab60a13
Compare
|
CI passed, implemented your different comments @remh thanks. Merging. |
[postgres] Collect StatIO per table & revamp test
|
Changes Unknown when pulling ab60a13 on alq666/pg-statio into * on master*. |
Based on @adriandoolittle's work, but restricted to relations that are listed in the configuration to avoid blowing up the number of metrics.