Skip to content

Subscribe to docker stats and place some in the Container topology.#217

Closed
tomwilkie wants to merge 2 commits intoweaveworks:masterfrom
tomwilkie:114-docker-stats
Closed

Subscribe to docker stats and place some in the Container topology.#217
tomwilkie wants to merge 2 commits intoweaveworks:masterfrom
tomwilkie:114-docker-stats

Conversation

@tomwilkie
Copy link
Copy Markdown
Contributor

Work towards #114

@tomwilkie tomwilkie changed the title Subscribe to docker stats and place some in the Container topology. WIP Subscribe to docker stats and place some in the Container topology. Jun 11, 2015
probe/main.go Outdated

This comment was marked as abuse.

This comment was marked as abuse.

@tomwilkie
Copy link
Copy Markdown
Contributor Author

Note this introduces a startup lag (as it connects all the stats go routines on the main thread). This should be moved off to a background thread (and parallelised?)

This comment was marked as abuse.

This comment was marked as abuse.

@peterbourgon
Copy link
Copy Markdown
Contributor

There's a lot of manual mutex locking going on. Since you already have a goroutine handling Docker events, why not thread all user requests through it, too? Then you can eliminate all the locks.

@tomwilkie
Copy link
Copy Markdown
Contributor Author

Yes the locking makes me nervous; going to an actor model seems appropriate here.

I was always considering having our own Container struct, and moving all the stats & updating stuff into that. Then docker mapper is responsible for creating / removing containers in response to events.

We should probably also break out the mapper & topology logic to be a client of this type.

This comment was marked as abuse.

This comment was marked as abuse.

@peterbourgon
Copy link
Copy Markdown
Contributor

I've been reading this for awhile and am having trouble building and keeping a mental model of what's happening. There's a lot of action at a distance, mutating the DockerTagger struct in-place. The preponderance of mutex-locking is an artifact of that. I think it would benefit from a single loop() method handling user requests, events from the Docker daemon, and events from individual /container/foo/stats streams.

@tomwilkie tomwilkie force-pushed the 114-docker-stats branch 2 times, most recently from b27ec9c to 854f957 Compare June 11, 2015 18:36
@tomwilkie
Copy link
Copy Markdown
Contributor Author

@peterbourgon I didn't go full actor on this, but I've rationalised the flow and locking.

Let me know what you think. There is a lot of potential for refactoring here, but I'd prefer to save anything major for another PR.

@tomwilkie tomwilkie changed the title WIP Subscribe to docker stats and place some in the Container topology. Subscribe to docker stats and place some in the Container topology. Jun 11, 2015

This comment was marked as abuse.

@peterbourgon
Copy link
Copy Markdown
Contributor

Much, much easier to understand. Thanks a bunch. LGTM.

@peterbourgon peterbourgon removed their assignment Jun 12, 2015
@tomwilkie
Copy link
Copy Markdown
Contributor Author

Going to hold off on this until post 0.3

@tomwilkie tomwilkie closed this Jun 15, 2015
@tomwilkie tomwilkie deleted the 114-docker-stats branch June 15, 2015 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants