Skip to content

Do not require "experimental" for metrics API#40427

Merged
tiborvass merged 1 commit into
moby:masterfrom
thaJeztah:prometheus_remove_experimental
May 8, 2020
Merged

Do not require "experimental" for metrics API#40427
tiborvass merged 1 commit into
moby:masterfrom
thaJeztah:prometheus_remove_experimental

Conversation

@thaJeztah

@thaJeztah thaJeztah commented Jan 29, 2020

Copy link
Copy Markdown
Member

This removes the requirement to enable experimental on the daemon to expose the prometheus metrics

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@AkihiroSuda

Copy link
Copy Markdown
Member

still draft?

@thaJeztah thaJeztah force-pushed the prometheus_remove_experimental branch from 973dd3e to 07af276 Compare February 10, 2020 20:26
@thaJeztah thaJeztah marked this pull request as ready for review February 10, 2020 20:26
@thaJeztah

Copy link
Copy Markdown
Member Author

Should probably be ok for review; rebased and moved out of "draft"

@thaJeztah thaJeztah added this to the 20.03.0 milestone Feb 14, 2020

@tao12345666333 tao12345666333 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@thaJeztah

Copy link
Copy Markdown
Member Author

ping @ehazlett PTAL; wdyt?

@thaJeztah

Copy link
Copy Markdown
Member Author

(@ehazlett discussing with @tonistiigi to check if the metrics we have currently would conflict with metrics we could add in future, coming from containerd)

Comment thread daemon/metrics_unix.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a debug log?

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the prometheus_remove_experimental branch from 07af276 to f337a8d Compare April 20, 2020 20:19
@thaJeztah

Copy link
Copy Markdown
Member Author

@cpuguy83 @tonistiigi updated; PTAL

@cpuguy83 cpuguy83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread daemon/metrics_unix.go
mux.Handle("/metrics", metrics.Handler())
go func() {
http.Serve(l, mux)
logrus.Debugf("metrics API listening on %s", l.Addr())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't understand why we have two places (here and in cmd/dockerd) where we start the metrics server.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for metrics plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment