Replicaset status fix #55
Conversation
|
Oh yeah, here's the before/after |
|
@naemono is there a scenario where you might want that behavior? If so maybe an option to toggle it would make sense. |
majormoses
force-pushed
the
replicaset-status-fix
branch
from
495563c
to
1a83c23
Compare
|
@naemono I rebased for you, any chance you could validate if my question needs any consideration? |
|
@naemono I'd love to see this get merged I am just waiting for an answer to my question so that I can better write a changelog entry. Can you expand on this or update per our guidelines |
|
Chiming in as I have seen the same issue myself recently. I think the only time you may want to not have this by default is if you gather all your metrics through a primary mongod in a cluster. The proposed change disables the Ruby mongodb driver autodiscovery during connect. https://docs.mongodb.com/ruby-driver/master/tutorials/ruby-driver-create-client/#ruby-options This is useful when when you have sensu-client installed on all your servers and gather metrics locally (which I'm guessing is the standard for most Sensu users). To be safe, maybe it could revert to auto-discovery when "--require-master" is true. An even better but much larger change could be to allow to specify a mongodb uri to connect which would make it simpler to properly support all topology. |
|
@majormoses sorry for the delays. I think @eberkut summed it up. I think typical user wants stats on all nodes of a replicaset, not just current master, or read-preference. It seems confusing. |
|
Thanks for answering, based on the responses there should definitely be an argument to control the behavior. You can't really make assumptions on how people use it. I saw someone make a similar assumption in another plugin and it would have really broken my use of it. If we want this to be a patch/bug fix then it should be defaulting to the old behavior and operators can opt in for new change. If we want to consider this a feature we can do it under a major release under a |
|
@majormoses I'll add as an argument. No problem. |
|
@naemono hey just checking if you had any plans to get back to this? If not I can work on this when I get some time in the next few weeks. |
…ng 'read preference'
majormoses
force-pushed
the
replicaset-status-fix
branch
from
1a83c23
to
1534adb
Compare
|
I rebased this to fix conflicts, if you'd like to get back to this let me know if not I will try to sit down tonight to update this. |
|
I have updated the pull request to reflect what was discussed here. If someone can run a quick test and see if this works I am 👍 to merge and release (as a major version). |
majormoses
force-pushed
the
replicaset-status-fix
branch
from
83d132e
to
4ec51e5
Compare
- when retrieving metrics it will now pull only local metrics rather than all nodes in the reiplca set. To revert this behavior you can add `--connect replica_set` - control of connection type for metric gatherering scripts Signed-off-by: Ben Abrams <me@benabrams.it>
majormoses
force-pushed
the
replicaset-status-fix
branch
from
4ec51e5
to
c9ed3f8
Compare
There was a problem hiding this comment.
Adding the following patches will hopefully resolve #54 (comment)
Allow for a 'none' to be default to not break existing behavior Add integration test with full mongodb replicaset to show/ensure behavior
Fixes #54
Pull Request Checklist
Is this in reference to an existing issue?
General
Update Changelog following the conventions laid out here
RuboCop passes
Existing tests pass
Purpose
Pulls in local statistics, not statistics from, say, current master
Known Compatibility Issues