chore: refactor types, error handling, clean up endpoints #37

vpavlin · 2024-08-07T08:07:53Z

extracted the telemetry types into a separate package
added a simple struct to unify the errors
fixed file naming for receivedenvelope and the test
added new "wrapping" types in the telemetry package and moved json.Unmarshal to process methods to de-bloat server.go
removed endpoints I did not find being used anywhere

TODO:

make sure the ErrorDetail is used everywhere
confirm the removed endpoints were actually unused:)

richard-ramos

LGTM. Just left few questions

richard-ramos · 2024-08-07T20:20:05Z

telemetry/server.go

 	server.Router.HandleFunc("/protocol-stats", server.createProtocolStats).Methods("POST")
-	server.Router.HandleFunc("/received-messages", server.createReceivedMessages).Methods("POST")
-	server.Router.HandleFunc("/waku-metrics", server.createWakuTelemetry).Methods("POST")
-	server.Router.HandleFunc("/received-envelope", server.createReceivedEnvelope).Methods("POST")
-	server.Router.HandleFunc("/sent-envelope", server.createSentEnvelope).Methods("POST")
 	server.Router.HandleFunc("/update-envelope", server.updateEnvelope).Methods("POST")


Cleaning up these endpoints mean that this is a breaking change, since clients out there will no longer be able to push metrics. Do we want to allow backwards compatibility for telemetry?

Yeah, good question. I was not sure if they are still actively used. Happy to bring them back if you think it would break things

Not sure tbh.
I think it would depend whether these changes will be available in status release 2.30 or not, and if not, if we care about the telemetry data from 2.30 users

richard-ramos · 2024-08-07T20:25:03Z

