-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat_: batch all telemetry data and send request every 10 seconds #5251
Conversation
Jenkins BuildsClick to see older builds (46)
|
28b4307
to
32ea02b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR!
Just left few comments and questions for your consideration
wakuv2/telemetry.go
Outdated
@@ -50,5 +50,7 @@ func (c *BandwidthTelemetryClient) PushProtocolStats(relayStats metrics.Stats, s | |||
_, err := c.httpClient.Post(url, "application/json", bytes.NewBuffer(body)) | |||
if err != nil { | |||
c.logger.Error("Error sending message to telemetry server", zap.Error(err)) | |||
} else { | |||
c.logger.Debug("Successfully pushed protocol stats to telemetry server") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be too noisy since this is the expected result. Consider not logging this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed in latest commit
telemetry/client.go
Outdated
@@ -96,5 +233,7 @@ func (c *Client) UpdateEnvelopeProcessingError(shhMessage *types.Message, proces | |||
_, err := c.httpClient.Post(url, "application/json", bytes.NewBuffer(body)) | |||
if err != nil { | |||
c.logger.Error("Error sending envelope update to telemetry server", zap.Error(err)) | |||
} else { | |||
c.logger.Debug("Successfully pushed envelope processing error to telemetry server", zap.String("hash", types.EncodeHex(shhMessage.Hash))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be too noisy due to it being the expected result. Consider not logging this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed in latest commit
telemetry/client.go
Outdated
}() | ||
} | ||
|
||
func (c *Client) Start() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pass a context to the Start() function?
We could use it for killing the go routine once logout happens, i.e.:
select {
...
case <-ctx.Done():
return
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in latest commit
telemetry/client.go
Outdated
} | ||
} | ||
|
||
func (c *Client) CollectAndProcessTelemetry() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should accept a context here too to exit the goroutine on logout and avoid having a dangling goroutine? (The messenger should have a m.ctx
context you could use)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in latest commit
telemetry/client.go
Outdated
} | ||
|
||
func (c *Client) PushReceivedMessages(filter transport.Filter, sshMessage *types.Message, messages []*v1protocol.StatusMessage) { | ||
func (c *Client) pushTelemetryRequest(request []TelemetryRequest) { | ||
c.logger.Debug("Pushing telemetry data", zap.Any("request", request)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider not logging this unless it's strictly necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed in latest commit
2982f05
to
5f3eac3
Compare
be1081a
to
60d38d2
Compare
60d38d2
to
8582881
Compare
Instead of making an http request for every individual telemetric, push each to a channel and periodically batch in a single request.
A description to understand introduced changes without reading the code.
Important changes:
Closes #5240