Add format to stats #24987

boaz0 · 2016-07-24T21:06:04Z

- What I did
I attempted to use the formatter API to display docker stats.
closes #20973

- How I did it

Created types.ContainerStats, CStatsContext and containerStats structs
Moved stats_helper's Display logic to CStatsContext's Write

- How to verify it
docker stats --format "{{.PIDs}}"

AkihiroSuda · 2016-07-25T01:07:07Z

vendor/src/github.com/docker/engine-api/types/types.go

+	BlockRead        float64
+	BlockWrite       float64
+	PidsCurrent      uint64
+	Mu               sync.Mutex


I think engine-api/types should not have mutex

I can do that. I can embed ContainerStats in another struct which gives the ability to do mutex lock.
I will also move the changes I have done in engine-api into its repo.
Right now, I just want to show all the changes I made.

Agree with @AkihiroSuda

boaz0 · 2016-07-25T08:51:44Z

BTW, it seems like that all the failed builds aren't related to this commit.

dnephin · 2016-07-25T17:42:55Z

Test failures are related, validate-lint is failing because of the code changes

boaz0 · 2016-07-25T20:45:05Z

@dnephin thanks for noticing. I'll fix it + rebase and push the changes soon.

boaz0 · 2016-07-26T12:34:39Z

I wonder what went wrong this time 😞
It seems like it is related to a time out exception when running the search-test in docker-py.

thaJeztah · 2016-07-26T13:11:14Z

@ripcurld00d I restarted CI just to be sure it's not a one-time issue

boaz0 · 2016-07-26T13:16:10Z

@thaJeztah thanks.

boaz0 · 2016-07-26T15:07:40Z

@thaJeztah now different failures.
I guess I will re-trigger tests later (when most of us are in bed).

BTW, should I create a new PR to docker/engine-api (this commit consists changes in vendor)?

thaJeztah · 2016-07-26T17:06:12Z

@ripcurld00d you can make the PR now, or wait until we're in code review. I added the "needs-vendoring" label to let others know

thaJeztah · 2016-08-11T18:08:11Z

We're ok with this; moving to code review!

tiborvass · 2016-08-11T18:08:26Z

vendor/src/github.com/docker/engine-api/types/types.go

+	BlockWrite       float64
+	PidsCurrent      uint64
+	Mu               sync.Mutex
+	Err              error


same for Err

Not sure we should move these to engine-api just to get formatting on the client.

@cpuguy83 if we keep the types.ContainerStats in the container package then we will get an import cycle error:

import cycle not allowed package github.com/docker/docker/cmd/docker imports github.com/docker/docker/cli/cobraadaptor imports github.com/docker/docker/api/client/container imports github.com/docker/docker/api/client/formatter imports github.com/docker/docker/api/client/container

Anyway, to avoid this cycle, that structure should be moved to another package. engine-api's type seems like a good place.

Move to formatter? This isn't really an API type, I don't think it belongs in engine-api.

OK, sounds like a better place.

boaz0 · 2016-08-16T18:38:49Z

Commit was updated.
cc @tiborvass @cpuguy83 @AkihiroSuda @thaJeztah

boaz0 · 2016-08-28T13:26:12Z

ummm... any feedback?

vdemeester · 2016-09-05T17:05:08Z

It's sad that Write method of the Context object do not return an error because if we do set a wrong format, it will print the template error… It would be could to either pre-validate the format or make Write return an error to not loop for ever.

boaz0 · 2016-09-06T05:49:22Z

@vdemeester, I agree! Nevertheless, it should be done in a different PR.
Anyway, I would like to work on it too, if you don't mind. 😅

BTW, should I open a proposal first?
Thanks.

vdemeester · 2016-09-06T07:38:48Z

@ripcurld00d yes, I'm more than ok for it to be in a separate PR, and you definitely can work on it yes 😉

boaz0 · 2016-09-14T06:54:18Z

@thaJeztah rebased:

~~I think the "needs-vendoring" tag can be removed.~~
~~It seems like tests failed due to a Jenkins slave connectivity problem.~~

thaJeztah · 2016-09-14T13:17:13Z

@ripcurld00d triggered a rebuild for CI

@vdemeester does the current implementation LGTY (if the other issue is handled in a different PR)?

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

boaz0 · 2016-09-18T12:29:56Z

Rebased (2) successfully 😰
@vdemeester @thaJeztah

boaz0 · 2016-09-21T04:45:50Z

ping 😊

vdemeester · 2016-09-21T07:55:23Z

LGTM 🐸
/cc @dnephin @cpuguy83 @runcom

dnephin · 2016-09-21T14:37:53Z

LGTM

thaJeztah · 2016-09-21T14:41:53Z

Looks like this went in without documentation changes opened #26779 for tracking that. @ripcurld00d can you do a follow up to update the documentation?

thaJeztah · 2016-09-21T14:42:26Z

and ping @albers @sdurrheimer for the completion scripts ❤️

runcom · 2016-09-21T14:42:58Z

looks like my comment went un-noticed as well :)

runcom · 2016-09-21T09:09:16Z

cli/command/formatter/stats.go

+
+// ContainerStats represents the containers statistics data.
+type ContainerStats struct {
+	Mu sync.RWMutex


Mu? :) how about Mutex?

BTW, exposing the mutex seems weird to me, I expected what needs to be done outside of this package was abstracted here so that the mutex is just an internal property of this struct. Exposing it, uhm, doesn't sound good. Maybe locking up in the call stack instead? /cc @cpuguy83 wdyt

@ripcurld00d @vdemeester I'd love to have this taken care of :(

@runcom I was about to suggest myself but I don't mind you fixing this.

@runcom I was about to suggest myself but I don't mind you fixing this.

yeah, I'll try to find some time so feel free to go ahead and take a stab at this...

@ripcurld00d you can go ahead, @runcom suggested we should take care of it, not that he would like to take care of it. It's all yours @ripcurld00d 😉

@runcom @vdemeester -- thanks. I would love to finish what I have started :)

albers · 2016-09-21T18:15:41Z

I'm a bit disappointed that --format does not give access to container names.

It has always been bothering me that the first column of docker stats shows container IDs. This is not very helpful because the rows keep flipping around. To get the names, you have to issue docker ps <container names>. This means that I have to decide whether I want to automatically see all containers as IDs or if I have to manually select them just because I want to see descriptive names.

How about adding .ID and .Name placeholders that let me decide what to see instead of the automatic .Container column?

boaz0 · 2016-09-21T18:25:36Z

@albers, this patch supposes to add format to the stats command. The next part, which I would like to work on, is adding container names to the stats.

albers · 2016-09-21T18:26:10Z

@ripcurld00d That's great news! Thanks.

thaJeztah · 2016-09-21T19:08:23Z

@ripcurld00d oh, that means #20973 should probably not be closed if this doesn't support .Names?

boaz0 · 2016-09-21T20:16:44Z

@thaJeztah that's right.

thaJeztah · 2016-09-26T16:48:46Z

docs were addressed in #26892

Add format to stats

GordonTheTurtle added the status/0-triage label Jul 24, 2016

boaz0 changed the title ~~Stats format prod~~ Add format to stats Jul 24, 2016

AkihiroSuda reviewed Jul 25, 2016
View reviewed changes

thaJeztah added status/1-design-review and removed status/0-triage labels Jul 25, 2016

thaJeztah added this to the 1.13.0 milestone Jul 25, 2016

dnephin added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 25, 2016

thaJeztah added the status/needs-vendoring label Jul 26, 2016

dnephin removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 26, 2016

thaJeztah mentioned this pull request Aug 4, 2016

Add api for stats all containers #25361

Closed

2 tasks

thaJeztah added status/2-code-review and removed status/1-design-review labels Aug 11, 2016

tiborvass reviewed Aug 11, 2016
View reviewed changes

vdemeester removed the status/needs-vendoring label Sep 14, 2016

Add the format switch to the stats command

96622b2

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

dnephin merged commit c0699cd into moby:master Sep 21, 2016

thaJeztah mentioned this pull request Sep 21, 2016

Add documentation for docker stats --format #26779

Closed

thaJeztah added impact/changelog impact/cli labels Sep 21, 2016

runcom reviewed Sep 21, 2016

View reviewed changes

thaJeztah added the docs/revisit label Sep 21, 2016

albers mentioned this pull request Sep 21, 2016

Add bash completion for docker stats --format #26788

Merged

boaz0 mentioned this pull request Sep 22, 2016

Hide the mutex lock in formatter.ContainerStats #26819

Merged

sdurrheimer mentioned this pull request Sep 26, 2016

Add zsh completion for 'docker stats --format' #26909

Merged

thaJeztah removed the docs/revisit label Sep 26, 2016

thaJeztah mentioned this pull request Sep 27, 2016

Update for docker stats #26938

Closed

This was referenced Oct 25, 2016

"docker stats" error on Windows #27671

Closed

Windows: Fix stats CLI #27759

Merged

thaJeztah mentioned this pull request Oct 31, 2016

docker stats: add blkio rate and blkio IOPS #21026

Closed

lowenna mentioned this pull request Nov 2, 2016

Windows docker stats CLI shows linux headers when no containers are running #28005

Closed

dnephin added a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#24987 from ripcurld00d/stats_format_prod

685613f

Add format to stats

Add format to stats #24987

Add format to stats #24987

Conversation

boaz0 commented Jul 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 commented Jul 25, 2016

dnephin commented Jul 25, 2016

boaz0 commented Jul 25, 2016

boaz0 commented Jul 26, 2016 • edited Loading

thaJeztah commented Jul 26, 2016

boaz0 commented Jul 26, 2016

boaz0 commented Jul 26, 2016 • edited Loading

thaJeztah commented Jul 26, 2016

thaJeztah commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 Aug 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 commented Aug 16, 2016

boaz0 commented Aug 28, 2016

vdemeester commented Sep 5, 2016

boaz0 commented Sep 6, 2016

vdemeester commented Sep 6, 2016

boaz0 commented Sep 14, 2016 • edited Loading

thaJeztah commented Sep 14, 2016

boaz0 commented Sep 18, 2016

boaz0 commented Sep 21, 2016

vdemeester commented Sep 21, 2016 • edited Loading

dnephin commented Sep 21, 2016

thaJeztah commented Sep 21, 2016

thaJeztah commented Sep 21, 2016

runcom commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 Sep 21, 2016 • 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

albers commented Sep 21, 2016 • edited Loading

boaz0 commented Sep 21, 2016

albers commented Sep 21, 2016

thaJeztah commented Sep 21, 2016

boaz0 commented Sep 21, 2016

thaJeztah commented Sep 26, 2016

boaz0 commented Jul 24, 2016 •

edited

Loading

boaz0 commented Jul 26, 2016 •

edited

Loading

boaz0 commented Jul 26, 2016 •

edited

Loading

boaz0 Aug 15, 2016 •

edited

Loading

boaz0 commented Sep 14, 2016 •

edited

Loading

vdemeester commented Sep 21, 2016 •

edited

Loading

boaz0 Sep 21, 2016 •

edited

Loading

albers commented Sep 21, 2016 •

edited

Loading