Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
pkg/api/status.go
Outdated
| LastSyncedBlock uint64 `json:"lastSyncedBlock"` | ||
| CommittedDepth uint8 `json:"committedDepth"` | ||
| IsWarmingUp bool `json:"isWarmingUp"` | ||
| Metrics map[string]string `json:"metrics,omitempty"` |
There was a problem hiding this comment.
Maybe metrics should not be exposed through the API? The response is cluttered with encoded metrics data that users can see for their own node on /metrics, and for peer nodes, maybe these metrics are not so relevant. Should we remove this field here?
There was a problem hiding this comment.
If you remove them here, what would be the options then?
There was a problem hiding this comment.
The status protocol will still exchange these metrics, but they will not be exposed here, only to be gathered over p2p network, which is sufficient.
There was a problem hiding this comment.
The status protocol will still exchange these metrics, but they will not be exposed here, only to be gathered over p2p network, which is sufficient.
I like this approach
There was a problem hiding this comment.
I totally agree. If we really need it, we can expose additional endpoint just for this case. But I don’t see a real reason to do so.
| reserve Reserve | ||
| sync SyncReporter | ||
| chainState postage.ChainStateGetter | ||
| metricsRegistry *prometheus.Registry |
There was a problem hiding this comment.
The above protocolVersion has to be incremented for this change, I think.
pkg/api/api.go
Outdated
| } | ||
| } | ||
|
|
||
| func (s *Service) downloadSpeedMetricMiddleware() func(h http.Handler) http.Handler { |
There was a problem hiding this comment.
I think we could add a string argument to the method, which would serve as a label for metrics. This way, we can easily distinguish between 'bzz' and 'bytes' in the endpoint data, because the download speed depends on the logic behind the handlers, and the logic in 'bzz' is more complex than in 'bytes'.
3 participants
Checklist
Description
This PR adds metrics to the status protocol for analysing upload and download speeds on different nodes, as well metrics suggested in comments in #5005.
This approach makes it easier to register metrics tat are exposed on the status protocol, by using metrics registry and rendering all metrics as a prometheus formatted strings in a map on a single field of the status protocol's Snapshot message.
Upload and download speed metrics
Upload and download metrics for speed are new ones so that they are measuring only the /bzz uploads and downloads, not /bytes or any other POST methods, making them more specific than content api duration metric which aggregates measurements from all endpoints into one metric.
Additional changes
HeadersExchangeDuration metric now measures only the time of the headers exchange, not a larger scope of functions.
Prometheus go client is upgraded to use a newer encoding type definition. The encoding is actually the same, just the go api has changed.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):