etcdserver: add metric counters for livez/readyz health checks. #16797

siyuanfoundation · 2023-10-18T16:48:28Z

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

part of the work for #16007

label field consistent with K8s health metrics

server/etcdserver/api/etcdhttp/health.go

server/etcdserver/api/etcdhttp/health_test.go

siyuanfoundation · 2023-10-18T22:40:40Z

cc @logicalhan @serathius

logicalhan · 2023-10-18T22:44:47Z

server/etcdserver/api/etcdhttp/health.go

+	rootHealthCheckGauge = prometheus.NewGaugeVec(prometheus.GaugeOpts{
+		Namespace: "etcd",
+		Subsystem: "server",
+		Name:      "root_health",
+		Help:      "This metric records the result of the root readyz/livez check.",
+	},
+		[]string{"type"},
+	)


Why do we need this root metric? Can't we just compute this from healthCheckGauge?

The user would need to know that min of all healthCheckGauge gives you the overall health gauge. That might be inconvenient or error prone.

It's pretty easy to do that in a prometheus query.. That's exactly what labels are meant for.

ok. Removed rootHealthCheckGauge.

logicalhan · 2023-10-19T16:53:13Z

server/etcdserver/api/etcdhttp/health.go

 	})
+	rootHealthCheckCounter = prometheus.NewCounterVec(prometheus.CounterOpts{


same concern here.

I think the accum counter can not be easily be derived from individual checks. The sum of all the checks depends on the number of checks there is.

I agree with Han, this is redundant with the other counter.

moved the root counts to healthCheckCounter with name="/"

server/etcdserver/api/etcdhttp/health_test.go

logicalhan

/lgtm
/approve

server/etcdserver/api/etcdhttp/health_test.go

chaochn47 · 2023-10-28T00:25:04Z

=== FAIL: integration/clientv3/examples  (0.00s)
2023/10/19 17:53:43 Running tests (examples) in dir(/tmp/etcd-integration1751247847): ...
panic: test timed out after 15m0s

It's a known flake. Could you please rebase on top of main or git commit --amend --no-edit and then push ?

@siyuanfoundation

serathius · 2023-10-28T09:12:49Z

It's a known flake. Could you please rebase on top of main or git commit --amend --no-edit and then push ?

Or ping @etcd-io/maintainers-etcd to rerun the test :P
Sorry that this is not available to etcd members, Github sucks. #15659

ahrtr · 2023-10-29T16:01:43Z

server/etcdserver/api/etcdhttp/health.go

@@ -241,7 +268,7 @@ func (reg *CheckRegistry) InstallHttpEndpoints(lg *zap.Logger, mux *http.ServeMu
 }

 func (reg *CheckRegistry) runHealthChecks(ctx context.Context, checkNames ...string) Health {
-	h := Health{Health: "true"}
+	h := Health{Status: HealthStatusSuccess}


Let's not to break the existing /health endpoint in 3.6.

Suggested change

h := Health{Status: HealthStatusSuccess}

h := Health{Health: "true", Status: HealthStatusSuccess}

this is not used in /health endpoint. It is only used for livez/healthz.

ahrtr · 2023-10-29T16:02:53Z

server/etcdserver/api/etcdhttp/health.go

@@ -250,9 +277,11 @@ func (reg *CheckRegistry) runHealthChecks(ctx context.Context, checkNames ...str
 		}
 		if err := check(ctx); err != nil {
 			fmt.Fprintf(&individualCheckOutput, "[-]%s failed: %v\n", checkName, err)
-			h.Health = "false"


ditto, let's keep it so as not to break the existing /health in 3.6.

this is not used in /health endpoint. It is only used for livez/healthz.

server/etcdserver/api/etcdhttp/health.go

ahrtr · 2023-10-29T16:25:15Z

server/etcdserver/api/etcdhttp/health.go

 	Health string `json:"health"`
 	Reason string `json:"reason"`
+	// Status field is used in new /readyz or /livez health checks instead of the Health field.
+	Status string `json:"-"`


Did you intentionally use "-"?

Suggested change

Status string `json:"-"`

Status string `json:"status"`

This is ignored because I don't want to change the response of existing /health endpoint. If use json:"status", the response would add an empty status field.

Why introduce this field Status? Why not to reuse the existing Health to check whether the health check is successful or failed? If you are planning to remove the fields "Health" and "Reason" and the legacy health check endpoint /health in future release e.g. 4.0, please add a comment.

We still would like to use the Health struct. But the Health string field is itself not very descriptive and not using constant strings. Added a comment about deprecation.

Can we have separate Health struct for new endpoints?
Old will use Health and Reason fields, while new will use Reason and Status.

Old will use Health and Reason fields, while new will use Reason and Status.

Sounds good to me.

Added a new HealthStatus struct.

server/etcdserver/api/etcdhttp/health.go

Signed-off-by: Siyuan Zhang <[email protected]>

serathius · 2023-11-15T09:00:08Z

server/etcdserver/api/etcdhttp/health.go

 }

 // newHealthHandler generates a http HandlerFunc for a health check function hfunc.
-func newHealthHandler(path string, lg *zap.Logger, hfunc func(*http.Request) Health) http.HandlerFunc {
+func newHealthHandler(path string, lg *zap.Logger, hfunc func(*http.Request) HealthStatus) http.HandlerFunc {


Should path be replaced with checkType ? It would make more sense that health probe logs emit the checkType instead of the path.

The path seems better here, because it may be

a root path something like /livez or /readyz;

a subpath something like /livez/serializable_read.

Paths are useful for HTTP access level logging,

ahrtr

lgtm

Thank you!

chaochn47 reviewed Oct 18, 2023

View reviewed changes

siyuanfoundation force-pushed the metrics branch 4 times, most recently from 703ad1d to 83858b6 Compare October 18, 2023 22:04

logicalhan reviewed Oct 18, 2023

View reviewed changes

siyuanfoundation force-pushed the metrics branch from 83858b6 to 72dec35 Compare October 19, 2023 16:51

logicalhan reviewed Oct 19, 2023

View reviewed changes

siyuanfoundation force-pushed the metrics branch from 72dec35 to 0ee14c2 Compare October 19, 2023 17:24

logicalhan reviewed Oct 19, 2023

View reviewed changes

server/etcdserver/api/etcdhttp/health_test.go Outdated Show resolved Hide resolved

server/etcdserver/api/etcdhttp/health_test.go Outdated Show resolved Hide resolved

siyuanfoundation force-pushed the metrics branch from 0ee14c2 to 92ab4d4 Compare October 19, 2023 17:45

logicalhan approved these changes Oct 19, 2023

View reviewed changes

server/etcdserver/api/etcdhttp/health_test.go Show resolved Hide resolved

ahrtr reviewed Oct 29, 2023

View reviewed changes

siyuanfoundation force-pushed the metrics branch from 806e2ca to a572bf4 Compare October 30, 2023 15:54

siyuanfoundation force-pushed the metrics branch 2 times, most recently from bd3dbc4 to 8f3c923 Compare November 10, 2023 21:56

serathius reviewed Nov 11, 2023

View reviewed changes

server/etcdserver/api/etcdhttp/health.go Outdated Show resolved Hide resolved

serathius reviewed Nov 11, 2023

View reviewed changes

server/etcdserver/api/etcdhttp/health.go Outdated Show resolved Hide resolved

siyuanfoundation force-pushed the metrics branch from 8f3c923 to f3c6db5 Compare November 13, 2023 17:57

serathius reviewed Nov 13, 2023

View reviewed changes

server/etcdserver/api/etcdhttp/health.go Outdated Show resolved Hide resolved

serathius reviewed Nov 13, 2023

View reviewed changes

server/etcdserver/api/etcdhttp/health.go Outdated Show resolved Hide resolved

siyuanfoundation force-pushed the metrics branch from c0579ea to 35b0ca3 Compare November 13, 2023 20:05

serathius reviewed Nov 14, 2023

View reviewed changes

server/etcdserver/api/etcdhttp/health.go Outdated Show resolved Hide resolved

etcdserver: add metric counters for livez/readyz health checks.

3897103

Signed-off-by: Siyuan Zhang <[email protected]>

siyuanfoundation force-pushed the metrics branch from 35b0ca3 to 3897103 Compare November 14, 2023 17:26

serathius reviewed Nov 15, 2023

View reviewed changes

serathius approved these changes Nov 15, 2023

View reviewed changes

ahrtr approved these changes Nov 15, 2023

View reviewed changes

ahrtr merged commit d0114cf into etcd-io:main Nov 15, 2023
33 checks passed

siyuanfoundation deleted the metrics branch November 15, 2023 23:15

siyuanfoundation mentioned this pull request Dec 6, 2023

[3.5] Backport livez/readyz #17039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdserver: add metric counters for livez/readyz health checks. #16797

etcdserver: add metric counters for livez/readyz health checks. #16797

siyuanfoundation commented Oct 18, 2023 •

edited

Loading

siyuanfoundation commented Oct 18, 2023

logicalhan Oct 18, 2023

siyuanfoundation Oct 19, 2023

logicalhan Oct 19, 2023

siyuanfoundation Oct 19, 2023

logicalhan Oct 19, 2023

siyuanfoundation Oct 19, 2023

dgrisonnet Oct 19, 2023

siyuanfoundation Oct 19, 2023

logicalhan left a comment

chaochn47 commented Oct 28, 2023

serathius commented Oct 28, 2023 •

edited

Loading

ahrtr Oct 29, 2023

siyuanfoundation Oct 30, 2023

ahrtr Oct 29, 2023

siyuanfoundation Oct 30, 2023

ahrtr Oct 29, 2023

siyuanfoundation Oct 30, 2023

ahrtr Nov 10, 2023

siyuanfoundation Nov 10, 2023

serathius Nov 11, 2023

ahrtr Nov 11, 2023

siyuanfoundation Nov 13, 2023

serathius Nov 15, 2023 •

edited

Loading

ahrtr Nov 15, 2023

serathius Nov 15, 2023

ahrtr left a comment

		})
		rootHealthCheckCounter = prometheus.NewCounterVec(prometheus.CounterOpts{

	h := Health{Status: HealthStatusSuccess}
	h := Health{Health: "true", Status: HealthStatusSuccess}

etcdserver: add metric counters for livez/readyz health checks. #16797

etcdserver: add metric counters for livez/readyz health checks. #16797

Conversation

siyuanfoundation commented Oct 18, 2023 • edited Loading

siyuanfoundation commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logicalhan left a comment

Choose a reason for hiding this comment

chaochn47 commented Oct 28, 2023

serathius commented Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

siyuanfoundation commented Oct 18, 2023 •

edited

Loading

serathius commented Oct 28, 2023 •

edited

Loading

serathius Nov 15, 2023 •

edited

Loading