feat_: add ability to process prometheus metrics in telemetry client #5782

vpavlin · 2024-08-29T13:34:00Z

This PR adds a general avility to telemetry client to work with Prometheus metrics exposed by status-go and underlying libraries.

It then uses an extended waku2_envelopes_validated_total (the extension is storing pubsub topic and type in labels, which allows us to filter for messages received due to regular store queries - status-im/telemetry#23) metric to showcase how this new feature works besically snapshoting and filtering recorded values for that metric based on MessageType = missing and then adding it to the telemetry request cache (which is regularly published to telemetry service)

This allows us to add new metrics via Prometheus - hence make them standardized reusable (and also tap into metrics provided by libraries - e.g. go-waku)

This is still a draft or the most part

status-im-auto · 2024-08-29T13:39:10Z

Jenkins Builds

Click to see older builds (13)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`e8f9704`	#1	2024-08-29 13:39:10	~4 min	`ios`	📦`zip`
✖️	`e8f9704`	#1	2024-08-29 13:41:29	~7 min	`tests`	📄`log`
✔️	`e8f9704`	#1	2024-08-29 13:41:57	~7 min	`linux`	📦`zip`
✔️	`e8f9704`	#2	2024-08-29 13:42:51	~3 min	`ios`	📦`zip`
✔️	`e8f9704`	#1	2024-08-29 13:43:04	~8 min	`tests-rpc`	📄`log`
✔️	`e8f9704`	#1	2024-08-29 13:43:37	~9 min	`android`	📦`aar`
✔️	`e8f9704`	#2	2024-08-29 13:44:10	~1 min	`linux`	📦`zip`
✔️	`e8f9704`	#2	2024-08-29 13:45:17	~1 min	`android`	📦`aar`

✔️	`acf238e`	#2	2024-09-02 14:14:08	~2 min	`tests-rpc`	📄`log`
✖️	`acf238e`	#2	2024-09-02 14:15:21	~3 min	`tests`	📄`log`
✔️	`acf238e`	#3	2024-09-02 14:15:55	~4 min	`linux`	📦`zip`
✔️	`acf238e`	#3	2024-09-02 14:17:04	~5 min	`android`	📦`aar`
✔️	`acf238e`	#3	2024-09-02 14:17:12	~5 min	`ios`	📦`zip`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✖️	`4912bcb`	#3	2024-09-02 16:12:18	~1 min	`tests`	📄`log`
✔️	`4912bcb`	#4	2024-09-02 16:13:24	~2 min	`android`	📦`aar`
✔️	`4912bcb`	#3	2024-09-02 16:13:25	~2 min	`tests-rpc`	📄`log`
✔️	`4912bcb`	#4	2024-09-02 16:14:15	~3 min	`ios`	📦`zip`
✔️	`4912bcb`	#4	2024-09-02 16:14:54	~3 min	`linux`	📦`zip`

✖️	`722d47e`	#4	2024-09-03 10:52:52	~1 min	`tests`	📄`log`
✔️	`722d47e`	#5	2024-09-03 10:53:41	~2 min	`android`	📦`aar`
✔️	`722d47e`	#4	2024-09-03 10:54:15	~2 min	`tests-rpc`	📄`log`
✔️	`722d47e`	#5	2024-09-03 10:55:41	~4 min	`linux`	📦`zip`
✔️	`722d47e`	#5	2024-09-03 10:57:56	~6 min	`ios`	📦`zip`

vpavlin · 2024-08-29T15:32:03Z

telemetry/client.go

+func (c *Client) ProcessReuglarStoryRetrievedMsgs(data MetricPayload) {
+	fmt.Println(data)
+
+	postBody := map[string]interface{}{


One thing to consider is to drop any processing on client and just push the metric as it is structured by Prometheus to the Telemetry server - then we can do whatever we want/need with it on the server - simplifies client quite a lot

That would be great

adklempner · 2024-08-29T15:55:25Z

telemetry/client.go

+		for {
+			select {
+			case <-ctx.Done():
+				fmt.Println("exit")


should probably remove

adklempner · 2024-08-29T15:57:49Z

telemetry/client.go

+func (c *Client) ProcessReuglarStoryRetrievedMsgs(data MetricPayload) {
+	fmt.Println(data)
+
+	postBody := map[string]interface{}{


That would be great

adklempner · 2024-08-29T15:58:30Z

telemetry/client.go

@@ -413,3 +450,26 @@ func (c *Client) UpdateEnvelopeProcessingError(shhMessage *types.Message, proces
 		c.logger.Error("Error sending envelope update to telemetry server", zap.Error(err))
 	}
 }
+
+func (c *Client) ProcessReuglarStoryRetrievedMsgs(data MetricPayload) {


should this be ProcessRegularStoreRetrievedMsgs?

adklempner · 2024-08-29T16:09:28Z

wakuv2/waku.go


 		w.wg.Add(1)
 		go func() {
-			w.wg.Done()
+			defer w.wg.Done()


not sure how this wait group is intended to behave cc @richard-ramos

Adding the defer is correct. This is a bug i introduced when doing the refactoring

richard-ramos · 2024-08-29T18:06:55Z

telemetry/client.go

@@ -180,6 +193,8 @@ func (c *Client) Start(ctx context.Context) {
 				c.telemetryCacheLock.Unlock()

 				if len(telemetryRequests) > 0 {
+					d, _ := json.MarshalIndent(telemetryRequests, "", "  ")
+					fmt.Println(string(d))


richard-ramos · 2024-08-29T18:07:04Z

telemetry/client.go

 	}

+	//client.promMetrics.Register("waku_connected_peers", GaugeType, nil, nil)


richard-ramos · 2024-08-29T18:07:27Z

telemetry/client.go

@@ -413,3 +450,26 @@ func (c *Client) UpdateEnvelopeProcessingError(shhMessage *types.Message, proces
 		c.logger.Error("Error sending envelope update to telemetry server", zap.Error(err))
 	}
 }
+
+func (c *Client) ProcessReuglarStoryRetrievedMsgs(data MetricPayload) {
+	fmt.Println(data)


richard-ramos · 2024-08-29T18:08:20Z

telemetry/prometheus.go

+	gatherer := prometheus.DefaultGatherer
+	metrics, err := gatherer.Gather()
+	if err != nil {
+		log.Fatalf("Failed to gather metrics: %v", err)


Not sure if we should use Fatalf. If i remember correctly, this panics, and IMO not being able to push metrics isn't an end of the world situation!

github-actions · 2024-09-02T16:11:20Z

Thank you for opening this pull request!

We require commits to follow the Conventional Commits, but with _ for non-breaking changes.
And it looks like your PR needs to be adjusted.

Details:

checking commits between: origin/develop HEAD
Commit message "move metric processing to server" is not well-formed
Commit message "test commit" is not well-formed

adklempner · 2024-09-07T01:13:29Z

wakuv2/common/metrics.go

@@ -52,7 +52,7 @@ var (

 func init() {
 	prom.MustRegister(EnvelopesReceivedCounter)
-	prom.MustRegister(EnvelopesRejectedCounter)


Was this metric supposed to be removed?

adklempner · 2024-09-07T02:45:38Z

wakuv2/waku.go

@@ -1347,7 +1351,7 @@ func (w *Waku) OnNewEnvelopes(envelope *protocol.Envelope, msgType common.Messag
 		trouble = true
 	}

-	common.EnvelopesValidatedCounter.Inc()
+	common.EnvelopesValidatedCounter.With(prometheus.Labels{"pubsubTopic": envelope.PubsubTopic(), "type": msgType}).Inc()


Note that we need the meaning of common.MissingMessageType to be consistent moving forward, otherwise it can mess up our metrics. i.e. only messages returned by the periodic store query can have this type

vpavlin force-pushed the feat/telemetry-prometheus branch from d10769e to e8f9704 Compare August 29, 2024 13:34

vpavlin commented Aug 29, 2024

View reviewed changes

adklempner reviewed Aug 29, 2024

View reviewed changes

vpavlin marked this pull request as draft August 29, 2024 16:10

richard-ramos reviewed Aug 29, 2024

View reviewed changes

vpavlin added 2 commits September 2, 2024 12:32

feat_: add ability to process prometheus metrics in telemetry client

7219270

fix_: move prom metric processing to server

acf238e

vpavlin force-pushed the feat/telemetry-prometheus branch from e8f9704 to acf238e Compare September 2, 2024 14:11

anastasiyaig mentioned this pull request Sep 2, 2024

chore: telemetry improvements status-im/status-desktop#16242

Closed

test commit

4912bcb

move metric processing to server

722d47e

adklempner reviewed Sep 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat_: add ability to process prometheus metrics in telemetry client #5782

feat_: add ability to process prometheus metrics in telemetry client #5782

vpavlin commented Aug 29, 2024

status-im-auto commented Aug 29, 2024 •

edited

Loading

vpavlin Aug 29, 2024

adklempner Aug 29, 2024

adklempner Aug 29, 2024

adklempner Aug 29, 2024

adklempner Aug 29, 2024

adklempner Aug 29, 2024

richard-ramos Aug 29, 2024

richard-ramos Aug 29, 2024

richard-ramos Aug 29, 2024

richard-ramos Aug 29, 2024

richard-ramos Aug 29, 2024 •

edited

Loading

github-actions bot commented Sep 2, 2024 •

edited

Loading

adklempner Sep 7, 2024

adklempner Sep 7, 2024

		}

		//client.promMetrics.Register("waku_connected_peers", GaugeType, nil, nil)

feat_: add ability to process prometheus metrics in telemetry client #5782

Are you sure you want to change the base?

feat_: add ability to process prometheus metrics in telemetry client #5782

Conversation

vpavlin commented Aug 29, 2024

status-im-auto commented Aug 29, 2024 • edited Loading

Jenkins Builds

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

richard-ramos Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

status-im-auto commented Aug 29, 2024 •

edited

Loading

richard-ramos Aug 29, 2024 •

edited

Loading

github-actions bot commented Sep 2, 2024 •

edited

Loading