Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fetch metrics for multiple databases #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fetch metrics for multiple databases #88
Changes from 2 commits
01474e1
fd1a376
3e2aae4
404fb81
53334f5
2b7e80e
fb711d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why only the first database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.