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

Add support for metrics plugins #32874

Merged
merged 1 commit into from May 12, 2017

Conversation

@cpuguy83
Contributor

cpuguy83 commented Apr 27, 2017

Allows for a plugin type that can be used to scrape metrics.
This is useful because metrics are not necessarily at a standard
location... --metrics-addr must be set, and must currently be a TCP
socket.
Even if metrics are done via a unix socket, there's no guarantee where
the socket may be located on the system, making bind-mounting such a
socket into a container difficult (and racey, failure-prone on daemon
restart).

Metrics plugins side-step this issue by always listening on a unix
socket and then bind-mounting that into a known path in the plugin
container.

Note there has been similar work in the past (and ultimately punted at
the time) for consistent access to the Docker API from within a
container.

Why not add metrics to the Docker API and just provide a plugin with
access to the Docker API? Certainly this can be useful, but gives a lot
of control/access to a plugin that may only need the metrics. We can
look at supporting API plugins separately for this reason.

p03t268b

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 27, 2017

Member

Some questions;

  • given that the metrics API is still experimental, should we mark this plugin-type as experimental as well?
  • would other plugin-types benefit from having access to the metrics API, or would that be conflating things? (not sure if entitlements are meant to be a thing for plugins, but linking in case they will be #32801)
Member

thaJeztah commented Apr 27, 2017

Some questions;

  • given that the metrics API is still experimental, should we mark this plugin-type as experimental as well?
  • would other plugin-types benefit from having access to the metrics API, or would that be conflating things? (not sure if entitlements are meant to be a thing for plugins, but linking in case they will be #32801)
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 27, 2017

Member

/cc @rogaha, who I think was interested in this functionality :)

Member

thaJeztah commented Apr 27, 2017

/cc @rogaha, who I think was interested in this functionality :)

@rogaha

This comment has been minimized.

Show comment
Hide comment
@rogaha

rogaha Apr 27, 2017

Contributor

@thaJeztah 👍 this is great!

given that the metrics API is still experimental, should we mark this plugin-type as experimental as well?

Ideally we should think about promoting the metrics API instead, it looks like we are standardizing on that. containerd is also using this approach.

would other plugin-types benefit from having access to the metrics API, or would that be conflating things? (not sure if entitlements are meant to be a thing for plugins, but linking in case they will be #32801)

I think this plugin type is generic enough to englobe all the use-cases that want to access the metrics API. @cpuguy83 do you see any restriction with current implementation in terms of accessing the metrics API data?

Contributor

rogaha commented Apr 27, 2017

@thaJeztah 👍 this is great!

given that the metrics API is still experimental, should we mark this plugin-type as experimental as well?

Ideally we should think about promoting the metrics API instead, it looks like we are standardizing on that. containerd is also using this approach.

would other plugin-types benefit from having access to the metrics API, or would that be conflating things? (not sure if entitlements are meant to be a thing for plugins, but linking in case they will be #32801)

I think this plugin type is generic enough to englobe all the use-cases that want to access the metrics API. @cpuguy83 do you see any restriction with current implementation in terms of accessing the metrics API data?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 27, 2017

Contributor

Plugins can claim they implement any number of plugin types. So when registering it just needs to report both it's "normal" type and "MetricsCollector".

Contributor

cpuguy83 commented Apr 27, 2017

Plugins can claim they implement any number of plugin types. So when registering it just needs to report both it's "normal" type and "MetricsCollector".

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 28, 2017

Member

Plugins can claim they implement any number of plugin types. So when registering it just needs to report both it's "normal" type and "MetricsCollector".

Ah, completely forgot about that; yes that's great

Member

thaJeztah commented Apr 28, 2017

Plugins can claim they implement any number of plugin types. So when registering it just needs to report both it's "normal" type and "MetricsCollector".

Ah, completely forgot about that; yes that's great

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe May 2, 2017

Contributor

Does this bind arbitrary metrics to Docker's /metrics API?

Contributor

stevvooe commented May 2, 2017

Does this bind arbitrary metrics to Docker's /metrics API?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 2, 2017

Contributor

@stevvooe No, this does not do anything to the Docker API.
It's a dedicated socket only for metrics.

Contributor

cpuguy83 commented May 2, 2017

@stevvooe No, this does not do anything to the Docker API.
It's a dedicated socket only for metrics.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe May 2, 2017

Contributor

@stevvooe No, this does not do anything to the Docker API.
It's a dedicated socket only for metrics.

Not quite what I asked: does this allow plugins to export metrics to on Docker's /metrics endpoint or does this expose new targets? Should we expose separate targets?

Contributor

stevvooe commented May 2, 2017

@stevvooe No, this does not do anything to the Docker API.
It's a dedicated socket only for metrics.

Not quite what I asked: does this allow plugins to export metrics to on Docker's /metrics endpoint or does this expose new targets? Should we expose separate targets?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 3, 2017

Contributor

This exposes docker's /metrics endpoint to a plugin.

Contributor

cpuguy83 commented May 3, 2017

This exposes docker's /metrics endpoint to a plugin.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe May 3, 2017

Contributor

This exposes docker's /metrics endpoint to a plugin.

Rad.

Contributor

stevvooe commented May 3, 2017

This exposes docker's /metrics endpoint to a plugin.

Rad.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 3, 2017

Member

design looks good to me as well, moving to code review

Member

thaJeztah commented May 3, 2017

design looks good to me as well, moving to code review

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented May 3, 2017

logrus.WithField("path", pluginSockPath).Debugf("creating plugin socket")
f, err := os.OpenFile(pluginSockPath, os.O_CREATE, 0600)
if err != nil {
return

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan May 5, 2017

Contributor

sockBase can be deleted on this and other errors.

@anusha-ragunathan

anusha-ragunathan May 5, 2017

Contributor

sockBase can be deleted on this and other errors.

This comment has been minimized.

@cpuguy83

cpuguy83 May 6, 2017

Contributor

Added

@cpuguy83

cpuguy83 May 6, 2017

Contributor

Added

@thaJeztah thaJeztah added this to the 17.06.0 milestone May 5, 2017

@thaJeztah thaJeztah removed this from backlog in maintainers-session May 5, 2017

}
```
If an error occurred during this request, add an error message to the `Err` field

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan May 5, 2017

Contributor

How about expect the plugin to net.Listen on the bind mounted metrics socket and reports errors? That's more defined. Else, its hard to know what errors the plugin to report here.

@anusha-ragunathan

anusha-ragunathan May 5, 2017

Contributor

How about expect the plugin to net.Listen on the bind mounted metrics socket and reports errors? That's more defined. Else, its hard to know what errors the plugin to report here.

This comment has been minimized.

@cpuguy83

cpuguy83 May 5, 2017

Contributor

I'm not sure I understand this one.

@cpuguy83

cpuguy83 May 5, 2017

Contributor

I'm not sure I understand this one.

}
```
If an error occurred during this request, add an error message to the `Err` field

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan May 5, 2017

Contributor

Report any socket Close errors?

@anusha-ragunathan

anusha-ragunathan May 5, 2017

Contributor

Report any socket Close errors?

This comment has been minimized.

@cpuguy83

cpuguy83 May 5, 2017

Contributor

?

@cpuguy83

cpuguy83 May 5, 2017

Contributor

?

@anusha-ragunathan

This comment has been minimized.

Show comment
Hide comment
@anusha-ragunathan

anusha-ragunathan May 5, 2017

Contributor

Design LGTM.

Can we add a test with a sample metrics plugin?
https://github.com/moby/moby/blob/master/docs/extend/config.md#config-field-descriptions needs to be updated with the new MetricsCollector interface

Contributor

anusha-ragunathan commented May 5, 2017

Design LGTM.

Can we add a test with a sample metrics plugin?
https://github.com/moby/moby/blob/master/docs/extend/config.md#config-field-descriptions needs to be updated with the new MetricsCollector interface

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 5, 2017

Contributor

@anusha-ragunathan How about a unit test? An integration test for this would be fairly difficult I think.

Nevermind, added integration test.

Contributor

cpuguy83 commented May 5, 2017

@anusha-ragunathan How about a unit test? An integration test for this would be fairly difficult I think.

Nevermind, added integration test.

@rogaha

This comment has been minimized.

Show comment
Hide comment
@rogaha

rogaha May 8, 2017

Contributor

awesome, thanks for adding the integration tests @cpuguy83. /cc @tiborvass

Contributor

rogaha commented May 8, 2017

awesome, thanks for adding the integration tests @cpuguy83. /cc @tiborvass

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 10, 2017

Member

This will also need changes to the CLI (possibly API) to support filtering for this type of plugin (and the CLI reference docs; https://github.com/moby/moby/blob/master/docs/reference/commandline/plugin_ls.md#filtering)

Member

thaJeztah commented May 10, 2017

This will also need changes to the CLI (possibly API) to support filtering for this type of plugin (and the CLI reference docs; https://github.com/moby/moby/blob/master/docs/reference/commandline/plugin_ls.md#filtering)

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 10, 2017

Contributor

This is updated.

Also added a note as discussed about the metrics plugin interface being non-experimental but the metrics themselves are still experimental and may change.

Contributor

cpuguy83 commented May 10, 2017

This is updated.

Also added a note as discussed about the metrics plugin interface being non-experimental but the metrics themselves are still experimental and may change.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 10, 2017

Contributor

@thaJeztah I'll work on a CLI update.
Doesn't look like API is too concerned about the plugin type that is passed through the filter.

Contributor

cpuguy83 commented May 10, 2017

@thaJeztah I'll work on a CLI update.
Doesn't look like API is too concerned about the plugin type that is passed through the filter.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 10, 2017

Contributor

Oh I see the docs for the CLI are still in this repo. Update incoming.

Contributor

cpuguy83 commented May 10, 2017

Oh I see the docs for the CLI are still in this repo. Update incoming.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 10, 2017

Contributor

Z failure is the known netns error, unrelated.
Otherwise all green.

Contributor

cpuguy83 commented May 10, 2017

Z failure is the known netns error, unrelated.
Otherwise all green.

@rogaha

This comment has been minimized.

Show comment
Hide comment
@rogaha

rogaha May 10, 2017

Contributor

👍 thanks @cpuguy83!

/cc @thaJeztah @tiborvass

Contributor

rogaha commented May 10, 2017

👍 thanks @cpuguy83!

/cc @thaJeztah @tiborvass

@thaJeztah

left two nits on the docs

Show outdated Hide outdated docs/extend/plugins_metrics.md
Show outdated Hide outdated docs/extend/plugins_metrics.md
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 10, 2017

Contributor

Fixed docs comments.

Contributor

cpuguy83 commented May 10, 2017

Fixed docs comments.

@thaJeztah

docs LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 11, 2017

Member

ping @tiborvass @anusha-ragunathan anything more needed?

Member

thaJeztah commented May 11, 2017

ping @tiborvass @anusha-ragunathan anything more needed?

Add support for metrics plugins
Allows for a plugin type that can be used to scrape metrics.
This is useful because metrics are not neccessarily at a standard
location... `--metrics-addr` must be set, and must currently be a TCP
socket.
Even if metrics are done via a unix socket, there's no guarentee where
the socket may be located on the system, making bind-mounting such a
socket into a container difficult (and racey, failure-prone on daemon
restart).

Metrics plugins side-step this issue by always listening on a unix
socket and then bind-mounting that into a known path in the plugin
container.

Note there has been similar work in the past (and ultimately punted at
the time) for consistent access to the Docker API from within a
container.

Why not add metrics to the Docker API and just provide a plugin with
access to the Docker API? Certainly this can be useful, but gives a lot
of control/access to a plugin that may only need the metrics. We can
look at supporting API plugins separately for this reason.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass May 12, 2017

Collaborator

LGTM

Collaborator

tiborvass commented May 12, 2017

LGTM

@tiborvass tiborvass merged commit 680084b into moby:master May 12, 2017

5 of 6 checks passed

windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13871 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34038 has succeeded
Details
janky Jenkins build Docker-PRs 42639 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3019 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2740 has succeeded
Details

@cpuguy83 cpuguy83 deleted the cpuguy83:metrics_plugins branch Jun 28, 2017

albers added a commit to albers/docker-cli that referenced this pull request Jul 3, 2017

Add new plugin types to bash completion for `plugin ls --filter capab…
…ility`

This adds bash completion for moby/moby#32874.

Signed-off-by: Harald Albers <github@albersweb.de>

albers added a commit to albers/docker-cli that referenced this pull request Jul 3, 2017

Add metric plugins to bash completion for `plugin ls --filter capabil…
…ity`

This adds bash completion for moby/moby#32874.

Signed-off-by: Harald Albers <github@albersweb.de>

thaJeztah added a commit to thaJeztah/docker-ce that referenced this pull request Jul 25, 2017

Add metric plugins to bash completion for `plugin ls --filter capabil…
…ity`

This adds bash completion for moby/moby#32874.

Signed-off-by: Harald Albers <github@albersweb.de>
(cherry picked from commit 2caf425f02363a5894b5790dcde47a11808d23e1)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

alshabib added a commit to alshabib/cli that referenced this pull request Aug 1, 2017

Add metric plugins to bash completion for `plugin ls --filter capabil…
…ity`

This adds bash completion for moby/moby#32874.

Signed-off-by: Harald Albers <github@albersweb.de>

riyazdf added a commit to riyazdf/cli that referenced this pull request Aug 2, 2017

Add metric plugins to bash completion for `plugin ls --filter capabil…
…ity`

This adds bash completion for moby/moby#32874.

Signed-off-by: Harald Albers <github@albersweb.de>

vieux pushed a commit to vieux/docker-ce that referenced this pull request Aug 10, 2017

Add metric plugins to bash completion for `plugin ls --filter capabil…
…ity`

This adds bash completion for moby/moby#32874.

Signed-off-by: Harald Albers <github@albersweb.de>
Upstream-commit: 2caf425f02363a5894b5790dcde47a11808d23e1
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment