app: push readyz in background #867

dB2510 · 2022-07-27T13:43:45Z

This change pushed readyz metrics every 1 second. Earlier we were pushing this metric only when someone calls readyz endpoint. If no-one calls that endpoint it will show inactive on readyz grafana panel.

category: bug
ticket: #880

dB2510 · 2022-07-27T13:44:23Z

app/monitoringapi.go

@@ -73,6 +73,24 @@ func wireMonitoringAPI(life *lifecycle.Manager, addr string, localNode *enode.Lo
 	life.RegisterStart(lifecycle.AsyncBackground, lifecycle.StartMonitoringAPI, httpServeHook(server.ListenAndServe))
 	life.RegisterStop(lifecycle.StopMonitoringAPI, lifecycle.HookFunc(server.Shutdown))

+	go func() {
+		ticker := time.NewTicker(time.Second)


we can discuss on the duration part, what will be the right duration to push metrics

metrics are not pushed, they are pull. So you do not need to update more than pull period, which is 15s. Suggest making this 10s.

dB2510 · 2022-07-27T13:44:46Z

app/monitoringapi.go

+			case <-ctx.Done():
+				return
+			case <-ticker.C:
+				syncing, err := beaconNodeSyncing(ctx, eth2Cl)


avoided logging errors here, since that would spam the logs

corverroos · 2022-07-27T13:46:29Z

app/monitoringapi.go

@@ -73,6 +73,24 @@ func wireMonitoringAPI(life *lifecycle.Manager, addr string, localNode *enode.Lo
 	life.RegisterStart(lifecycle.AsyncBackground, lifecycle.StartMonitoringAPI, httpServeHook(server.ListenAndServe))
 	life.RegisterStop(lifecycle.StopMonitoringAPI, lifecycle.HookFunc(server.Shutdown))

+	go func() {


suggest extracting a function isReady(ctx) bool which you call here and in newReadyHandler

Also add a test for isReady please 🙏

Maybe we should ONLY have a async "isReady" resolver, and then readyz just reads the latest value..?

codecov · 2022-07-27T13:47:30Z

Codecov Report

Merging #867 (ced872e) into main (2237124) will decrease coverage by 0.03%.
The diff coverage is 58.73%.

❗ Current head ced872e differs from pull request most recent head b8a97a0. Consider uploading reports for the commit b8a97a0 to get more accurate results

@@            Coverage Diff             @@
##             main     #867      +/-   ##
==========================================
- Coverage   54.73%   54.69%   -0.04%     
==========================================
  Files         113      114       +1     
  Lines       12079    12265     +186     
==========================================
+ Hits         6611     6708      +97     
- Misses       4505     4587      +82     
- Partials      963      970       +7

Impacted Files	Coverage Δ
app/app.go	`56.01% <0.00%> (-0.70%)`	⬇️
app/monitoringapi.go	`66.92% <59.67%> (+37.89%)`	⬆️
core/signeddata.go	`41.66% <0.00%> (-2.05%)`	⬇️
cmd/createcluster.go	`49.43% <0.00%> (-1.69%)`	⬇️
testutil/beaconmock/beaconmock.go	`62.65% <0.00%> (-1.55%)`	⬇️
p2p/discovery.go	`47.88% <0.00%> (-1.39%)`	⬇️
core/leadercast/transport.go	`75.14% <0.00%> (-1.19%)`	⬇️
core/tracker/tracker.go	`59.76% <0.00%> (-0.72%)`	⬇️
testutil/beaconmock/options.go	`43.49% <0.00%> (-0.42%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2237124...b8a97a0. Read the comment docs.

dB2510 · 2022-07-28T11:55:43Z

app/monitoringapi.go

 	)
 	for {
 		select {
 		case <-ctx.Done():
 			return ctx.Err()
 		case res := <-results:
 			if res.Error != nil {
-				continue
+				errCount++


added error counter to return error if are unable to ping quorum number of peers

corverroos · 2022-07-28T14:40:04Z

app/monitoringapi.go

+			case <-ctx.Done():
+				return
+			case <-ticker.Chan():
+				mu.Lock()


remember to decrease mutex locked scope to absolute minimum, and remember to NEVER do IO (disk/network) calls while a lock is held.

corverroos · 2022-07-28T14:43:58Z

app/monitoringapi.go

+		actual   int
+		errCount int


suggest: okCount, errCount

corverroos · 2022-07-28T14:50:12Z

app/monitoringapi_internal_test.go

+
+	// We wrap the Advance() calls with blockers to make sure that the ticker
+	// can go to sleep and produce ticks without time passing in parallel.
+	clock.BlockUntil(1)


BlockUntil only works for calls to Sleep and After, not Timer.Chan().. so you'll need to another way.

corverroos · 2022-07-28T14:50:38Z

app/monitoringapi_internal_test.go

+	}
+}
+
+func TestStartCheckerPingFail(t *testing.T) {


lots of duplication in these tests, can't we make it a table test rather?

corverroos · 2022-07-28T14:51:59Z

app/monitoringapi.go

+					readyErr = errors.New("beacon node not synced")
+					readyzGauge.Set(0)
+				} else if peersReady(ctx, peerIDs, tcpNode) != nil {
+					readyErr = errors.New("couldn't ping all peers")


suggest using error sentinels (var errReadyNoPing = errors.New("blah blah")) and use them for comparison in tests

corverroos · 2022-07-29T10:50:32Z

app/monitoringapi.go

+	readyErrFunc := startReadyChecker(ctx, tcpNode, eth2Cl, peerIDs, clockwork.NewRealClock())
+	mux.HandleFunc("/readyz", func(w http.ResponseWriter, r *http.Request) {
+		readyErr := readyErrFunc()
+		if readyErr != nil {


yeah but i avoided this because it needs to send response as error string which will result in calling readErrFunc() twice and again asking for mutex.

corverroos · 2022-07-29T10:51:36Z

app/monitoringapi.go

+func startReadyChecker(ctx context.Context, tcpNode host.Host, eth2Cl eth2client.NodeSyncingProvider, peerIDs []peer.ID, clock clockwork.Clock) func() error {
+	var (
+		mu       sync.Mutex
+		readyErr error


suggest initialising the error with non-nil, "errReadyUninit = "ready check uninitialised"

corverroos · 2022-07-29T10:53:18Z

app/monitoringapi.go

+					mu.Lock()
+					readyErr = errReadySyncing
+					mu.Unlock()


rather do locking in one place.

suggest:

... } else if syncing { err = errReadySyncing readyzGauge.Set(0) } ... mu.Lock() readyErr = err mu.Unlock()

push readyz metrics every second

73dd6a3

dB2510 commented Jul 27, 2022

View reviewed changes

corverroos approved these changes Jul 27, 2022

View reviewed changes

dB2510 added 3 commits July 28, 2022 15:37

add start checker and tests

bb2e7e5

cleanup

5665a2d

cleanup

f3aeeb4

dB2510 commented Jul 28, 2022

View reviewed changes

corverroos reviewed Jul 28, 2022

View reviewed changes

feedback

ced872e

corverroos approved these changes Jul 29, 2022

View reviewed changes

feedback

0baae16

dB2510 added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jul 29, 2022

dB2510 linked an issue Jul 29, 2022 that may be closed by this pull request

Update readyz gauge asynchronously #880

Closed

cleanup

b8a97a0

obol-bulldozer bot merged commit a17bbb4 into main Jul 29, 2022

obol-bulldozer bot deleted the dhruv/callreadyz branch July 29, 2022 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

app: push readyz in background #867

app: push readyz in background #867

dB2510 commented Jul 27, 2022 •

edited

Loading

dB2510 Jul 27, 2022

corverroos Jul 27, 2022

dB2510 Jul 27, 2022

corverroos Jul 27, 2022 •

edited

Loading

corverroos Jul 27, 2022 •

edited

Loading

corverroos Jul 27, 2022

codecov bot commented Jul 27, 2022 •

edited

Loading

dB2510 Jul 28, 2022

corverroos Jul 28, 2022

corverroos Jul 28, 2022

corverroos Jul 28, 2022

corverroos Jul 28, 2022

dB2510 Jul 28, 2022

corverroos Jul 28, 2022

corverroos Jul 29, 2022

dB2510 Jul 29, 2022 •

edited

Loading

corverroos Jul 29, 2022

corverroos Jul 29, 2022

app: push readyz in background #867

app: push readyz in background #867

Conversation

dB2510 commented Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

corverroos Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 27, 2022 • edited Loading

Codecov Report

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

dB2510 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dB2510 commented Jul 27, 2022 •

edited

Loading

corverroos Jul 27, 2022 •

edited

Loading

corverroos Jul 27, 2022 •

edited

Loading

codecov bot commented Jul 27, 2022 •

edited

Loading

dB2510 Jul 29, 2022 •

edited

Loading