add docker stats (new in docker1.5) by donhcd · Pull Request #71 · samalba/dockerclient

donhcd · 2015-02-03T01:18:20Z

I'm very open to suggestions on a better interface for stats, or better ways to write this

donhcd · 2015-02-03T01:20:01Z

ping @vieux @aluzzardi please take a look when you get a moment

BrianBland · 2015-02-03T01:59:29Z

You should remove these debug prints first...

good point 👍
On Mon, Feb 2, 2015 at 17:59 Brian Bland notifications@github.com wrote:

In dockerclient.go
#71 (comment):

for {

go func() {

defer func() {

recover() // ugly but necessary for sending on closed chan

}()

var containerStats stats.Stats

if err := decoder.Decode(&containerStats); err != nil {

internalErrorChan <- err

return

}

internalStatsChan <- containerStats

}()

select {

case containerStats := <-internalStatsChan:

fmt.Printf("first\n")

You should remove these debug prints first...

—
Reply to this email directly or view it on GitHub
https://github.com/samalba/dockerclient/pull/71/files#r23977267.

samalba · 2015-02-03T19:33:20Z

No way, we should never import docker packages directly.

The main purpose of starting this library was to be entirely client side (everything over HTTP, no docker internal import). This can break the library anytime.

samalba · 2015-02-03T19:35:06Z

Does the docker remote API have support for stats yet? If yes, then we should leverage it. If not, the library cannot support it and this PR should be rejected.

donhcd · 2015-02-03T20:36:37Z

@samalba I am using the docker remote API. I'm only importing docker/api/stats for the stats.Stats type, which is the type that the remote API encodes. This type should be relatively stable, considering https://github.com/docker/docker/blob/master/daemon/stats.go#L36, but I can redeclare this struct if you prefer that

samalba · 2015-02-03T20:52:25Z

@donhcd Got it. So you should redeclare the types you need over there: https://github.com/samalba/dockerclient/blob/master/types.go

donhcd · 2015-02-03T21:25:56Z

okay, will do

donhcd · 2015-02-03T21:46:55Z

redeclared the types, but there's actually a race in the test right now, which I'm working on resolving

donhcd · 2015-02-03T22:04:34Z

fixed the race, ready for review now. I'm running the tests 1000 times just in case any other races pop up, but I haven't seen any problems yet. ping @samalba

aluzzardi · 2015-02-04T01:37:26Z

mockclient/mock.go should be updated as well.

donhcd · 2015-02-04T04:38:02Z

@aluzzardi updated. I'm a little confused what mockclient is used for... is it used somewhere?

aluzzardi · 2015-02-04T04:43:34Z

@donhcd It's for library clients to use in order to mock dockerclient (as in, to fake dockerclient responses in unit tests).

Here's an example on how we use this on Swarm:
https://github.com/docker/swarm/blob/master/cluster/node_test.go#L43

donhcd · 2015-02-04T05:39:23Z

ah, I see. thanks for the link :)

donhcd · 2015-02-09T17:54:37Z

ping @aluzzardi @vieux

vieux · 2015-02-09T18:19:37Z

@samalba are you ok with the current format ?

samalba · 2015-02-09T18:29:47Z

@vieux Looks ok to me, what do you think?

vieux · 2015-02-09T19:33:20Z

same, please @crosbymichael can you take a look, you are the most familiar with the stats endpoint.

always require sending on closeChan, even if an error was sent.

the reader only needs to close closeChan when they are done

donhcd · 2015-03-31T20:44:46Z

ping @crosbymichael

Conflicts: dockerclient.go dockerclient_test.go example_responses.go types.go

Conflicts: dockerclient.go dockerclient_test.go example_responses.go interface.go mockclient/mock.go types.go

Conflicts: interface.go mockclient/mock.go

pdevine · 2016-02-03T06:59:20Z

Unfortunately stats don't get added until API 1.17, so you'll have to bump APIVersion. I don't know what the delta between 1.15 and 1.17 is though and if we'd be missing anything here.

the general approach of this repo so far has just been to keep the API version the same despite adding API calls that rely on newer API versions (e.g. ListNetworks). Given that (1) we don't have a testing framework to actually test all of the dockerclient methods on docker engines of all versions and (2) it's not uncommon for API changes to be breaking and undocumented, switching the APIVersion has high risk and low reward :/

If we were to switch the APIVersion, it should probably be in a separate PR.

Def. agree we should do it in a separate PR. For that change we should just tag the repo at 1.5 and move on. If someone wants the older version, they can pull the tag. How about a compromise for this PR until we get the library in better shape by just commenting it (and maybe ListNetworks too) so we don't forget.

The flip side of this is it looks really weird in the UCP (and presumably DTR) logs that says the API version is 1.15.

pdevine · 2016-02-03T07:27:03Z

The only thing that really concerns me is we should bump the APIVersion up, but I don't know what's missing in here vs. 1.17. Otherwise, LGTM.

donhcd · 2016-02-04T16:35:13Z

@pdevine I just added a comment to the APIVersion

pdevine · 2016-02-04T23:08:17Z

LGTM.

add docker stats (new in docker1.5)

BrianBland reviewed Feb 3, 2015
View reviewed changes

donhcd force-pushed the stats branch 2 times, most recently from 388f1a6 to af77c7c Compare February 3, 2015 02:04

samalba reviewed Feb 3, 2015
View reviewed changes

donhcd force-pushed the stats branch from 8192a20 to 7aa221f Compare February 3, 2015 22:07

donhcd added 7 commits March 24, 2015 01:22

add docker stats (new in docker1.5)

fd15319

factor out decode loop

67dfd25

fix closeChan race

a54efa2

always require sending on closeChan, even if an error was sent.

copy Stats struct from docker/docker/api/stats

5cceda0

fix race in dockerclient test, fix logs

28390c7

update mockclient

a893008

don't require sending to closeChan

04885d9

the reader only needs to close closeChan when they are done

donhcd force-pushed the stats branch from a054340 to 04885d9 Compare March 24, 2015 01:29

donhcd added 12 commits April 10, 2015 20:35

fix comments

b603144

fix comments again

85d6209

fix comments again

23cbb10

fix naming and remove print

e280bcb

fix naming and remove print

787e623

update ContainerStats to use readJSONStream method

b451772

remove nested struct in MonitorEventsOptions

f771cfc

properly use the secret MonitorEvents API

d6ddf74

remove recover() handler

978fef2

Merge branch 'betterMonitorEventsPR' into stats

a25593a

Conflicts: dockerclient.go dockerclient_test.go example_responses.go types.go

fix deadlock

11fef81

Merge branch 'betterMonitorEventsPR' into stats

606c3b0

donhcd mentioned this pull request May 18, 2015

add MonitorEvents which returns channels #90

Merged

Merge branch 'upstream/master' into stats

856f09d

Conflicts: dockerclient.go dockerclient_test.go example_responses.go interface.go mockclient/mock.go types.go

donhcd force-pushed the stats branch 3 times, most recently from 8e9eccd to 04885d9 Compare February 3, 2016 06:29

donhcd added 2 commits February 3, 2016 06:37

Merge branch 'master' of github.com:samalba/dockerclient into stats

10dc8b1

Conflicts: interface.go mockclient/mock.go

add nopclient

9017ecd

pdevine reviewed Feb 3, 2016
View reviewed changes

donhcd added 2 commits February 3, 2016 19:19

addressing comments

2b6db89

add comment to APIVersion

b3002dd

donhcd added a commit that referenced this pull request Feb 4, 2016

Merge pull request #71 from donhcd/stats

4278ba3

add docker stats (new in docker1.5)

donhcd merged commit 4278ba3 into samalba:master Feb 4, 2016

donhcd deleted the stats branch February 4, 2016 23:12

Conversation

donhcd commented Feb 3, 2015

Uh oh!

donhcd commented Feb 3, 2015

Uh oh!

BrianBland Feb 3, 2015

Choose a reason for hiding this comment

Uh oh!

donhcd Feb 3, 2015

Choose a reason for hiding this comment

Uh oh!

samalba Feb 3, 2015

Choose a reason for hiding this comment

Uh oh!

samalba commented Feb 3, 2015

Uh oh!

donhcd commented Feb 3, 2015

Uh oh!

samalba commented Feb 3, 2015

Uh oh!

donhcd commented Feb 3, 2015

Uh oh!

donhcd commented Feb 3, 2015

Uh oh!

donhcd commented Feb 3, 2015

Uh oh!

aluzzardi commented Feb 4, 2015

Uh oh!

donhcd commented Feb 4, 2015

Uh oh!

aluzzardi commented Feb 4, 2015

Uh oh!

donhcd commented Feb 4, 2015

Uh oh!

donhcd commented Feb 9, 2015

Uh oh!

vieux commented Feb 9, 2015

Uh oh!

samalba commented Feb 9, 2015

Uh oh!

vieux commented Feb 9, 2015

Uh oh!

donhcd commented Mar 31, 2015

Uh oh!

pdevine Feb 3, 2016

Choose a reason for hiding this comment

Uh oh!

donhcd Feb 3, 2016

Choose a reason for hiding this comment

Uh oh!

pdevine Feb 3, 2016

Choose a reason for hiding this comment

Uh oh!

pdevine commented Feb 3, 2016

Uh oh!

donhcd commented Feb 4, 2016

Uh oh!

pdevine commented Feb 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants