Add multihost support for PostgreSQL collector #646
Conversation
src/collectors/postgres/postgres.py
Outdated
| for instance in self.config['instances'].keys(): | ||
| # Get list of databases | ||
| #import pdb | ||
| #pdb.set_trace() |
|
Changes Unknown when pulling 1ebc62f on wstranger:master into ** on python-diamond:master**. |
|
Changes Unknown when pulling e6d03ba on wstranger:master into ** on python-diamond:master**. |
src/collectors/postgres/postgres.py
Outdated
| config.update({ | ||
| 'path': 'postgres', | ||
| 'host': 'localhost', |
There was a problem hiding this comment.
Currently this PR is a breaking change. However, if you keep this config option, add it to the instance list in the collect method if it isn't already in it, and then exclude host from falling back to the default in _get_config, then it would not be a breaking change.
There was a problem hiding this comment.
I already added it in my last commit in the collect method. Same way as pgbouncer collector.
There was a problem hiding this comment.
I don't see the host config option being used in e6d03ba
shortdudey123
added
needs: feedback from author
and removed
needs: tests failures fixed
labels
|
Changes Unknown when pulling b6eb57d on wstranger:master into ** on python-diamond:master**. |
|
Can you quickly give a run down on why the multiple collector core code doesn't work for this? Thanks! |
|
I didn't find any suitable implementation in core and use pgbouncer collector code as reference. |
|
Hrm. I can't find it documented in the docs :( Basically, core allows you to spin up multiple instances of any collector via multiple config files of the pattern |
|
Ok. At the doc http://diamond.readthedocs.io/en/latest/Getting-Started/Configuration/ - we can see next -
Hrm. Nothing about wildcards. |
|
PR LGTM now |
|
|
||
| # HACK: setting default with subcategory messes up merging of configs, | ||
| # so we only set the default if one wasn't provided. | ||
| if not instances: |
There was a problem hiding this comment.
Won't this code never run since you defined a default for instances that is not None?
| # HACK: setting default with subcategory messes up merging of configs, | ||
| # so we only set the default if one wasn't provided. | ||
| if not instances: | ||
| instances = { |
There was a problem hiding this comment.
You need to set to self.instances since you use it in _get_fig()
| @@ -14,43 +14,60 @@ class TestPostgresqlCollector(CollectorTestCase): | |||
| def setUp(self, allowed_names=None): | |||
| if not allowed_names: | |||
| allowed_names = [] | |||
|
|
|||
| default_config = get_collector_config('PostgresqlCollector', {}) | |||
| self.default_collector = PostgresqlCollector(default_config, None) | |||
There was a problem hiding this comment.
Doesn't look like you ever used this. Can you add tests using it to test the default config behavior?
Add multihost support for PostgreSQL collector