Refactor the statistics of network in docker stats by HuKeping · Pull Request #15786 · moby/moby

HuKeping · 2015-08-24T09:37:14Z

For now docker stats will sum the rxbytes, txbytes, etc. of all
the interfaces.

It is OK for the output of CLI docker stats but not good for
the API response, especially when the container is in sereval
subnets.

It's better to leave these origianl data to user.

Signed-off-by: Hu Keping [email protected]

HuKeping · 2015-08-24T09:38:41Z

For example, the output of stats after this PR will be like:

    ...
    "network": {
        "eth0": {
            "rx_bytes": 5338,
            "rx_dropped": 0,
            "rx_errors": 0,
            "rx_packets": 36,
            "tx_bytes": 648,
            "tx_dropped": 0,
            "tx_errors": 0,
            "tx_packets": 8
        },
        "eth5": {
            "rx_bytes": 4641,
            "rx_dropped": 0,
            "rx_errors": 0,
            "rx_packets": 26,
            "tx_bytes": 690,
            "tx_dropped": 0,
            "tx_errors": 0,
            "tx_packets": 9
        }
    },
    ...

calavera · 2015-08-24T10:04:04Z

I agree that we should change this. However, we cannot replace the json response without versioning the fields. We should keep returning the current json for request versions <= 1.21 and the new json for request versions > 1.22.

calavera · 2015-08-24T10:04:47Z

It will also need some tests and changes in the api docs 😸

HuKeping · 2015-08-24T13:19:30Z

Test updated.

calavera · 2015-08-25T08:03:32Z

api/types/stats.go

We rather move deprecated fields to a separated struct. Look what we do with ContainerJSON in this same file. We have a ContainerJSONBase with the common fields and we compose structs for specific versions with it and the fields required by each version of the api.

I do considered to use the similar solution as what the ContainerJSON does. But later I found it is complicated, and provides not obvious value, it will introduce many things like:

if version.LessThan("1.xx") { ... }

which I don't thinks is a good code style, and it will force the relative functions to add arguments version and maybe more things.

Besides the word Network explain itself well and so does the Networks. When someday we decide to drop Network, it is fine to just remove it and it will not need to remove those

if version <=

If you prefer the old ways, it's OK for me.

We're working on a better way to serialize objects in the api, but until then, we prefer the old way. Believe me, I dislike those conditionals as much as you do, but we need to be consistent.

Updated. I tried to not make a over deep change of the existing code. So I place the version-check the moment before this API return:

If request version < 1.21, sum up those original data and return

if request version >=1.21, the original data will be returned

But it still seems quite redundant. any better solutions please @calavera ?

thaJeztah · 2015-08-30T20:57:27Z

@HuKeping looks like this needs a rebase

Also, don't forget to update the example output in the API docs and add a mention to the API change log here; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md

HuKeping · 2015-08-31T04:17:37Z

@calavera rebased, please take a look if this change is OK

@thaJeztah thanks and I'll update the docs after code review

ehazlett · 2015-09-04T16:25:24Z

@HuKeping would you mind rebasing again? thanks!

HuKeping · 2015-09-06T03:02:03Z

rebased @ehazlett @calavera

calavera · 2015-09-07T15:58:10Z

Thanks @HuKeping. LGTM

runcom · 2015-09-07T16:03:03Z

daemon/stats.go

can we have booleans in ContainerStatsConfig saying which format to use instead of leaking API version here in daemon?

Put API version in ContainerStatsConfig is a good idea, but I prefer to use version instead of boolean, WDYT

jessfraz · 2015-09-08T21:11:58Z

this will need docs updates right

thaJeztah · 2015-09-08T21:45:50Z

Yup, it does; #15786 (comment)

HuKeping · 2015-09-09T01:51:45Z

Docs added @thaJeztah @jfrazelle , PTAL

thaJeztah · 2015-09-09T10:28:08Z

Thanks, @HuKeping. Docs changes LGTM (but we're not in docs-review yet).
Question; this is only affects the API; the output of docker stats on the CLI is still the same, correct?

HuKeping · 2015-09-09T10:45:47Z

Question; this is only affects the API; the output of docker stats on the CLI is still the same, correct?

Yes.

HuKeping · 2015-09-11T10:15:43Z

rebased for conflict

jessfraz · 2015-09-11T20:17:30Z

needs rebase sorry

HuKeping · 2015-09-14T12:32:53Z

rebased @jfrazelle

For now docker stats will sum the rxbytes, txbytes, etc. of all the interfaces. It is OK for the output of CLI `docker stats` but not good for the API response, especially when the container is in sereval subnets. It's better to leave these origianl data to user. Signed-off-by: Hu Keping <[email protected]>

HuKeping · 2015-09-16T01:39:34Z

ping @cpuguy83

cpuguy83 · 2015-09-16T14:32:29Z

LGTM, moving to docs review.

thaJeztah · 2015-09-16T17:19:19Z

docs re-LGTM

ping @SvenDowideit @moxiegirl

moxiegirl · 2015-09-16T18:26:48Z

@HuKeping thank you! LGTM

thaJeztah · 2015-09-16T18:29:08Z

All green, merging \o/

Refactor the statistics of network in docker stats

twhiteman · 2015-10-19T17:37:32Z

@HuKeping the API docs example shows a network JSON entry, it should be networks now right?

https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api_v1.21.md

{
     "read" : "2015-01-08T22:57:31.547920715Z",
     "network": {      <- should be "networks"
             "eth0": {
                 "rx_bytes": 5338,
                 "rx_dropped": 0,
                 ...

HuKeping · 2015-10-20T00:22:59Z

Yes it is @twhiteman , sorry for that I'll fix it.

This fixes a bug introduced in moby#15786: * if a pre-v1.20 client requested docker stats, the daemon would return both an API-compatible JSON blob *and* an API-incompatible JSON blob: see https://gist.github.com/donhcd/338a5b3681cd6a071629 Signed-off-by: Donald Huang <[email protected]>

This fixes a bug introduced in moby#15786: * if a pre-v1.20 client requested docker stats, the daemon would return both an API-compatible JSON blob *and* an API-incompatible JSON blob: see https://gist.github.com/donhcd/338a5b3681cd6a071629 Signed-off-by: Donald Huang <[email protected]> (cherry picked from commit d2c04f8) The commit title wrongfully mentioned API v1.22, when it meant to mention v1.21.

GordonTheTurtle added the status/0-triage label Aug 24, 2015

calavera added status/1-design-review and removed status/0-triage labels Aug 24, 2015

calavera added status/2-code-review and removed status/1-design-review labels Aug 24, 2015

calavera reviewed Aug 25, 2015
View reviewed changes

HuKeping mentioned this pull request Sep 6, 2015

Fix connection block when using docker stats API #14488

Merged

runcom reviewed Sep 7, 2015
View reviewed changes

thaJeztah added impact/api impact/changelog labels Sep 9, 2015

calavera added this to the 1.9.0 milestone Sep 9, 2015

cpuguy83 added status/3-docs-review and removed status/2-code-review labels Sep 16, 2015

moxiegirl added status/4-merge and removed status/3-docs-review labels Sep 16, 2015

thaJeztah added a commit that referenced this pull request Sep 16, 2015

Merge pull request #15786 from HuKeping/stats-network

259a0fb

Refactor the statistics of network in docker stats

thaJeztah merged commit 259a0fb into moby:master Sep 16, 2015

HuKeping deleted the stats-network branch September 17, 2015 01:36

HuKeping mentioned this pull request Oct 20, 2015

Docs: update docs for API stats #17192

Merged

This was referenced Oct 28, 2015

API Changes in Docker 1.9 fsouza/go-dockerclient#407

Closed

API Changes in Docker 1.9 nginx-proxy/docker-gen#129

Open

donhcd mentioned this pull request Oct 30, 2015

fix pre-1.21 docker stats #17549

Merged

thaJeztah mentioned this pull request Jan 15, 2025

use StatsResponse instead of Stats in tests #49284

Merged

Comments

Conversation

HuKeping commented Aug 24, 2015

Uh oh!

HuKeping commented Aug 24, 2015

Uh oh!

calavera commented Aug 24, 2015

Uh oh!

calavera commented Aug 24, 2015

Uh oh!

HuKeping commented Aug 24, 2015

Uh oh!

calavera Aug 25, 2015

Choose a reason for hiding this comment

Uh oh!

HuKeping Aug 25, 2015

Choose a reason for hiding this comment

Uh oh!

calavera Aug 25, 2015

Choose a reason for hiding this comment

Uh oh!

HuKeping Aug 27, 2015

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 30, 2015

Uh oh!

HuKeping commented Aug 31, 2015

Uh oh!

ehazlett commented Sep 4, 2015

Uh oh!

HuKeping commented Sep 6, 2015

Uh oh!

calavera commented Sep 7, 2015

Uh oh!

runcom Sep 7, 2015

Choose a reason for hiding this comment

Uh oh!

HuKeping Sep 8, 2015

Choose a reason for hiding this comment

Uh oh!

jessfraz commented Sep 8, 2015

Uh oh!

thaJeztah commented Sep 8, 2015

Uh oh!

HuKeping commented Sep 9, 2015

Uh oh!

thaJeztah commented Sep 9, 2015

Uh oh!

HuKeping commented Sep 9, 2015

Uh oh!

HuKeping commented Sep 11, 2015

Uh oh!

jessfraz commented Sep 11, 2015

Uh oh!

HuKeping commented Sep 14, 2015

Uh oh!

HuKeping commented Sep 16, 2015

Uh oh!

cpuguy83 commented Sep 16, 2015

Uh oh!

thaJeztah commented Sep 16, 2015

Uh oh!

moxiegirl commented Sep 16, 2015

Uh oh!

thaJeztah commented Sep 16, 2015

Uh oh!

twhiteman commented Oct 19, 2015

Uh oh!

HuKeping commented Oct 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants