Skip to content
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

Replicaset status fix #55

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

naemono
Copy link
Contributor

@naemono naemono commented Oct 2, 2017

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

@naemono
Copy link
Contributor Author

naemono commented Oct 2, 2017

Oh yeah, here's the before/after
https://gist.github.com/naemono/d9a7ef94b030d5e06b4b9dc378ba89aa

@majormoses majormoses self-requested a review October 3, 2017 21:02
@majormoses
Copy link
Member

@naemono is there a scenario where you might want that behavior? If so maybe an option to toggle it would make sense.

@majormoses
Copy link
Member

@naemono I rebased for you, any chance you could validate if my question needs any consideration?

@majormoses
Copy link
Member

@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

@eberkut
Copy link

eberkut commented Feb 19, 2018

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.

@naemono
Copy link
Contributor Author

naemono commented Feb 19, 2018

@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.

@majormoses
Copy link
Member

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 ### Breaking Changes header in the changelog.

@naemono
Copy link
Contributor Author

naemono commented Feb 19, 2018

@majormoses I'll add as an argument. No problem.

@majormoses
Copy link
Member

@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.

@majormoses majormoses force-pushed the replicaset-status-fix branch from 1a83c23 to 1534adb Compare August 2, 2018 16:41
@majormoses
Copy link
Member

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.

@majormoses
Copy link
Member

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 majormoses force-pushed the replicaset-status-fix branch from 83d132e to 4ec51e5 Compare August 7, 2018 00:39
Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Though really my review should not really count. We minimally need a test before merging.

- 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 <[email protected]>
@majormoses majormoses force-pushed the replicaset-status-fix branch from 4ec51e5 to c9ed3f8 Compare August 7, 2018 00:43
Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the following patches will hopefully resolve #54 (comment)

bin/metrics-mongodb-replication.rb Outdated Show resolved Hide resolved
bin/metrics-mongodb.rb Outdated Show resolved Hide resolved
majormoses and others added 3 commits April 24, 2021 12:33
Allow for a 'none' to be default to not break existing behavior
Add integration test with full mongodb replicaset to show/ensure behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants