Allow pulling stats once and disconnecting. by cpuguy83 · Pull Request #10766 · moby/moby

cpuguy83 · 2015-02-13T16:50:02Z

Adds a once query param to the stats API which allows API users to
only collect one stats entry and disconnect instead of keeping the
connection alive to stream more stats.

Open to changing the query param here... maybe have a "follow" param that defaults to true (since today we follow by default)?
I've seen several people wanting to just be able to grab and go instead of streaming... and then I saw: discourse/discourse_docker@03b5043#commitcomment-9731330

Figure simple change on the Docker side so clients don't have to deal with it.

phemmer · 2015-02-13T20:30:39Z

Why would this be a server side option, and not client side? The client would just disconnect when it has had enough.
If this is going to be server side, I would argue for a count, not a 'once' parameter. As in "give me 1 stats entry".

cpuguy83 · 2015-02-13T20:43:50Z

@phemmer As stated above, purely for convenience. Easy change, we can take it or leave it (or change it for a better query param behavior).

jessfraz · 2015-02-24T01:34:53Z

@crosbymichael wdyt

icecrime · 2015-03-07T02:13:33Z

Reviewed with @crosbymichael: we suggest having a boolean stream parameter instead, which would be true by default, and stream only once when false. This would have to be available from the cli.

cpuguy83 · 2015-03-07T02:14:32Z

+1 I'll implement.

cpuguy83 · 2015-03-07T04:53:24Z

This is now implemented with the stream query param.
In the CLI I implemented as --no-stream

coolljt0725 · 2015-03-07T11:17:39Z

api/client/commands.go

I think the bash completion scrip should also be update, the --no-stream flag should be add to it:)

That's right, but we usually do it at release time rather than for each CLI change it seems (ping @jfrazelle as 1.6.0 release captain: is it ok for you?)

I personally would prefer we take the habit of updating the completion scripts in the same PR :)

