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.

Sign up for GitHub

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

Jump to bottom

Fix MongoDB dbstats fields mapping #4025

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Fix MongoDB dbstats fields mapping #4025

merged 1 commit into from
Apr 21, 2017

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Apr 17, 2017

Fix mongodb dbstats mapping to avoid errors like this:

WARN Can not index event (status=400): {"type":"mapper_parsing_exception","reason":"object mapping for [mongodb.dbstats.avg_obj_size] tried to parse field [avg_obj_size] as object, but found a concrete value"}

closes #3989

@exekias exekias force-pushed the fix-mongodb-fields branch from 537d484 to 42fe544 Compare April 17, 2017 16:07
ruflin
ruflin suggested changes Apr 17, 2017
View reviewed changes
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

The dbstats metricset is still experimental. We should change the fields in the mapping and not the documentation. Means we should add .bytes etc. as it is according to our convention. This is a BC break but with experimental metricsets that is fine in my opinion.

@exekias exekias force-pushed the fix-mongodb-fields branch from 42fe544 to a83d718 Compare April 17, 2017 18:03
@exekias exekias added the in progress Pull request is currently in progress. label Apr 17, 2017
@exekias exekias force-pushed the fix-mongodb-fields branch from a83d718 to 7891e8f Compare April 17, 2017 18:32
@exekias exekias removed the in progress Pull request is currently in progress. label Apr 17, 2017
ruflin
ruflin reviewed Apr 18, 2017
View reviewed changes
metricbeat/module/mongodb/dbstats/data.go Outdated
"db": c.Str("db"),
"collections": c.Int("collections"),
"objects": c.Int("objects"),
"avg_obj_size.bytes": c.Int("avgObjSize"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make it incompatible with elasticsearch 2.x. You must use s.Object{ ... }  to add the nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't know that one, thanks!

@exekias exekias force-pushed the fix-mongodb-fields branch from 7891e8f to 73b3fc4 Compare April 19, 2017 07:36
@monicasarbu
Copy link
Contributor

@exekias Some tests are failing. Can you please have a look?

ruflin
ruflin suggested changes Apr 20, 2017
View reviewed changes
metricbeat/module/mongodb/dbstats/data.go Outdated
"num": c.Int("num"),
"size": c.Int("size"),
"num": c.Int("num"),
"size.bytes": c.Int("size"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Still one with .bytes

metricbeat/module/mongodb/dbstats/dbstats_integration_test.go Outdated
@@ -30,13 +30,13 @@ func TestFetch(t *testing.T) {
objects := event["objects"].(int64)
assert.True(t, objects > 0)

avgObjSize := event["avg_obj_size"].(int64)
avgObjSize := event["avg_obj_size.bytes"].(int64)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that is not going to work, as it is a nested object.

metricbeat/module/mongodb/dbstats/_meta/data.json Outdated
@@ -12,15 +12,15 @@
},
"mongodb": {
"dbstats": {
"avg_obj_size": 59,
"avg_obj_size.bytes": 59,
Copy link
Contributor

Choose a reason for hiding this comment

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

You should update this one with make generation-json.

@exekias exekias force-pushed the fix-mongodb-fields branch from 73b3fc4 to 6b0b077 Compare April 20, 2017 16:04
@ruflin ruflin merged commit fb92dee into elastic:master Apr 21, 2017
@ashwinboinala
Copy link

ashwinboinala commented May 4, 2017 •
edited
Loading

@ruflin How do I make changes to my existing metric beat installation, please advise.

@ruflin
Copy link
Contributor

ruflin commented May 9, 2017

@ashwinboinala There are two options. Either you adjust the mapping manually and overwrite the one you have or you switch to the nightly build found here and let metricbeat overwrite your template: https://beats-nightlies.s3.amazonaws.com/index.html?prefix=metricbeat/

@exekias Should we backport this to 5.x?

@exekias
Copy link
Contributor Author

exekias commented May 9, 2017

@ruflin uhm probably yes, let me open a PR

@exekias exekias mentioned this pull request May 9, 2017
Merged
@ashwinboinala
Copy link

@ruflin thanks for your response.
I found another issue with mongo module, below is my connection string.
hosts: ["mongodb://username:password@servername:27017/dbname"]
In the dashboard, I see hostname as username:password which is metric.hostname
Is it expected behavior or am I doing anything wrong, below is my metricbeat config.
Please advise

  • module: mongodb
    metricsets: ["dbstats", "status"]
    enabled: true
    period: 10s
    hosts: ["mongodb://username:password@servername:27017/dbname"]

-Ashwin

@ruflin
Copy link
Contributor

ruflin commented May 11, 2017

@ashwinboinala This is not expected. It sounds like something does not work well with the host parser. Could you open a new Github issue for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin requested changes

Assignees
No one assigned
Labels
bug Metricbeat Metricbeat review v6.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some MongoDB fields are wrong
4 participants
@exekias @monicasarbu @ashwinboinala @ruflin