Add ceph osd_df to metricbeat #5606

elaron · 2017-11-16T03:54:51Z

osd_df will record each osd node's disk usage for ceph cluster.

elasticmachine · 2017-11-16T03:54:52Z

Can one of the admins verify this patch?

houndci-bot · 2017-11-16T03:54:56Z

metricbeat/module/ceph/osd_df/osd_df_test.go

@@ -0,0 +1,63 @@
+package osd_df


don't use an underscore in package name

houndci-bot · 2017-11-16T03:54:56Z

metricbeat/module/ceph/osd_df/osd_df_integration_test.go

@@ -0,0 +1,48 @@
+package osd_df


don't use an underscore in package name

houndci-bot · 2017-11-16T03:54:56Z

metricbeat/module/ceph/osd_df/osd_df.go

+	}, nil
+}
+
+func (m *MetricSet) Fetch() ([]common.MapStr, error) {


exported method MetricSet.Fetch should have comment or be unexported

houndci-bot · 2017-11-16T03:54:56Z

metricbeat/module/ceph/osd_df/osd_df.go

+	*helper.HTTP
+}
+
+func New(base mb.BaseMetricSet) (mb.MetricSet, error) {


exported function New should have comment or be unexported

houndci-bot · 2017-11-16T03:54:56Z

metricbeat/module/ceph/osd_df/osd_df.go

+	}
+}
+
+type MetricSet struct {


exported type MetricSet should have comment or be unexported

houndci-bot · 2017-11-16T03:54:56Z

metricbeat/module/ceph/osd_df/osd_df.go

@@ -0,0 +1,58 @@
+package osd_df


don't use an underscore in package name

houndci-bot · 2017-11-16T03:54:56Z

metricbeat/module/ceph/osd_df/data.go

+	Nodes []Node `json:"nodes"`
+}
+
+type OsdDfRequest struct {


exported type OsdDfRequest should have comment or be unexported

houndci-bot · 2017-11-16T03:54:57Z

metricbeat/module/ceph/osd_df/data.go

+	DeviceClass string `json:"device_class"`
+}
+
+type Output struct {


exported type Output should have comment or be unexported

houndci-bot · 2017-11-16T03:54:57Z

metricbeat/module/ceph/osd_df/data.go

+	"github.com/elastic/beats/libbeat/logp"
+)
+
+type Node struct {


exported type Node should have comment or be unexported

houndci-bot · 2017-11-16T03:54:57Z

metricbeat/module/ceph/osd_df/data.go

@@ -0,0 +1,61 @@
+package osd_df


don't use an underscore in package name

ruflin

Thanks a lot for this contribution. It is great to see contributions of this quality with tests. I left a few minor comments. For the df -> filesystem comment lets discuss first before you change anything. Not really sure about this one.

ruflin · 2017-11-23T04:45:14Z

metricbeat/docs/fields.asciidoc

@@ -1192,6 +1192,83 @@ type: long
 Last updated


+[float]
+== osd_df fields


One thing that popped up in my head is that we have here df and we have system.filesystem with similar info. Don't know if osd_filesystem would make sense here (even though pretty long).

Actually the function of osd df in ceph is very similar with *system.filesystem * in linux. It shows osds' weight, reweight, size, used, avai and used pct information. Naming this new metricbet as osd_df is because it's a standard command in Ceph, and I consider that it will be much more familer to ceph users. For more about the command's detials:
http://docs.ceph.com/docs/master/man/8/ceph/

We had a similar discussion with the system module. Most sysadmins will be used to run the df command. But someone consuming the data is more looking for information about the filesystem which could be a different person. TBH don't know which one is better. I'm fine moving forward with df.

ruflin · 2017-11-23T04:46:52Z

metricbeat/docs/fields.asciidoc

+
+
+[float]
+=== `ceph.osd_df.total_byte`


This should probably be ceph.osd_df.total.bytes. have a look at the naming conventions: https://www.elastic.co/guide/en/beats/devguide/current/event-conventions.html Same applies to the fields below.

ok, I will fix it.

ruflin · 2017-11-23T04:47:39Z

metricbeat/docs/fields.asciidoc

+
+
+[float]
+=== `ceph.osd_df.avail_byte`


available.bytes: We prefer no abbreviations if not in the list.

ruflin · 2017-11-23T04:48:45Z

metricbeat/docs/fields.asciidoc

+
+
+[float]
+=== `ceph.osd_df.used_pct`


used.pct and you can use format: percent so Kibana nows it's a percentage.

The type you can set to type: scaled_float as this should be enough for percentages (normally)

ruflin · 2017-11-23T04:49:41Z

metricbeat/module/ceph/osd_df/_meta/fields.yml

@@ -0,0 +1,40 @@
+- name: osd_df


Left the comments in the fields.asciidoc, but they should obviously go here.

I get it. :)

