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

Adds support for WiredTiger Engine #61

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

Conversation

parinapatel
Copy link

@parinapatel parinapatel commented Feb 14, 2018

Pull Request Checklist

Is this in reference to an existing issue?

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets

  • Binstubs are created if needed

  • RuboCop passes

  • Existing tests pass

New Plugins

  • Tests

  • Add the plugin to the README

  • Does it have a complete header as outlined here

Purpose

Known Compatibility Issues

@majormoses
Copy link
Member

do you need any help with working through the rubocop issues?

@majormoses
Copy link
Member

Looks like we need a reabse. Let me know if you need any help working through CI issues.

@majormoses
Copy link
Member

@parin921996 any chance you plan on coming back to this?

@parinapatel
Copy link
Author

@majormoses I have kinda lost track of this one , i will try to fix it by this weekend .

@parinapatel
Copy link
Author

@majormoses Hey I have Fixed the issue , I will create new PR for any new metrics introduced in mongo 3.4 and above.

@majormoses
Copy link
Member

Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly.

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.

Overall I think this looks really good just a couple of improvements and some nitpicks and we are good to go.

@@ -5,6 +5,10 @@ This CHANGELOG follows the format located [here](https://github.com/sensu-plugin

## [Unreleased]

## [2.0.3] - 2018-09-14
Copy link
Member

Choose a reason for hiding this comment

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

In general the versioning and dating are left to the maintainers for several reasons:

  • we may disagree on the impact of the change and therefore how its versioned (in this case it would be a minor so it would be 2.1.0) per our versioning guidelines
  • we date the release rather then when the contribution was submitted, PR review and acceptance has no SLA and is not guaranteed to be merged within the same day
  • PRs are not a FIFO queue, as such versioning them prior to acceptance leads to needing to ask people to rebase more often

@@ -5,6 +5,10 @@ This CHANGELOG follows the format located [here](https://github.com/sensu-plugin

## [Unreleased]

## [2.0.3] - 2018-09-14
### New Feature
Copy link
Member

Choose a reason for hiding this comment

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

When adding a new feature we use the ### Added header

Copy link
Author

Choose a reason for hiding this comment

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

Feel free to update it.

wiredtiger[key1].each do |key2, value2|
if value2.is_a? Hash
wiredtiger[key1][key2].each do |key3, value3|
server_metrics['wiredTiger.' + key1.gsub(/(,|\s|-|\()+/, '_').tr(')', '') + '.' +
Copy link
Member

Choose a reason for hiding this comment

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

it looks like we are repeating the same logic/regex here can we make a function and call that instead I think it would make the code a lot more readable. Maybe something along the lines of wiretiger_metric_(converter|builder|formatter)

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking to write json flattener but current use case was quite specific so I quickly submit this PR. But I kinda liked the idea.

@majormoses
Copy link
Member

@parin921996 any chance you have some systems to test #55 with? I'd really like to see both of these over the line.

@parinapatel
Copy link
Author

@parin921996 any chance you have some systems to test #55 with? I'd really like to see both of these over the line.

I dont have any replica set running locally..

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.

2 participants