*: serve '/metrics' in insecure port #8282
Conversation
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"` | |||
There was a problem hiding this comment.
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
force-pushed
the
metrics-port
branch
3 times, most recently
from
851c62a
to
a731fb1
Compare
gyuho
force-pushed
the
metrics-port
branch
3 times, most recently
from
39a24f0
to
71f355f
Compare
gyuho
force-pushed
the
metrics-port
branch
4 times, most recently
from
ad1349d
to
7aaeb9f
Compare
etcdserver/api/metrics/metrics.go
Outdated
| } | ||
|
|
||
| // Handler serves health and metrics information. | ||
| func Handler(srv *etcdserver.EtcdServer) http.HandlerFunc { |
There was a problem hiding this comment.
NewMetricsHandler to be consistent with NewClientHandler in v2http?
etcdserver/api/metrics/metrics.go
Outdated
| } | ||
|
|
||
| // RegisterPrometheus registers prometheus handler on '/metrics'. | ||
| func RegisterPrometheus(mux *http.ServeMux) { |
There was a problem hiding this comment.
should these be called HandlePrometheus and HandleHealth since there's already Handle everywhere?
etcdserver/api/metrics/metrics.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| PathMetrics = "/metrics" |
There was a problem hiding this comment.
keep unexported until the code needs to override them?
etcdserver/api/metrics/metrics.go
Outdated
| @@ -0,0 +1,115 @@ | |||
| // Copyright 2017 The etcd Authors | |||
There was a problem hiding this comment.
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)
e2e/cluster_proxy_test.go
Outdated
| @@ -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} } | |||
There was a problem hiding this comment.
can this panic instead? the metrics tests are !cluster_proxy so it shouldn't hit this path
e2e/metrics_test.go
Outdated
| defer testutil.AfterTest(t) | ||
|
|
||
| cfg := configTLS | ||
| cfg.metricsURL = true |
There was a problem hiding this comment.
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
...
}
etcdserver/api/etcdhttp/metrics.go
Outdated
| ) | ||
|
|
||
| // Health returns true if etcdserver is available. | ||
| func Health(srv *etcdserver.EtcdServer) bool { |
etcdserver/api/etcdhttp/metrics.go
Outdated
| } | ||
|
|
||
| // HealthHandler handles '/health' requests. | ||
| func HealthHandler(srv *etcdserver.EtcdServer) http.HandlerFunc { |
etcdserver/api/etcdhttp/metrics.go
Outdated
| // NewMetricsHandler serves health and metrics information. | ||
| func NewMetricsHandler(srv *etcdserver.EtcdServer) http.HandlerFunc { | ||
| h := prometheus.Handler() | ||
| return func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
reuse the health handler instead of reimplementing?
hh := NewHealthHander(srv)
return func(...) {
...
hh(w, r)
}
etcdserver/api/etcdhttp/metrics.go
Outdated
| h.ServeHTTP(w, r) | ||
| return | ||
| } | ||
| conn, buf, err := hj.Hijack() |
There was a problem hiding this comment.
probably needs a comment on why this needs hijacking; it's not clear to me why ordinary muxing won't work
gyuho
force-pushed
the
metrics-port
branch
2 times, most recently
from
df93fa5
to
272eaf8
Compare
etcdserver/api/etcdhttp/metrics.go
Outdated
| // 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 |
There was a problem hiding this comment.
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))
}
etcdserver/api/etcdhttp/metrics.go
Outdated
| h := prometheus.Handler() | ||
| hh := newHealthHandler(srv, "prometheus") // to be consistent with Prometheus handler | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| hh(w, r) |
There was a problem hiding this comment.
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...
e2e/metrics_test.go
Outdated
| } | ||
| }() | ||
|
|
||
| if err = cURLGet(epc, cURLReq{endpoint: "/metrics", expected: `health true`, metricsURL: true}); err != nil { |
There was a problem hiding this comment.
test /health as well as /metrics?
gyuho
changed the title
gyuho
force-pushed
the
metrics-port
branch
2 times, most recently
from
79beb19
to
518db42
Compare
Signed-off-by: Gyu-Ho Lee <[email protected]>
Signed-off-by: Gyu-Ho Lee <[email protected]>
Signed-off-by: Gyu-Ho Lee <[email protected]>
Fix #8226.