Fetch metrics for multiple databases #88
Conversation
* add info to changelog
bin/metric-postgres-locks.rb
Outdated
| locks_per_type[lock_name] = row['count'] | ||
| databases = pgdatabases | ||
| con = PG.connect(host: config[:hostname], | ||
| dbname: databases.first, |
There was a problem hiding this comment.
The first database from the list is simply used to make a valid connection and the locks statistics are then queried through SQL statements for all databases. It is simply not necessary to make individual connections for each database. Please see line 99-103 for the queries run for each database.
There was a problem hiding this comment.
ah gotcha, that makes sense, would not hurt to add a comment about that. I will review this in greater detail when I have more time. Is there any way we can provide backwards compatibility?
There was a problem hiding this comment.
I currently see two possible options:
Option 1:
Require the user to set the --db option to * if all databases should be checked. If it is not set to * the current behaviour is retained.
But in lib/sensu-plugins-postgres/pgpass.rb there is a check for * to switch to fallbacks and to be consistent the switch to fallbacks should no longer be done which will also introduce a breaking change (with less users affected).
Option 2:
Add a new option e.g. --all-databases which will override the --db option. This means the user has to specify the --all-databases option if he really would like to get metrics for all databases or use the existing --db option to list database names explicitly.
Also Option 1 has the advantage over Option 2 that it is easier to parameterize the check through tokens if there is no option to invalidate/override another.
Let me know what you think or if you have another idea.
There was a problem hiding this comment.
I think its a trade off between backwards compatibility and code readability I tend to prefer to not break users regardless of how small the numbers are unless there is a compelling reason.
|
Seems like the conversation is paused. Any update @majormoses @ydkn ? |
bin/metric-postgres-connections.rb
Outdated
| option :database, | ||
| description: 'Database name', | ||
| option :databases, | ||
| description: 'Database names, separated by ";"', |
There was a problem hiding this comment.
I am thinking it might be safer to pass it with a comma separated list? Is it because we are passing this as raw SQL?
There was a problem hiding this comment.
You're right, using "," as separator is better than ";".
Also it was not escaped when used in a query.
I've updated the PR with patches for both.
* add escaping of query parameters where possible
|
PR updated to be compatible with current master (fbc7833) |
| con = PG.connect(host: config[:hostname], | ||
| dbname: config[:database], | ||
| dbname: databases.first, |
There was a problem hiding this comment.
I think this still needs to be updated right?
There was a problem hiding this comment.
Updated in which way? It connects to the first DB and then queries the information for all databases. There is no need to connect to each DB individually.
| con = PG.connect(host: config[:hostname], | ||
| dbname: config[:database], | ||
| dbname: databases.first, |
There was a problem hiding this comment.
I think this still needs to be updated right?
There was a problem hiding this comment.
Updated in which way? It connects to the first DB and then queries the information for all databases. There is no need to connect to each DB individually.
Co-Authored-By: Ben Abrams <[email protected]>
Co-Authored-By: Ben Abrams <[email protected]>
Pull Request Checklist
General
[ x] Update Changelog following the conventions laid out here
[ x] Update README with any necessary configuration snippets
[ x] Binstubs are created if needed
[x ] RuboCop passes
[ x] Existing tests pass
Purpose
Allow to specify multiple databases separated by a semicolon instead of only one database to gather metrics from.
Known Compatibility Issues
If the database CLI argument (-p, --db) was not set now metrics for all databases are returned.
Suggestions how to be more backward compatible are welcome!