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 metrics metricset to MongoDB module #7611

Merged
merged 23 commits into from Jul 31, 2018
Merged

Add metrics metricset to MongoDB module #7611

merged 23 commits into from Jul 31, 2018

Conversation

a3dho3yn
Copy link
Contributor

The status metricset does not completely reflect db.serverStatus(). So it was requested to add a metric set to cover the metrics field.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Only some small comments, thanks for all the work in this module!

description: >
Statistics that reflect the current use and state of a running `mongod` instance
for more information, take a look at https://docs.mongodb.com/manual/reference/command/serverStatus/#serverstatus.metrics
fields:
Copy link
Member

Choose a reason for hiding this comment

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

Could you add descriptions to fields too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"replset_update_position": c.Dict("replSetUpdatePosition", commandSchema),
"server_status": c.Dict("serverStatus", commandSchema),
"update": c.Dict("update", commandSchema),
"whatsmyuri": c.Dict("whatsmyuri", commandSchema),
Copy link
Member

Choose a reason for hiding this comment

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

As we are collecting metrics about each one of the commands here, maybe we want to keep the commands as field names, even if they are camel cased, @ruflin wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tricky one. I would prefer to keep all names lower case if possible but I understand that it might confuse people that search for a specific command. at the same time I would hope autocomplete and when they type the first characters, it becomes clear.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's keep them without camel case by now, we can revisit the decision in the future if it confuses people.

return nil, err
}

data, _ := schema.Apply(result)
Copy link
Member

Choose a reason for hiding this comment

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

We could check errors after #7335

// New creates a new instance of the MetricSet
// Part of new is also setting up the configuration by processing additional
// configuration entries if needed.
func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Add an experimental warning here.

@jsoriano
Copy link
Member

jenkins, test this

@jsoriano
Copy link
Member

jenkins, test this

@a3dho3yn
Copy link
Contributor Author

a3dho3yn commented Jul 24, 2018

There is an error with test_base.Test.test_template. Is there any issue in my code?
@jsoriano

- name: attempts_to_become_secondary
type: long
- name: batches
type: groupt
Copy link
Member

Choose a reason for hiding this comment

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

@a3dho3yn typo here (it should be group), test is failing because of this.

- name: ready
type: int
- name: free
type: int
Copy link
Member

Choose a reason for hiding this comment

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

int should be long.

@jsoriano
Copy link
Member

jenkins, test this please

@@ -59,6 +59,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
*Metricbeat*

- Add metrics about cache size to memcached module {pull}7740[7740]
- Add `locks`, `global_locks`, `oplatencies` and `process` fields to `status` metricset of MongoDB module. {pull}7613[7613]
Copy link
Member

Choose a reason for hiding this comment

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

This changelog entry seems incorrect.

@@ -2,6 +2,8 @@
#metricsets:
# - dbstats
# - status
# - collstats
# - metrics
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add them to the config.reference.yml file?

@jsoriano
Copy link
Member

jenkins, test this

@jsoriano
Copy link
Member

jenkins, test this

@jsoriano jsoriano merged commit 5b7d31c into elastic:master Jul 31, 2018
jsoriano pushed a commit to jsoriano/beats that referenced this pull request Aug 20, 2018
Add a metricset to cover the metrics field included in the response of `db.serverStatus()`.

(cherry picked from commit 5b7d31c)
ruflin pushed a commit that referenced this pull request Aug 23, 2018
Add a metricset to cover the metrics field included in the response of `db.serverStatus()`.

(cherry picked from commit 5b7d31c)
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.

4 participants