-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Allow pulling stats once and disconnecting. #10766
Conversation
Why would this be a server side option, and not client side? The client would just disconnect when it has had enough. |
@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). |
@crosbymichael wdyt |
Reviewed with @crosbymichael: we suggest having a boolean |
+1 I'll implement. |
0294bcf
to
a838b45
Compare
This is now implemented with the |
@@ -2722,6 +2732,7 @@ func (s *containerStats) Display(w io.Writer) error { | |||
|
|||
func (cli *DockerCli) CmdStats(args ...string) error { | |||
cmd := cli.Subcmd("stats", "CONTAINER [CONTAINER...]", "Display a live stream of one or more containers' resource usage statistics", true) | |||
noStream := cmd.Bool([]string{"-no-stream"}, false, "Disable streaming stats and only pull the first result") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the bash completion scrip should also be update, the --no-stream
flag should be add to it:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally would prefer we take the habit of updating the completion scripts in the same PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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) :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and added these completion scripts. Please check!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh well 😄 re-LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you not call the flag stream
also? --stream=true|false
so much cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
a838b45
to
821d598
Compare
@@ -2660,6 +2666,10 @@ func (s *containerStats) Collect(cli *DockerCli) { | |||
for { | |||
var v *types.Stats | |||
if err := dec.Decode(&v); err != nil { | |||
if err == io.EOF && !streamStats { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sorry, I misunderstood on first read. So this is basically saying: EOF
is expected when you didn't ask for streamStats
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
LGTM |
821d598
to
e751ab3
Compare
|
||
chErr := make(chan error, 1) | ||
go func() { | ||
_, err := runCommand(exec.Command(dockerBinary, "stats", "--no-stream", name)) |
There was a problem hiding this comment.
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()
3a34c7c
to
967721a
Compare
Updated |
@cpuguy83 is this a breaking change in API? I see |
@@ -430,6 +430,13 @@ func getContainersStats(eng *engine.Engine, version version.Version, w http.Resp | |||
} | |||
name := vars["name"] | |||
job := eng.Job("container_stats", name) | |||
|
|||
stream := r.Form.Get("stream") | |||
if stream == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiborvass It shouldn't be defaulting to false because of this right here.
Tested again just to make sure.
Docs LGTM |
Rebased. |
Needs rebase :-( |
d9b26ed
to
8d346c3
Compare
rebased |
Great job @cpuguy83 👍 |
name = "statscontainer" | ||
runCmd = exec.Command(dockerBinary, "run", "-d", "--name", name, "busybox", "top") | ||
) | ||
out, _, err := runCommandWithOutput(runCmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would inline runCmd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Updated |
LGTM |
docs LGTM |
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? |
Not for this one but thanks @thaJeztah ! |
Allow pulling stats once and disconnecting.
Great job @cpuguy83 and @tiborvass 👍 We gonna have this feature soon! |
Will this fix work for API versions before 1.19 ? |
Adds a
once
query param to the stats API which allows API users toonly 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.