ruflin · 2017-11-23T04:52:11Z

metricbeat/module/ceph/osd_df/data.go

+	//osd node list
+	events := []common.MapStr{}
+	for _, node := range nodeList {
+		nodeInfo := common.MapStr{}


you could do here

nodeInfo := common.MapStr{ "id": node.ID, "name": node.Name, ...

I think it would make the code a bit nicer (exact same outcome)

Good suggestion, your code looks much better. I will refine mine.

ruflin · 2017-11-23T04:52:36Z

metricbeat/module/ceph/osd_df/osd_df.go

+}
+
+func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
+	cfgwarn.Beta("The ceph osd_df metricset is beta")


I suggest we make this first experimental (as we do with all new metricsets first)

Ok, I will change it to experimental metricbet.

elaron · 2017-11-23T08:08:56Z

It's weird, when I cd beats/metricbeat, and run make update, the metricbeat/docs/modules/ceph/osd_df.asciidoc file does not update automatically. And continuous-integration/travis-ci/pr build fail.

ruflin · 2017-11-26T22:33:44Z

That is strange. Can you run make clean and then run make update again? Make sure that if you run make status all files are added.

elaron · 2017-11-27T02:33:32Z

I run make clean , and then make update.
But asciidoc file cannot update automatically.

My python version is:
[root@centos7gui metricbeat]# python
Python 2.7.12 (default, Nov 16 2017, 11:01:26)

and virtualenv has been installed:
[root@centos7gui metricbeat]# virtualenv
You must provide a DEST_DIR
Usage: virtualenv [OPTIONS] DEST_DIR

Is there any log than I can see to check what is going wrong while executing make update command?

ruflin · 2017-11-27T03:10:48Z

Hm, that all looks good. Let me pull your branch and see what happens. In general make update should tell you what went wrong.

ruflin · 2017-11-27T03:13:16Z

metricbeat/module/ceph/osd_df/_meta/fields.yml

@@ -0,0 +1,41 @@
+- name: osd_df
+  type: group


It's a new feature I just added recently. Could you add release: experimental here? Like this you don't have to add it manually to the docs.

Ok, I will add it.

ruflin · 2017-11-27T03:13:59Z

metricbeat/module/ceph/osd_df/_meta/docs.asciidoc

@@ -0,0 +1,3 @@
+=== Ceph cluster_health metricset


Also this title can now be removed with our new script. Sorry missed that previously but realised it now that i checked out the branch locally.

ruflin · 2017-11-27T03:14:46Z

Just did a pull and it gives me a diff for the asciidoc file. Perhaps what I posted above fixes it?

ruflin · 2017-11-27T03:37:44Z

@elaron Can you run make update and paste the output here?

elaron · 2017-11-27T03:43:02Z

OK, this is the output of make clean and make update:
[root@centos7gui metricbeat]# make clean
[root@centos7gui metricbeat]# make update
Using base prefix '/usr/local'
New python executable in /root/Projects/go/src/github.com/elastic/beats/metricbeat/build/python-env/bin/python3.5
Also creating executable in /root/Projects/go/src/github.com/elastic/beats/metricbeat/build/python-env/bin/python
Installing setuptools, pip, wheel...done.
Updating generated files for metricbeat
make[1]: 进入目录“/root/Projects/go/src/github.com/elastic/beats/libbeat”
make[1]: 离开目录“/root/Projects/go/src/github.com/elastic/beats/libbeat”
-- The index pattern was created under /root/Projects/go/src/github.com/elastic/beats/metricbeat/_meta/kibana/5.x/index-pattern/metricbeat.json
-- The index pattern was created under /root/Projects/go/src/github.com/elastic/beats/metricbeat/_meta/kibana/default/index-pattern/metricbeat.json
[root@centos7gui metricbeat]#

ruflin · 2017-11-27T04:39:27Z

Hm, that looks exactly like what I would expect. What does git status show? Otherwise perhaps run make clean and make update in the beats directory and check again git status, this will take a bit of time.

elaron · 2017-11-27T07:03:01Z

git status shows that the osd_df branch is clean.
After running make clean and make update in beats directory, it shows that packetbeat/docs/fields.asciidoc has been updated.

[root@centos7gui metricbeat]# git status
# 位于分支 osd_df
无文件要提交，干净的工作区

[root@centos7gui metricbeat]# cd ../
[root@centos7gui beats]# make clean;make update;
//.... takes a while ...

[root@centos7gui beats]# git status
# 位于分支 osd_df
# 尚未暂存以备提交的变更：
# （使用 "git add ..." 更新要提交的内容）
# （使用 "git checkout -- ..." 丢弃工作区的改动）
#
# 修改： packetbeat/docs/fields.asciidoc
#
修改尚未加入提交（使用 "git add" 和/或 "git commit -a"）

ruflin · 2017-11-27T22:30:49Z

Could you show the git diff output? Otherwise what do you think if I open a PR against your branch with the update so you can merge it in and we can get the PR in. Not sure what is happening here.

elaron · 2017-11-28T01:19:42Z

Yes, please open a PR against my branch with the updates. I will merge them in. Thanks a lot.

ruflin · 2017-11-28T23:50:56Z

@elaron Ok, I fetched your branch and the issue seems to be that it's based on an older version of master. If I rebase on top of our most recent master, the update works as expected. We have now 2 options:

You rebase on master, run make update and force push
I force push to your branch :-(

We could also merge master in if you prefer that over rebasing.

Which path should we go?

elaron · 2017-11-29T08:13:38Z

@ruflin , I rebase on master, then run make update and force push. Now all checks have passed.

Should I update the CHANGELOG.asciidoc?

ruflin · 2017-11-30T03:02:43Z

@elaron Awesome, that looks very green. Yes can you push one more commit with the CHANGELOG entry?

elaron · 2017-11-30T06:01:59Z

@ruflin CHANGELOG has been update.
But it's strange that the CI failed, because some kinds of logstash test case error?
"FAIL: logstash node_stats metricset test"

ruflin · 2017-11-30T23:21:53Z

jenkins, test it

ruflin · 2017-11-30T23:24:37Z

@elaron Don't worry about the node_stats test. It's not related to this PR.

elaron · 2017-12-01T01:12:38Z

OK, thanks a lot for your help. :)

ruflin · 2017-12-01T01:32:40Z

@elaron Merged. Thanks a lot for your contribution.

elaron · 2017-12-01T01:57:47Z

It's my pleasure.

elaron added 2 commits November 16, 2017 11:04

Add ceph osd_df to metricbeat

0b18029

update asciidocs

7b482c9

houndci-bot reviewed Nov 16, 2017

View reviewed changes

ruflin added Metricbeat Metricbeat review labels Nov 16, 2017

ruflin reviewed Nov 23, 2017

View reviewed changes

refine code and fields's name

929de4d

update asciidoc file

c38bd85

ruflin reviewed Nov 27, 2017

View reviewed changes

add experimental flag to fields.yml

2a973c3

elaron added 2 commits November 29, 2017 14:08

Merge remote-tracking branch 'upstream/master'

400672c

update osd_df.asciidoc

3010821

update CHANGELOG

70dbeea

ruflin approved these changes Nov 30, 2017

View reviewed changes

ruflin merged commit 9bb8617 into elastic:master Dec 1, 2017

elaron deleted the osd_df branch December 1, 2017 01:57

ruflin mentioned this pull request Dec 3, 2017

Update modules list for Metricbeat #5794

Merged



		[float]
		=== `ceph.osd_df.total_byte`



		[float]
		=== `ceph.osd_df.avail_byte`



		[float]
		=== `ceph.osd_df.used_pct`

Add ceph osd_df to metricbeat #5606

Add ceph osd_df to metricbeat #5606

Conversation

elaron commented Nov 16, 2017

elasticmachine commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elaron commented Nov 23, 2017 • edited Loading

ruflin commented Nov 26, 2017

elaron commented Nov 27, 2017

ruflin commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Nov 27, 2017

ruflin commented Nov 27, 2017

elaron commented Nov 27, 2017

ruflin commented Nov 27, 2017

elaron commented Nov 27, 2017 • edited Loading

ruflin commented Nov 27, 2017

elaron commented Nov 28, 2017

ruflin commented Nov 28, 2017

elaron commented Nov 29, 2017 • edited Loading

ruflin commented Nov 30, 2017

elaron commented Nov 30, 2017 • edited Loading

ruflin commented Nov 30, 2017

ruflin commented Nov 30, 2017

elaron commented Dec 1, 2017

ruflin commented Dec 1, 2017

elaron commented Dec 1, 2017 • edited Loading

elaron commented Nov 23, 2017 •

edited

Loading

elaron commented Nov 27, 2017 •

edited

Loading

elaron commented Nov 29, 2017 •

edited

Loading

elaron commented Nov 30, 2017 •

edited

Loading

elaron commented Dec 1, 2017 •

edited

Loading