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

Add repl_oplog_window_s metric to mongodb input #3964

Merged
merged 1 commit into from
Apr 6, 2018

Conversation

subuk
Copy link
Contributor

@subuk subuk commented Apr 3, 2018

Hello!

Here is new metric named oplog_timediff.

References:
https://www.mongodb.com/blog/post/replica-set-health-is-more-than-just-replication
https://github.com/DataDog/integrations-core/blob/master/mongo/datadog_checks/mongo/mongo.py#L972

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@subuk subuk force-pushed the master branch 2 times, most recently from 64913c1 to dfb269c Compare April 3, 2018 09:27
@danielnelson danielnelson added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin area/mongodb labels Apr 3, 2018
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Thanks @subuk, this is a great addition, I do have a couple suggestions:

@@ -57,6 +57,7 @@ and create a single measurement containing values e.g.
* ttl_passes_per_sec
* repl_lag
* jumbo_chunks (only if mongos or mongo config)
* oplog_timediff
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should call the field repl_oplog_window_s. This more closely matches the linked documentation from mongodb, while still using the same repl prefix as the other replication metrics and also includes the units. I sort of dislike _s because its not immediately clear what it means, but it matches more in style with existing _ns readings.

Copy link
Contributor Author

@subuk subuk Apr 4, 2018

Choose a reason for hiding this comment

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

Okay, I will rename oplog_timediff to repl_oplog_window_s and move it closer to other repl_* metrics in code.

localdb := s.Session.DB("local")
oplog_collection_name := ""

collection_names, err := localdb.CollectionNames()
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like instead of checking for the existence of this items we should just try to get the data and if it's not there then we don't report anything. This will result in less network calls and checking doesn't guarantee the data will be there later anyway due to possible changes.

How about we just try to get the entries for oplog.rs and if that fails we can try oplog.$main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it makes sense, will be fixed.

@subuk subuk changed the title Add oplog timediff metric to mongodb input Add repl_oplog_window_s metric to mongodb input Apr 4, 2018
@subuk
Copy link
Contributor Author

subuk commented Apr 4, 2018

Fixed. Metric renamed to repl_oplog_window_s, collection list query removed.

@danielnelson danielnelson merged commit 01ede2e into influxdata:master Apr 6, 2018
@danielnelson danielnelson added this to the 1.7.0 milestone Apr 6, 2018
@danielnelson
Copy link
Contributor

Thank you, this will be in version 1.7.0

@danielnelson
Copy link
Contributor

@subuk I renamed this repl_oplog_window_sec as I noticed that this matches the style of the other fields in this plugin. 1890efb

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/mongodb feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants