Skip to content
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

*: serve '/metrics' in insecure port #8282

Merged
merged 3 commits into from
Jul 26, 2017
Merged

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jul 19, 2017

Fix #8226.

embed/config.go Outdated
@@ -118,6 +118,7 @@ type Config struct {
Metrics string `json:"metrics"`
ListenMetricsUrls []url.URL
ListenMetricsUrlsJSON string `json:"listen-metrics-urls"`
ListenMetricsInsecure bool `json:"listen-metrics-insecure"`
Copy link
Contributor

@heyitsanthony heyitsanthony Jul 19, 2017

Choose a reason for hiding this comment

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

why is this needed? this should probably be like the listen flags so it accepts "https://whatever,http://whatever" to use insecure connections with https

@gyuho gyuho force-pushed the metrics-port branch 3 times, most recently from 851c62a to a731fb1 Compare July 19, 2017 21:48
@gyuho gyuho removed the WIP label Jul 19, 2017
@gyuho gyuho force-pushed the metrics-port branch 3 times, most recently from 39a24f0 to 71f355f Compare July 21, 2017 20:41
@etcd-io etcd-io deleted a comment from codecov-io Jul 22, 2017
@etcd-io etcd-io deleted a comment from codecov-io Jul 22, 2017
@etcd-io etcd-io deleted a comment from codecov-io Jul 23, 2017
@gyuho gyuho force-pushed the metrics-port branch 4 times, most recently from ad1349d to 7aaeb9f Compare July 24, 2017 14:24
}

// Handler serves health and metrics information.
func Handler(srv *etcdserver.EtcdServer) http.HandlerFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

NewMetricsHandler to be consistent with NewClientHandler in v2http?

}

// RegisterPrometheus registers prometheus handler on '/metrics'.
func RegisterPrometheus(mux *http.ServeMux) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be called HandlePrometheus and HandleHealth since there's already Handle everywhere?

)

const (
PathMetrics = "/metrics"
Copy link
Contributor

Choose a reason for hiding this comment

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

keep unexported until the code needs to override them?

@@ -0,0 +1,115 @@
// Copyright 2017 The etcd Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

can this stay in etcdhttp but just export the handlers? having both health and metrics in a package called metrics is a little confusing (e.g., does Handler give me a metrics handler or health+metrics handlers)

@@ -55,6 +55,9 @@ func (p *proxyEtcdProcess) Config() *etcdServerProcessConfig { return p.etcdProc
func (p *proxyEtcdProcess) EndpointsV2() []string { return p.proxyV2.endpoints() }
func (p *proxyEtcdProcess) EndpointsV3() []string { return p.proxyV3.endpoints() }

// TODO: proxy doesn't provide health information
func (p *proxyEtcdProcess) EndpointsMetrics() []string { return []string{p.proxyV3.murl} }
Copy link
Contributor

Choose a reason for hiding this comment

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

can this panic instead? the metrics tests are !cluster_proxy so it shouldn't hit this path

defer testutil.AfterTest(t)

cfg := configTLS
cfg.metricsURL = true
Copy link
Contributor

Choose a reason for hiding this comment

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

possible to simplify the config by replacing the metricsURL and insecureMetricsURL with metricsURLScheme string with empty string meaning no metrics url:

func TestV3MetricsSecure(t *testing.T)   { testV3Metrics(t, "https") }
func testV3Metrics(t *testing.T, scheme string) {
	defer testutil.AfterTest(t)

	cfg := configTLS
	cfg.metricsURLScheme = scheme
        ...
}

)

// Health returns true if etcdserver is available.
func Health(srv *etcdserver.EtcdServer) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

unexport?

}

// HealthHandler handles '/health' requests.
func HealthHandler(srv *etcdserver.EtcdServer) http.HandlerFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

NewHealthHandler

// NewMetricsHandler serves health and metrics information.
func NewMetricsHandler(srv *etcdserver.EtcdServer) http.HandlerFunc {
h := prometheus.Handler()
return func(w http.ResponseWriter, r *http.Request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

reuse the health handler instead of reimplementing?

hh := NewHealthHander(srv)
return func(...) {
    ...
    hh(w, r)
}

h.ServeHTTP(w, r)
return
}
conn, buf, err := hj.Hijack()
Copy link
Contributor

Choose a reason for hiding this comment

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

probably needs a comment on why this needs hijacking; it's not clear to me why ordinary muxing won't work

@gyuho gyuho force-pushed the metrics-port branch 2 times, most recently from df93fa5 to 272eaf8 Compare July 25, 2017 18:53
// NewMetricsHandler serves health and metrics information.
func NewMetricsHandler(srv *etcdserver.EtcdServer) http.HandlerFunc {
h := prometheus.Handler()
hh := newHealthHandler(srv, "prometheus") // to be consistent with Prometheus handler
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the metrics port serve "/metrics" and "/health" with separate handlers so it matches the routes normally exposed by etcd? Like this:

func HandleMetricsHealth(mux *http.ServeMux, srv *etcdserver.EtcdServer) {
    mux.Handler(pathMetrics, NewMetricsHandler(srv))
    mux.Handler(pathHealth, NewHealthHandler(srv))
}

h := prometheus.Handler()
hh := newHealthHandler(srv, "prometheus") // to be consistent with Prometheus handler
return func(w http.ResponseWriter, r *http.Request) {
hh(w, r)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the handler still writing out the health if it can be accessed through /health? the metrics should probably match an ordinary client endpoint if possible...

}
}()

if err = cURLGet(epc, cURLReq{endpoint: "/metrics", expected: `health true`, metricsURL: true}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

test /health as well as /metrics?

@gyuho gyuho changed the title *: serve '/metrics' in insecure port with 'health' information *: serve '/metrics' in insecure port Jul 25, 2017
@gyuho gyuho force-pushed the metrics-port branch 2 times, most recently from 79beb19 to 518db42 Compare July 25, 2017 23:55
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

lgtm thanks

@gyuho gyuho merged commit ff7a021 into etcd-io:master Jul 26, 2017
@gyuho gyuho deleted the metrics-port branch July 26, 2017 16:27
@gyuho gyuho mentioned this pull request Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants