[postgres] Collect StatIO per table & revamp test #1415

alq666 · 2015-03-07T03:33:20Z

Based on @adriandoolittle's work, but restricted to relations that are listed in the configuration to avoid blowing up the number of metrics.

alq666 · 2015-03-07T03:33:41Z

@remh I still have to update the test for it.

miketheman · 2015-03-25T20:38:15Z

checks.d/postgres.py

@@ -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),


This might break existing user's screens, alerts, etc. Renaming a metric that's out there could be considered a breaking change.

Agreed. I checked and no such screens and monitors exist right now. We can recheck before we release.

@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

Notes are good! :) Thanks for the explain @LeoCavaille .

remh · 2015-04-02T22:08:41Z

tests/test_postgres.py

+            }
+        ]
+
+        self.run_check(dict(instances=instances))


Let's use run_check_twice instead ?

aha nice, of course. That didn't even existed when I submitted the PR ⏪

remh · 2015-04-02T22:21:30Z

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 · 2015-04-07T23:15:20Z

CI passed, implemented your different comments @remh thanks. Merging.

[postgres] Collect StatIO per table & revamp test

coveralls · 2016-10-14T13:18:54Z

Changes Unknown when pulling ab60a13 on alq666/pg-statio into * on master*.

LeoCavaille added this to the 5.3.0 milestone Mar 13, 2015

LeoCavaille self-assigned this Mar 13, 2015

LeoCavaille added checks feature labels Mar 13, 2015

LeoCavaille force-pushed the alq666/pg-statio branch 2 times, most recently from 89d1b81 to f52c827 Compare March 20, 2015 00:49

degemer changed the title ~~Collect StatIO per table~~ [postgres] Collect StatIO per table & revamp test Mar 25, 2015

degemer mentioned this pull request Mar 25, 2015

[tests] review of all our integration tests #1458

Closed

53 tasks

miketheman reviewed Mar 25, 2015
View reviewed changes

LeoCavaille removed their assignment Mar 25, 2015

remh reviewed Apr 2, 2015
View reviewed changes

adriandoolittle and others added 9 commits April 7, 2015 15:37

[postgres] new metrics from pg_statio_all_tables

8870d8c

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

[postgres] Rename table count metric

cba5daf

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`

[postgres] Make STATIO a per relation scope

2d921ad

* Otherwise it will fail at formatting the query * Removed unused `warning_func` def * Renamed the scope for more consistency

[postgres] Fix single query of per instance metrics

1629ce0

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.

[postgres] Revamp the test suite

e5c892e

* 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

[postgres] rename test_postgresql in test_postgres

1e33409

`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

LeoCavaille force-pushed the alq666/pg-statio branch from 33b5372 to ab60a13 Compare April 7, 2015 21:34

LeoCavaille added a commit that referenced this pull request Apr 7, 2015

Merge pull request #1415 from DataDog/alq666/pg-statio

c1570d8

[postgres] Collect StatIO per table & revamp test

LeoCavaille merged commit c1570d8 into master Apr 7, 2015

LeoCavaille deleted the alq666/pg-statio branch April 7, 2015 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[postgres] Collect StatIO per table & revamp test #1415

[postgres] Collect StatIO per table & revamp test #1415

alq666 commented Mar 7, 2015

alq666 commented Mar 7, 2015

miketheman Mar 25, 2015

alq666 Mar 25, 2015

miketheman Mar 25, 2015

LeoCavaille Mar 25, 2015

miketheman Mar 25, 2015

remh Apr 2, 2015

LeoCavaille Apr 2, 2015

remh commented Apr 2, 2015

LeoCavaille commented Apr 7, 2015

coveralls commented Oct 14, 2016

[postgres] Collect StatIO per table & revamp test #1415

[postgres] Collect StatIO per table & revamp test #1415

Conversation

alq666 commented Mar 7, 2015

alq666 commented Mar 7, 2015

miketheman Mar 25, 2015

Choose a reason for hiding this comment

alq666 Mar 25, 2015

Choose a reason for hiding this comment

miketheman Mar 25, 2015

Choose a reason for hiding this comment

LeoCavaille Mar 25, 2015

Choose a reason for hiding this comment

miketheman Mar 25, 2015

Choose a reason for hiding this comment

remh Apr 2, 2015

Choose a reason for hiding this comment

LeoCavaille Apr 2, 2015

Choose a reason for hiding this comment

remh commented Apr 2, 2015

LeoCavaille commented Apr 7, 2015

coveralls commented Oct 14, 2016