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 docker diskio stats on Windows #8126

Merged
merged 5 commits into from
Aug 29, 2018

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Aug 28, 2018

Docker stats use a different data structure for blkio stats in Windows, the main difference is that in posix systems (Linux at least) stats are separated by device by Major/Minor (lists of BlkioStatsEntry), and in Windows they are directly summarized by reads and writes, by operations and volume (StorageStats).

This change counts the metrics of both data structures and aggregates the result.

Fixes #6815

@jsoriano jsoriano added the in progress Pull request is currently in progress. label Aug 28, 2018
@jsoriano
Copy link
Member Author

I'm thinking now that it'd be better if the module collected metrics from all OSs instead of having two different metrics, as one metricbeat can be collecting from a docker server in a different OS.

@jsoriano jsoriano removed the review label Aug 28, 2018
type BlkioRaw struct {
Time time.Time
reads uint64
writes uint64
totals uint64
}

func (s *BlkioRaw) Add(o *BlkioRaw) {

Choose a reason for hiding this comment

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

exported method BlkioRaw.Add should have comment or be unexported

@@ -36,13 +36,28 @@ type BlkioStats struct {
servicedBytes BlkioRaw
}

func (s *BlkioStats) Add(o *BlkioStats) {

Choose a reason for hiding this comment

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

exported method BlkioStats.Add should have comment or be unexported

@jsoriano jsoriano added review and removed in progress Pull request is currently in progress. labels Aug 28, 2018
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for taking this one!

Copy link
Member

@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.

Sounds like we should test docker also on Windows in the future ...

@ruflin
Copy link
Member

ruflin commented Aug 29, 2018

@jsoriano Does this need a backport label?

@jsoriano
Copy link
Member Author

jsoriano commented Aug 29, 2018

Yes, I will backport it for 6.x

@jsoriano jsoriano added the needs_backport PR is waiting to be backported to other branches. label Aug 29, 2018
@jsoriano jsoriano merged commit 63f25a4 into elastic:master Aug 29, 2018
@jsoriano jsoriano deleted the docker-diskio-windows branch August 29, 2018 09:33
jsoriano added a commit to jsoriano/beats that referenced this pull request Aug 29, 2018
Docker stats use a different data structure for blkio stats in Windows, the main difference is that in posix systems (Linux at least) stats are separated by device by Major/Minor (lists of BlkioStatsEntry), and in Windows they are directly summarized by reads and writes, by operations and volume (StorageStats).

This change counts the metrics of both data structures and aggregates the result.

Fixes elastic#6815

(cherry picked from commit 63f25a4)
@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Aug 29, 2018
jsoriano added a commit that referenced this pull request Aug 29, 2018
Docker stats use a different data structure for blkio stats in Windows, the main difference is that in posix systems (Linux at least) stats are separated by device by Major/Minor (lists of BlkioStatsEntry), and in Windows they are directly summarized by reads and writes, by operations and volume (StorageStats).

This change counts the metrics of both data structures and aggregates the result.

Fixes #6815

(cherry picked from commit 63f25a4)
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