telemetry/peer_count.go

 type PeerCount struct {
-	ID            int    `json:"id"`
-	CreatedAt     int64  `json:"createdAt"`
-	Timestamp     int64  `json:"timestamp"`
-	NodeName      string `json:"nodeName"`
-	NodeKeyUid    string `json:"nodeKeyUid"`
-	PeerID        string `json:"peerId"`
-	PeerCount     int    `json:"peerCount"`
-	StatusVersion string `json:"statusVersion"`
+	data types.PeerCount


Instead of creating a data attribute, wdyt about using

type PeerCount struct { types.PeerCount

And the same for PeerConnFailure:

type PeerConnFailure struct { types.PeerCount

I might be wrong but I think this would let you access the attributes directly, i.e:
myPeerConnFailure.Timestamp, instead of myPeerConnFailure.data.Timestamp

Sadly, not the case, I'd have to implement the getters for the values - calling methods on composed types works, but to access the attributes, you need to do something like

type PeerCount struct { types.PeerCount } func (r *PeerCount) process(db *sql.DB, errs *MetricErrors, data *types.TelemetryRequest) error { ... lastInsertId := 0 err = stmt.QueryRow( r.PeerCount.Timestamp, ...

So it is not really solving much:( The only benefit is that I can use json.Unmarshal(*data.TelemetryData, &r); instead of &r.data, but that seems to be the only benefit

richard-ramos · 2024-08-07T20:29:45Z

telemetry/server.go

-			if err := json.Unmarshal(*data.TelemetryData, &stats); err != nil {
-				errorDetails = append(errorDetails, map[string]interface{}{"Id": data.Id, "Error": fmt.Sprintf("Error decoding protocol stats: %v", err)})
+			err := stats.process(s.DB, &errorDetails, &data)
+			if err != nil {


Should we log the err? Same question for lines 81 87 93 99 105 and 111, or is the log in line 120 enough?

I could also add logging the error to the errors.append method to make sure every new error is logged?

Tried, this would result in telemetry logs like the following, and we could remove the log on line 120 to not duplicate

2024-08-08T10:28:43.752+0200 ERROR telemetry/errors.go:28 Error saving peer count: pq: duplicate key value violates unique constraint "peercount_unique" github.com/status-im/dev-telemetry/telemetry.(*MetricErrors).Append /home/vpavlin/devel/github.com/vpavlin/telemetry/telemetry/errors.go:28 github.com/status-im/dev-telemetry/telemetry.(*PeerCount).process /home/vpavlin/devel/github.com/vpavlin/telemetry/telemetry/peer_count.go:41 github.com/status-im/dev-telemetry/telemetry.(*Server).createTelemetryData /home/vpavlin/devel/github.com/vpavlin/telemetry/telemetry/server.go:99 net/http.HandlerFunc.ServeHTTP /usr/lib/go/src/net/http/server.go:2136 github.com/rs/cors.(*Cors).Handler-fm.(*Cors).Handler.func1 /home/vpavlin/devel/pkg/mod/github.com/rs/[email protected]/cors.go:289 net/http.HandlerFunc.ServeHTTP /usr/lib/go/src/net/http/server.go:2136 github.com/status-im/dev-telemetry/telemetry.(*Server).rateLimit-fm.(*Server).rateLimit.func1 /home/vpavlin/devel/github.com/vpavlin/telemetry/telemetry/server.go:222 net/http.HandlerFunc.ServeHTTP /usr/lib/go/src/net/http/server.go:2136 github.com/gorilla/mux.(*Router).ServeHTTP /home/vpavlin/devel/pkg/mod/github.com/gorilla/[email protected]/mux.go:210 net/http.serverHandler.ServeHTTP /usr/lib/go/src/net/http/server.go:2938 net/http.(*conn).serve /usr/lib/go/src/net/http/server.go:2009

richard-ramos · 2024-08-07T20:31:47Z

telemetry/peer_count.go

 	if err != nil {
+		errs.Append(data.Id, fmt.Sprintf("Error saving peer count: %v", err))


I wonder if we should return the details of the error to the user, or if instead we should log the err, and just return error saving peer count.

I guess this depends on where is it better for us to see the errors: clients running status-go, the telemetry server, or both? I think we're most likely to look at the status-go logs from running status-desktop

Yeah, it is a tough question - returning the error leaks implementation details to clients - a) they usually don't need to know as they cannot do anything about it, b) it might be dangerous from the security perspective

That said, we'd need to make sure we have good access to Telemetry server logs otherwise to be able to spot the errors and reasons there.

Also, I would prefer not change it in this PR - how about a new issue "Review error handling and reporting in telemetry server and client"?

That said, we'd need to make sure we have good access to Telemetry server logs otherwise to be able to spot the errors and reasons there.

This is possible to do via kibana: https://kibana.infra.status.im/app/discover#/?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))&_a=(columns:!(),filters:!(('$state':(store:appState),meta:(alias:!n,disabled:!f,index:ffcc22b0-0116-11ed-9719-cdd3b483481c,key:logsource,negate:!f,params:(query:node-01.he-eu-hel1.tmetry.misc),type:phrase),query:(match_phrase:(logsource:node-01.he-eu-hel1.tmetry.misc))),('$state':(store:appState),meta:(alias:!n,disabled:!f,index:ffcc22b0-0116-11ed-9719-cdd3b483481c,key:program,negate:!f,params:(query:docker%2Ftelemetry-srv),type:phrase),query:(match_phrase:(program:docker%2Ftelemetry-srv)))),index:ffcc22b0-0116-11ed-9719-cdd3b483481c,interval:auto,query:(language:kuery,query:''),sort:!(!('@timestamp',desc)))

I would prefer not change it in this PR - how about a new issue "Review error handling and reporting in telemetry server and client"?

Sure. Can be done in a separate PR. Will open an issue so we dont forget

richard-ramos · 2024-08-07T20:33:09Z

telemetry/server.go

+	peerIDHash := sha256.Sum256([]byte(protocolStats.data.PeerID))
+	protocolStats.data.PeerID = hex.EncodeToString(peerIDHash[:])


Remembering past discussions, maybe we should just store the peerID without hashing it? wdyt?

I did not want to change it here to avoid bringing undrelated changes, but can do if it is pereferred:)

Can be done either on a diff PR or in a diff commit! no preferences here either 🚀

vpavlin · 2024-08-12T14:17:33Z

telemetry/ratelimiter.go

@@ -99,7 +99,8 @@ func (rl *RateLimiter) cleanup(ctx context.Context, cleanupEvery time.Duration)
 			return
 		case now := <-t.C:
 			numCleaned := 0
-			for ip, limiter := range rl.values() {
+			limiters := rl.values()


@richard-ramos unrelated to the refactoring, but the CI failed, so I added the fix (I hope this should fix it?)

https://github.com/status-im/telemetry/actions/runs/10353387310/job/28656165988?pr=37

vpavlin requested review from adklempner and richard-ramos August 7, 2024 11:33

richard-ramos approved these changes Aug 7, 2024

View reviewed changes

vpavlin mentioned this pull request Aug 8, 2024

chore: use imported telemetry types, clean bandwith metrics status-im/status-go#5674

Open

5 tasks

richard-ramos mentioned this pull request Aug 8, 2024

Review error handling and reporting in telemetry server and client #39

Open

This was referenced Aug 12, 2024

Use embedded types for metrics #40

Open

Review PeerId hashing for telemetry #41

Closed

vpavlin force-pushed the chore/extract-types branch from d8b1e11 to 7444689 Compare August 12, 2024 13:59

vpavlin added 3 commits August 12, 2024 16:02

chore: refactor types, error handling, clean up endpoints

962845f

fix: add missing ReceivedMessages processing

41ebc2f

fix: log each processing error separately

9d80407

vpavlin force-pushed the chore/extract-types branch from 7444689 to 9d80407 Compare August 12, 2024 14:02

fix: concurrent map iteration and map write

dce9202

vpavlin commented Aug 12, 2024

View reviewed changes

adklempner approved these changes Aug 12, 2024

View reviewed changes

vpavlin merged commit 63f5395 into status-im:master Aug 13, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor types, error handling, clean up endpoints #37

chore: refactor types, error handling, clean up endpoints #37

vpavlin commented Aug 7, 2024

richard-ramos left a comment

richard-ramos Aug 7, 2024

vpavlin Aug 8, 2024

richard-ramos Aug 8, 2024

richard-ramos Aug 7, 2024

vpavlin Aug 8, 2024 •

edited

Loading

richard-ramos Aug 8, 2024

richard-ramos Aug 7, 2024

vpavlin Aug 8, 2024

vpavlin Aug 8, 2024

richard-ramos Aug 7, 2024

adklempner Aug 8, 2024

vpavlin Aug 8, 2024

richard-ramos Aug 8, 2024

richard-ramos Aug 8, 2024

richard-ramos Aug 7, 2024

vpavlin Aug 8, 2024

richard-ramos Aug 8, 2024

vpavlin Aug 12, 2024 •

edited

Loading

		if err != nil {
		errs.Append(data.Id, fmt.Sprintf("Error saving peer count: %v", err))

		peerIDHash := sha256.Sum256([]byte(protocolStats.data.PeerID))
		protocolStats.data.PeerID = hex.EncodeToString(peerIDHash[:])

chore: refactor types, error handling, clean up endpoints #37

chore: refactor types, error handling, clean up endpoints #37

Conversation

vpavlin commented Aug 7, 2024

richard-ramos left a 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

vpavlin Aug 8, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpavlin Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

vpavlin Aug 8, 2024 •

edited

Loading

vpavlin Aug 12, 2024 •

edited

Loading