-
Notifications
You must be signed in to change notification settings - Fork 18
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
WIP: Add the metrics
recording feature for libp2p
#431
Conversation
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.
Looks good. Just one question. How do we query those metrics later on? I guess we will integrate it withing the network service?
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.
Looks good, at some point it would be nice to discuss how this approach could be used for all metrics.
Ok(Swarm { swarm }) | ||
#[cfg(feature = "metrics")] | ||
let metrics = { | ||
let mut metric_registry = Registry::default(); |
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.
Is it true, that if we'll want to have one prometheus service inside the node (spawned as described in TODO comment) then metrics_registry
will need to be passed as reference instead of created here?
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.
Ah right. I’ll reopen this after checking how this can be used with the existing the metrics service. I think we cannot merge this without figuring out how to use 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.
Hm. I think that the way how libp2p gathers/exposes metrics is not flexible. Even if we add a prometheus
backend in our metrics service, we need to pass the reference of prometheus_client::registry::Registry
to the libp2p network service, so that it can be used as described in this PR. I don't think that's a good design. Also, it seems that the only way we can query metrics from the Registry
is dumping all metrics using the text encoder or others. I think that this way doesn't fit with the design of our metrics service which has update()
and load()
functions.
I'd like to suggest to postpone this discussion since this topic isn't in priority. Sorry for opening this PR without enough research. Later when we really begin using our metrics service, we can think about how we can adopt prometheus-client (or if it's necessary).
metrics
recording feature for libp2pmetrics
recording feature for libp2p
Closing this for now since we don't clearly know which metrics should be collected from network layer in order to measure the quality of privacy. I'll revisit this PR after we develop more dicussions about the network privacy. |
Using libp2p_metrics, we can gather metrics by recording libp2p events. For example, we can record how many messages are received from certain peers. This would be useful for #391 in the future.
TODO: expose libp2p metrics to the metrics service.