Metricbeat: Improvements in docker diskio metricset #6701

jsoriano · 2018-03-29T18:41:20Z

This PR continues with #6608 in the aim to provide docker metrics more consistent with docker stats. Currently we report for disk I/O the number of operations per second.

This PR adds accumulated I/O stats per number of operations and per data volume since the container started, as docker stats does. I think this information is more valuable, as it permits to have further aggregations when visualizing it. It also addresses some other issues in the metricset.

Fields in events have been reorganized and old ones have been marked as deprecated.

We could also add some extra functionality, as obtaining metrics also on Windows hosts (as is this module now it would only work with docker daemons running on Linux and not sure about Mac). In Linux we could also collect metrics per block device.

Add accumulated I/O stats to diskio metricset
Fix diskio metricset when docker reads from multiple devices
Some naming and documentation fixes
Fix memory leak in blkio service (last seen stats were being stored, but never removed)
More tests

To follow-up in other issues:

~~Group by device in Linux?~~ To be done if requested in the future
Support for Windows storage stats? Follow-up issue Support docker diskio stats in Windows #6815

houndci-bot · 2018-03-29T18:41:29Z

metricbeat/module/docker/diskio/helper.go

+	BlkioStatsPerContainer map[string]BlkioRaw
+}
+
+func NewBlkioService() *BlkioService {


exported function NewBlkioService should have comment or be unexported

houndci-bot · 2018-03-29T18:41:30Z

metricbeat/module/docker/diskio/helper.go

@@ -29,14 +26,20 @@ type BlkioRaw struct {
 	totals uint64
 }

-type BLkioService struct {
-	BlkioSTatsPerContainer map[string]BlkioRaw
+type BlkioService struct {


exported type BlkioService should have comment or be unexported

jsoriano · 2018-03-29T18:44:57Z

metricbeat/module/docker/diskio/data.go

@@ -21,6 +21,20 @@ func eventMapping(stats *BlkioStats) common.MapStr {
 		"reads":  stats.reads,
 		"writes": stats.writes,
 		"total":  stats.totals,
+		"serviced": common.MapStr{


I follow this structure for backwards compatibility, but current metrics should probably be at the same level as these ones.

the only conflict we would have would be for total as the other ones are plural. We could use summary instead of total? Not the nice thing :-( Other ideas on how we could work around this?

Umm, this could be an approach, yes, I'll update the PR with this.

Added a new commit with this approach 2a0306b

jsoriano · 2018-03-29T18:46:22Z

metricbeat/module/docker/diskio/_meta/fields.yml

@@ -7,12 +7,56 @@
    - name: reads
      type: scaled_float
      description: >
-        Number of reads.
+        Number of current reads per second


I think that calculating the operations or bytes/second should be done at visualization time, given that we now will have the accumulated values, but removing this would be a breaking change. Opinions?

Removing it would be a breaking change. We should have a "deprecated" field in fields.yml so we don't forget to remove such fields and it's also marked in the docs.

In general I prefer counters over gauges exactly because it allows to do the calculations over different time frames on the ES side. This case is a bit special as we summarise it and not return what we get from the API. I would hope the API gives us counters which is not the case here and does some precalculations.

+1 to the deprecated field, I think we need something like this.

The API exports it as gauges, but I'd say it doesn't do any calculation for block I/O. The problem I see with counters and this kind of metrics is that you have to be very careful about how and when you collect them.

Should we call it just deprecated: true in the fields.yml?

jsoriano · 2018-03-29T18:49:32Z

metricbeat/module/docker/diskio/helper.go

@@ -45,56 +48,62 @@ func (io *BLkioService) getBlkioStatsList(rawStats []docker.Stat, dedot bool) []
 	return formattedStats
 }

-func (io *BLkioService) getBlkioStats(myRawStat *docker.Stat, dedot bool) BlkioStats {
+// TODO: User StorageStats instead of BlkioStats on Windows


According to docker stats API documentation, BlkioStats is only populated in Linux, for Windows there is another structure under StorageStats.

Should we open a follow up github issue for that?

The biggest issue about Windows at the moment is testing it, maybe better to leave the TODO here and open a follow-up issue, yes.

jsoriano · 2018-03-29T18:50:53Z

metricbeat/module/docker/diskio/helper.go

 	stats := BlkioRaw{
 		Time:   time,
 		reads:  0,
 		writes: 0,
 		totals: 0,
 	}
+
 	for _, myEntry := range blkioEntry {


Entries are per operation type, but also per Major/Minor (per device), so they should be summed by type. We could also decide to send metrics per device. Opinions?

If we send metrics per device, would that be an additional metricset? I would assume that each device is on event? Or how would the output look like?

What do you mean by Major/Minor?

These blkio entries are defined as:

// BlkioStatEntry is one small entity to store a piece of Blkio stats // Not used on Windows. type BlkioStatEntry struct { Major uint64 `json:"major"` Minor uint64 `json:"minor"` Op string `json:"op"` Value uint64 `json:"value"` }

I mean Major/Minor numbers as the identifier of a device in a posix system, in this case they identify the block device related to these stats.

So we have information per device, and we could send that, though the mapping between Major/Minor and device files, mountpoints or something useful would be tricky, and we couldn't provide the same functionality for Windows.

It seems this is only partially related to this PR so I suggest to merge this one and follow up the discussion in an other thread?

Yes, in any case I'm not sure about how to do it in a useful way with proper mapping between device ids and device files or mountpoints. We can leave it till we see someone needing it.

ruflin · 2018-03-30T10:55:25Z

metricbeat/module/docker/diskio/helper.go

@@ -45,56 +48,62 @@ func (io *BLkioService) getBlkioStatsList(rawStats []docker.Stat, dedot bool) []
 	return formattedStats
 }

-func (io *BLkioService) getBlkioStats(myRawStat *docker.Stat, dedot bool) BlkioStats {
+// TODO: User StorageStats instead of BlkioStats on Windows


Should we open a follow up github issue for that?

ruflin · 2018-03-30T10:59:00Z

metricbeat/module/docker/diskio/data.go

@@ -21,6 +21,20 @@ func eventMapping(stats *BlkioStats) common.MapStr {
 		"reads":  stats.reads,
 		"writes": stats.writes,
 		"total":  stats.totals,
+		"serviced": common.MapStr{


the only conflict we would have would be for total as the other ones are plural. We could use summary instead of total? Not the nice thing :-( Other ideas on how we could work around this?

ruflin · 2018-03-30T11:01:28Z

metricbeat/module/docker/diskio/helper.go

 	stats := BlkioRaw{
 		Time:   time,
 		reads:  0,
 		writes: 0,
 		totals: 0,
 	}
+
 	for _, myEntry := range blkioEntry {


If we send metrics per device, would that be an additional metricset? I would assume that each device is on event? Or how would the output look like?

What do you mean by Major/Minor?

ruflin · 2018-03-30T11:04:17Z

metricbeat/module/docker/diskio/_meta/fields.yml

@@ -7,12 +7,56 @@
    - name: reads
      type: scaled_float
      description: >
-        Number of reads.
+        Number of current reads per second


Removing it would be a breaking change. We should have a "deprecated" field in fields.yml so we don't forget to remove such fields and it's also marked in the docs.

In general I prefer counters over gauges exactly because it allows to do the calculations over different time frames on the ES side. This case is a bit special as we summarise it and not return what we get from the API. I would hope the API gives us counters which is not the case here and does some precalculations.

ruflin · 2018-03-30T11:04:47Z

metricbeat/module/docker/diskio/helper.go

 	for _, myEntry := range blkioEntry {
 		if myEntry.Op == "Write" {
-			stats.writes = myEntry.Value
+			stats.writes += myEntry.Value


I'm wonder if the counter we calculate here is actually correct. To we get the sum over the last 2 seconds here or do we get the value since our last request?

We only get the value of the last request. The sum is to sum the same operation on multiple devices.

jsoriano · 2018-04-09T17:46:41Z

metricbeat/module/docker/diskio/helper.go

 	if exist {
 		myBlkioStats.reads = io.getReadPs(&oldBlkioStats, &newBlkioStats)
 		myBlkioStats.writes = io.getWritePs(&oldBlkioStats, &newBlkioStats)
 		myBlkioStats.totals = io.getTotalPs(&oldBlkioStats, &newBlkioStats)
 	}

-	io.BlkioSTatsPerContainer[myRawStat.Container.ID] = newBlkioStats
+	// FIXME: Memory leak
+	io.BlkioStatsPerContainer[myRawStat.Container.ID] = newBlkioStats


I think we have a memory leak here, entries are never removed, I'll take a look to this too.

jsoriano · 2018-04-10T09:07:12Z

I have added a new test, to cover all calculations done on raw stats, there are some other tests that could be revisited, but I left them by now. Regarding Windows, I have created a follow-up issue #6815.

This would be ready for review.

ruflin

I think the data.json file should be generated to have the new values in.

ruflin · 2018-04-11T06:41:37Z

metricbeat/module/docker/diskio/_meta/fields.yml

+          description: >
+            Number of reads during the life of the container
+        - name: bytes
+          type: long


Could you add here format: bytes to kibana can make sense of it. Same is true for other bytes values.

jsoriano · 2018-04-11T08:56:25Z

data.json updated and bytes fileds formatted as bytes

ruflin

Waiting for getting back our CI builds. Seems there are some issues with Kafka.

* Add accumulated I/O stats to diskio metricset * Restructure fields in events, mark old fields as deprecated * Fix diskio metricset when docker reads from multiple devices * Some naming fixes

The metricset stores last seen stats to calculate rates per second, but they were never being removed for removed containers.

jsoriano · 2018-04-13T07:28:09Z

Failing tests not related to these changes.

jsoriano added in progress Pull request is currently in progress. module feedback needed Metricbeat Metricbeat labels Mar 29, 2018

houndci-bot reviewed Mar 29, 2018

View reviewed changes

jsoriano added the enhancement label Mar 29, 2018

jsoriano commented Mar 29, 2018

View reviewed changes

ruflin reviewed Apr 3, 2018

View reviewed changes

jsoriano force-pushed the docker-blockio branch 3 times, most recently from ea10321 to fdf7fbf Compare April 9, 2018 17:44

jsoriano commented Apr 9, 2018

View reviewed changes

jsoriano force-pushed the docker-blockio branch from 2d24fd9 to 2e3b94a Compare April 10, 2018 08:47

jsoriano mentioned this pull request Apr 10, 2018

Support docker diskio stats in Windows #6815

Closed

jsoriano added review and removed in progress Pull request is currently in progress. labels Apr 10, 2018

jsoriano force-pushed the docker-blockio branch from 2e3b94a to 48a2f7c Compare April 10, 2018 09:26

ruflin reviewed Apr 11, 2018

View reviewed changes

jsoriano force-pushed the docker-blockio branch from 48a2f7c to ca64792 Compare April 11, 2018 08:55

ruflin approved these changes Apr 11, 2018

View reviewed changes

jsoriano added 4 commits April 12, 2018 17:31

Metricbeat: Improvements in docker diskio metricset

d63c7ce

* Add accumulated I/O stats to diskio metricset * Restructure fields in events, mark old fields as deprecated * Fix diskio metricset when docker reads from multiple devices * Some naming fixes

Metricbeat: Added test for BlkioStats

7dabc8f

Metricbeat: Fix memory leak in diskio metricset of docker module

f5ebc9f

The metricset stores last seen stats to calculate rates per second, but they were never being removed for removed containers.

Metricbeat: Format fields for bytes and updated data.json

c7d798c

jsoriano force-pushed the docker-blockio branch from ca64792 to c7d798c Compare April 12, 2018 15:31

ruflin merged commit aaae0b7 into elastic:master Apr 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metricbeat: Improvements in docker diskio metricset #6701

Metricbeat: Improvements in docker diskio metricset #6701

jsoriano commented Mar 29, 2018 •

edited

Loading

houndci-bot Mar 29, 2018

houndci-bot Mar 29, 2018

jsoriano Mar 29, 2018

ruflin Mar 30, 2018

jsoriano Apr 3, 2018

jsoriano Apr 3, 2018

jsoriano Mar 29, 2018 •

edited

Loading

ruflin Mar 30, 2018

jsoriano Apr 3, 2018

ruflin Apr 4, 2018

jsoriano Apr 4, 2018

jsoriano Mar 29, 2018 •

edited

Loading

ruflin Mar 30, 2018

jsoriano Apr 3, 2018

jsoriano Mar 29, 2018 •

edited

Loading

ruflin Mar 30, 2018

jsoriano Apr 3, 2018

ruflin Apr 4, 2018

jsoriano Apr 4, 2018

ruflin Mar 30, 2018

ruflin Mar 30, 2018

ruflin Mar 30, 2018

ruflin Mar 30, 2018

ruflin Mar 30, 2018

jsoriano Apr 3, 2018

jsoriano Apr 9, 2018

jsoriano commented Apr 10, 2018

ruflin left a comment

ruflin Apr 11, 2018

jsoriano Apr 11, 2018

jsoriano commented Apr 11, 2018

ruflin left a comment

jsoriano commented Apr 13, 2018

Metricbeat: Improvements in docker diskio metricset #6701

Metricbeat: Improvements in docker diskio metricset #6701

Conversation

jsoriano commented Mar 29, 2018 • edited Loading

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

jsoriano Mar 29, 2018 • edited Loading

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

jsoriano Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Mar 29, 2018 • edited Loading

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

jsoriano commented Apr 10, 2018

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

jsoriano commented Apr 11, 2018

ruflin left a comment

Choose a reason for hiding this comment

jsoriano commented Apr 13, 2018

jsoriano commented Mar 29, 2018 •

edited

Loading

jsoriano Mar 29, 2018 •

edited

Loading

jsoriano Mar 29, 2018 •

edited

Loading

jsoriano Mar 29, 2018 •

edited

Loading