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

MongoDB metricbeat module #1837

Merged
merged 1 commit into from Jun 15, 2016

Conversation

Projects
None yet
2 participants
@tsg
Copy link
Collaborator

commented Jun 13, 2016

  • Contains the status metricset, reporting parts of the db.serverStatus()
    output. The locks and metrics sections are not yet reported.
  • Includes docs, basic integration tests, and basic system tests

@tsg tsg added the review label Jun 13, 2016

- module: mongodb
metricsets: ["status"]
enabled: true
period: 1s

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 13, 2016

Collaborator

Should we make the default 10s?

@@ -34,7 +34,8 @@ List of standardised words and units across all metricsets. On the left are the
* pct: precentage
* request: req
* sec: seconds, second, s
*
* ms: millisecond, millis
* mb: megabytes

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 13, 2016

Collaborator

one more we could add here that I spotted in your code and like is msg for message.

various phases of journaling in the last journal group commit
interval.
fields:
- name: dt.ms

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 13, 2016

Collaborator

What is dt standing for?

This comment has been minimized.

Copy link
@tsg

tsg Jun 15, 2016

Author Collaborator

Hmm, not sure, I think "delta t". I could expand it to delta_t, not sure if that would be better.

event := common.MapStr{
"version": mustBeString("version", status, errs),
"uptime": common.MapStr{
"ms": mustBeInteger("uptimeMillis", status, errs),

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 13, 2016

Collaborator

Interesting approach with the errs. So far I used to helper.ToInt( functions etc.

This comment has been minimized.

Copy link
@tsg

tsg Jun 13, 2016

Author Collaborator

The reason I couldn't use the ToInt stuff is that the stuff from mongo is already parsed in a map[string]interface{} so no conversions from strings are required. Very similar otherwise, though.

About the errs, I wanted to have the key missing in case of errors, rather than filled with the zero value.

"local_time": mustBeTime("localTime", status, errs),
"write_backs_queued": mustBeBool("writeBacksQueued", status, errs),
}
removeErroredKeys(event, errs)

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 13, 2016

Collaborator

The current approach was to set empty / default values for non existing values to not "break" dashboards / queries because of missing values. But this approach could be even more powerful and better cross version compatible.

This comment has been minimized.

Copy link
@tsg

tsg Jun 13, 2016

Author Collaborator

Ok, I see, it's debatable what's best, but with Rhythm I think we'll generally want missing values in case of errors.

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 14, 2016

Collaborator

By "missing values" you mean missing keys, so your approach?

This comment has been minimized.

Copy link
@tsg

tsg Jun 15, 2016

Author Collaborator

Right, IMO kibana does a relatively good job for when there are no keys. I would be more worried if we fill it with zeros, as that might hide real issues and we lose users' trust.

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 15, 2016

Collaborator

Agree. Lets try then to get the other metricsets in line here.

@@ -34,6 +37,9 @@ kibana:
apache:
build: ${PWD}/module/apache/_beat

mongodb:
image: mongo:3.0

This comment has been minimized.

Copy link
@ruflin

ruflin Jun 13, 2016

Collaborator

Could we add the tested version somewhere to the docs (see other modules).

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2016

LGTM. It is interesting that you introduced for most of the fields the "type" like ms or mb. So far I left it out most of the time except it was already part of the name. I was thinking to do it in the templates but this feels very natural and makes it easy to read.

@ruflin ruflin added the Metricbeat label Jun 14, 2016

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2016

@tsg Seems like Jenkins hit a flaky test?

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2016

jenkins, retest it

MongoDB metricbeat module
* Contains the status metricset, reporting parts of the `db.serverStatus()`
  output. The locks and metrics sections are not yet reported.
* Includes docs, basic integration tests, and  basic system tests

@tsg tsg force-pushed the tsg:metricbeat_mongo branch from 7bbeba9 to 6450d7f Jun 15, 2016

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 15, 2016

@ruflin i addressed most comments and rebased to master to make sure the _beat to _meta rename happens correctly. To make the rebase easier I squashed to one commit.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2016

jenkins, test it

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2016

LGTM

@ruflin ruflin merged commit 66e4d05 into elastic:master Jun 15, 2016

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

medcl added a commit to medcl/beats that referenced this pull request Aug 17, 2016

MongoDB metricbeat module (elastic#1837)
* Contains the status metricset, reporting parts of the `db.serverStatus()`
  output. The locks and metrics sections are not yet reported.
* Includes docs, basic integration tests, and  basic system tests

@tsg tsg deleted the tsg:metricbeat_mongo branch Aug 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.