@tiborvass I agree it would be better, but I hate asking for contributors to learn about those (for example the fish completion requires to use a python script which generates completion file from Docker's help message) :-(

I went ahead and added these completion scripts. Please check!

Oh well 😄 re-LGTM!

Why would you not call the flag stream also? --stream=true|false so much cleaner

Because it would have to be defaulted to true, and as such someone would have to type out the full --stream=false, instead of simply --no-stream

icecrime · 2015-03-09T17:10:14Z

api/client/commands.go

I'm confused about the use of && !streamStats here. If the server breaks after 1 stream anyway, isn't the stop condition properly covered by the err = io.EOF part?

Well, I didn't want to change the existing behavior.
And I specifically had to adjust this so that the CLI would actually output data and not an EOF.

Ok sorry, I misunderstood on first read. So this is basically saying: EOF is expected when you didn't ask for streamStats.

icecrime · 2015-03-09T23:26:48Z

LGTM

tiborvass · 2015-03-12T18:07:26Z

integration-cli/docker_cli_stat_test.go

@cpuguy83 chErr <- exec.Command(...).Run()

cpuguy83 · 2015-03-13T15:02:03Z

Updated

tiborvass · 2015-03-13T22:13:09Z

@cpuguy83 is this a breaking change in API? I see stream being false by default.

cpuguy83 · 2015-03-15T17:56:16Z

api/server/server.go

@tiborvass It shouldn't be defaulting to false because of this right here.
Tested again just to make sure.

jamtur01 · 2015-03-17T01:33:59Z

Docs LGTM

cpuguy83 · 2015-04-16T16:02:10Z

Rebased.
Also exits more cleanly/safely now, and fixed a display issue from the last rebase.

icecrime · 2015-04-28T01:34:45Z

Needs rebase :-(

cpuguy83 · 2015-04-28T02:57:35Z

rebased

tobegit3hub · 2015-04-28T03:08:55Z

Great job @cpuguy83 👍

tiborvass · 2015-05-04T14:30:37Z

integration-cli/docker_cli_stat_test.go

i would inline runCmd

wow, no idea why I had it this way.

Adds a `stream` query param to the stats API which allows API users to only collect one stats entry and disconnect instead of keeping the connection alive to stream more stats. Also adds a `--no-stream` flag to `docker stats` which does the same Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2015-05-04T14:50:36Z

Updated

tiborvass · 2015-05-04T17:29:23Z

LGTM

tiborvass · 2015-05-04T17:29:58Z

Ping @moxiegirl @fredlf @SvenDowideit

SvenDowideit · 2015-05-04T23:12:42Z

docs LGTM

thaJeztah · 2015-05-05T01:15:37Z

docs LGTM

@tiborvass @icecrime do we need another maintainer to LGTM, because there were a number of code changes made after the original LGTM's?

tiborvass · 2015-05-05T05:27:42Z

Not for this one but thanks @thaJeztah !

Allow pulling stats once and disconnecting.

tobegit3hub · 2015-05-05T08:39:34Z

Great job @cpuguy83 and @tiborvass 👍

We gonna have this feature soon!

Priyanky · 2015-08-25T04:59:01Z

Will this fix work for API versions before 1.19 ?
I use 1.18 and streaming seems to be the only way

GordonTheTurtle added the status/0-triage label Feb 13, 2015

tiborvass added status/1-design-review and removed status/0-triage labels Feb 13, 2015

ssanderson mentioned this pull request Feb 26, 2015

[docker] container metrics support from stats endpoint DataDog/dd-agent#1392

Closed

icecrime mentioned this pull request Mar 7, 2015

Add "docker stats -d <count>" option to "Show only count displays, then ... #10790

Closed

cpuguy83 force-pushed the stats_pull_once branch 2 times, most recently from 0294bcf to a838b45 Compare March 7, 2015 04:52

coolljt0725 reviewed Mar 7, 2015
View reviewed changes

cpuguy83 force-pushed the stats_pull_once branch from a838b45 to 821d598 Compare March 7, 2015 14:07

icecrime reviewed Mar 9, 2015
View reviewed changes

icecrime added status/2-code-review and removed status/1-design-review labels Mar 9, 2015

cpuguy83 force-pushed the stats_pull_once branch from 821d598 to e751ab3 Compare March 10, 2015 00:24

tiborvass reviewed Mar 12, 2015
View reviewed changes

integration-cli/docker_cli_stat_test.go Outdated

Copy link

Contributor

tiborvass Mar 12, 2015

Choose a reason for hiding this comment

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

@cpuguy83 chErr <- exec.Command(...).Run()

cpuguy83 force-pushed the stats_pull_once branch 2 times, most recently from 3a34c7c to 967721a Compare March 13, 2015 15:01

thaJeztah mentioned this pull request Mar 15, 2015

support for streaming stats samalba/dockerclient#78

Merged

cpuguy83 reviewed Mar 15, 2015
View reviewed changes

GordonTheTurtle added the dco/yes label Apr 16, 2015

icecrime removed the dco/yes label Apr 23, 2015

cpuguy83 force-pushed the stats_pull_once branch 2 times, most recently from d9b26ed to 8d346c3 Compare April 28, 2015 02:57

tiborvass reviewed May 4, 2015
View reviewed changes

cpuguy83 force-pushed the stats_pull_once branch from 8d346c3 to 9d2cddd Compare May 4, 2015 14:48

cpuguy83 force-pushed the stats_pull_once branch from 9d2cddd to f3023a9 Compare May 4, 2015 14:49

tiborvass added status/3-docs-review and removed status/2-code-review labels May 4, 2015

tiborvass added a commit that referenced this pull request May 5, 2015

Merge pull request #10766 from cpuguy83/stats_pull_once

91fb4d6

Allow pulling stats once and disconnecting.

tiborvass merged commit 91fb4d6 into moby:master May 5, 2015

tobegit3hub mentioned this pull request May 5, 2015

Monitor metrics of containers with docker stats tobegit3hub/seagull#40

Closed

thaJeztah mentioned this pull request May 18, 2015

"/container/<id>/stats" does not return any response even an error when querying with an exited container. #12529

Closed

runcom mentioned this pull request May 23, 2015

GET /containers/(id)/stats broken in 1.7? #13421

Closed

unguiculus mentioned this pull request Jun 12, 2015

there no method for docker stats? spotify/docker-client#172

Closed

cpuguy83 deleted the stats_pull_once branch August 27, 2015 15:48

Conversation

cpuguy83 commented Feb 13, 2015

Uh oh!

phemmer commented Feb 13, 2015

Uh oh!

cpuguy83 commented Feb 13, 2015

Uh oh!

jessfraz commented Feb 24, 2015

Uh oh!

icecrime commented Mar 7, 2015

Uh oh!

cpuguy83 commented Mar 7, 2015

Uh oh!

cpuguy83 commented Mar 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icecrime commented Mar 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Mar 13, 2015

Uh oh!

tiborvass commented Mar 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamtur01 commented Mar 17, 2015

Uh oh!

cpuguy83 commented Apr 16, 2015

Uh oh!

icecrime commented Apr 28, 2015

Uh oh!

cpuguy83 commented Apr 28, 2015

Uh oh!

tobegit3hub commented Apr 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented May 4, 2015

Uh oh!

tiborvass commented May 4, 2015

Uh oh!

tiborvass commented May 4, 2015

Uh oh!

SvenDowideit commented May 4, 2015

Uh oh!

thaJeztah commented May 5, 2015

Uh oh!

tiborvass commented May 5, 2015

Uh oh!

tobegit3hub commented May 5, 2015

Uh oh!

Priyanky commented Aug 25, 2015

Uh oh!

Reviewers