-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[pgbouncer] Fixed newest versions failing checks due to extra field #735
Conversation
@ISauve, thanks for your PR! By analyzing the history of the files in this pull request, we identified @olivielpeau, @gmmeyer and @degemer to be potential reviewers. |
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.
Could you please update the changelog and manifest? You can take a look at other PR's to see what changes are expected.
Other than that it looks good! Just one nit. 🙇
pgbouncer/check.py
Outdated
@@ -50,7 +50,7 @@ class PgBouncer(AgentCheck): | |||
('database', 'db'), | |||
('user', 'user'), | |||
], | |||
'metrics': [ | |||
'metrics': [ |
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.
nit: can we remove this extra space?
row = row[:-1] | ||
elif len(row) == len(cols) + len(desc) + 2: |
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.
As we discussed in person, we cannot be 100% sure if we'll get all extra columns in other versions, but since we can't really get the column titles without resorting to some crazy VIEW trickery (http://peter.eisentraut.org/blog/2015/03/25/retrieving-pgbouncer-statistics-via-dblink/) - this sensible assumption is probably the best we can do. Tests seem to confirm this now works on newer pgbouncers.
448de6d
to
bbd8973
Compare
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.
Let's go!
A customer reported that the newest version of pgbouncer was failing our check because the 'SHOW POOLS' query returned an extra column (issue #555). This query already returned a variable number of columns (either 10 or 11 depending on which version of pgbouncer was being run), and the newest update means that this number can now vary from 10-12.
A brief description of the change being made with this pull request.
Added a small fix so that if there is 1 or 2 extra columns returned, check.py will now just ignore them.
Also added checks to travis.yml for the different versions of pgbouncer currently available on docker.
Versioning
manifest.json
CHANGELOG.md
. Please useUnreleased
as the date in the titlefor the new section.
Additional Notes
The issue with this approach is that our check simply verifies if we have at least 10 columns returned (and no more than 12), but it does not check which columns. So, were there to be a case where an extra column is returned but a main metric is not (due to some other issue), the test would still pass.
The best solution I could see would be to check for the existence of these extra fields (version 1.7.2 introduced pool_mode and the newest versions will now also have maxwait_us) and then adjusting the number of expected columns accordingly - however, the only information accessible through the connection to the PostgreSQL db is the return value of the query, not the accompanying keys identifying which value belonged to what. Thus this may not be possible.
Another possible solutions could be to check which version of pgbouncer is being run, and have a set number of columns that should be returned for that version - but again, this information is not accessible.