Add the ability to specify 'use_cached' connections in config #2000
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
During our use of the datadog pgbouncer check, we found an issue where the agent would fail to reconnect to a recovered pgbouncer process. We simulated this by starting our `pgbouncer` and `datadog-agent` processes, and then subsequently stopping `pgbouncer`, waiting a few minutes, and restarting `pgbouncer`. The agent would correctly alert us that `pgbouncer` was down, but upon recovery, the agent would continue to be in an error state. We would have to restart the agent to get it reporting "healthy" again. We have reason to believe this is due to the caching of the underlying Postgres connection. We simulated this again with the change in this PR and the agent behaved as expected. Please let us know if there's additional testing coverage or simulations we can provide. Cheers!
kellydunn
force-pushed
the
pgbouncer-exposed-cached
branch
from
73df382
to
8731fd6
Compare
|
We've been running this fork of the
Let us know any more context or perfomance data you may need when reviewing this PR :) Thanks! |
ofek
requested changes
Aug 7, 2018
| @@ -206,6 +206,7 @@ def check(self, instance): | |||
| password = instance.get('password', '') | |||
| tags = instance.get('tags', []) | |||
| database_url = instance.get('database_url') | |||
| use_cached = instance.get('use_cached', True) | |||
There was a problem hiding this comment.
Can you please wrap this with is_affirmative? from datadog_checks.config import is_affirmative
| @@ -206,6 +206,7 @@ def check(self, instance): | |||
| password = instance.get('password', '') | |||
| tags = instance.get('tags', []) | |||
| database_url = instance.get('database_url') | |||
| use_cached = instance.get('use_cached', True) | |||
There was a problem hiding this comment.
Can you please also document this here? https://github.com/DataDog/integrations-core/blob/master/pgbouncer/datadog_checks/pgbouncer/data/conf.yaml.example
|
@ofek Thanks for the feedback! Let me know if you think I need to address anything else! Thanks! |
ofek
approved these changes
Aug 7, 2018
|
LGTM. Also, congratulations on getting PR number |
ofek
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
During our use of the datadog pgbouncer check, we found an issue where the agent would fail to reconnect to a recovered pgbouncer process. We simulated this by starting our
pgbounceranddatadog-agentprocesses, and then subsequently stoppingpgbouncer, waiting a few minutes, and restartingpgbouncer.The agent would correctly alert us that
pgbouncerwas down, but upon recovery, the agent would continue to be in an error state. We would have to restart the agent to get it reporting "healthy" again. We have reason to believe this is due to the caching of the underlying Postgres connection.We simulated this again with the change in this PR and the agent behaved as expected.
Please let us know if there's additional testing coverage or simulations we can provide.
Cheers!
What does this PR do?
This enables users to configure whether or not they want to used cached connections to check pgbouncer.
Motivation
As mentioned above, we were evaluating this check in our development environment and we observed that when a
pgbouncerprocess stopped, and restarted again, the datadog agent would continually think it was in an error state. We have reason to believe it is due to caching the connection object and attempting to call functions on it after it has been closed.Review checklist
no-changeloglabel attachedAdditional Notes
I'm more than open to explore other possibilities too, like catching a different exception or finding out why
ShouldRestartExceptionisn't being thrown in this instance.