Skip to content
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

Add infrastructure for Subspace networking metrics. #2284

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

shamil-gadelshin
Copy link
Contributor

This PR adds the infrastructure code for custom DSN metrics. It follows a discussion from this PR: #2280

The PR exports a single metric (established connections) for demo purposes.

Code contributor checklist:

@shamil-gadelshin shamil-gadelshin added the networking Subspace networking (DSN) label Dec 1, 2023
@shamil-gadelshin shamil-gadelshin self-assigned this Dec 1, 2023
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I fully understand why we need external and internal metrics separation. In fact I'm not sure why those are even exposed as networking types, I would have expected optional Registry to be provided as a construction argument for networking to register whatever metrics it needs just like it is with libp2p itself.

And I'd like to emphasize that none of this is about networking metrics, we need more farmer and other metrics too, so changes should be done with that in mind.

@shamil-gadelshin
Copy link
Contributor Author

AFAIK, the Registry struct we use doesn't support clone() and must be operated using &mut which will leak the lifetime up from the first (lowest) usage of the registry across the library stack which will be very inconvenient. it's easier to pass preconfigured structs here. I would also prefer simplifications and am open to suggestions.

For example, we could invert the dependencies and return Registry along with node and node_runner with downsides starting with "incorrect abstraction level". Another option would be exposing "Registry" as a detachable property for &mut Node variable (node.registry.take()). Both options could resolve your request but I'm not sure they are better solutions.

@nazar-pc
Copy link
Member

AFAIK, the Registry struct we use doesn't support clone() and must be operated using &mut which will leak the lifetime up from the first (lowest) usage of the registry across the library stack which will be very inconvenient. it's easier to pass preconfigured structs here. I would also prefer simplifications and am open to suggestions.

Yes, you send &mut Registry and instantiate whatever structs you need internally for specific metrics, but it is an internal implementation detail. This is how libp2p works, how Substrate works (modulo the fact that they have a different library) and how our code should work as well IMO. In Pulsar/Space Acres we will instantiate registry at the very top of the stack and pass it down for all the various components to add theirs specific metrics to it.

For example, we could invert the dependencies and return Registry along with node and node_runner with downsides starting with "incorrect abstraction level". Another option would be exposing "Registry" as a detachable property for &mut Node variable (node.registry.take()). Both options could resolve your request but I'm not sure they are better solutions.

This is backwards if we are talking about generic metrics that networking contributes to rather than networking metrics that others are using by accident.

We can wrap Registry in Arc<Mutex<Registry>>, but I don't think that'll be necessary.

@shamil-gadelshin
Copy link
Contributor Author

Yes, you send &mut Registry and instantiate whatever structs you need internally for specific metrics...

It makes sense. I misunderstood you then.

I think the constructor() could accept &mut Registry and create both metrics types internally. That would be an improvement.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality-wise looks fine, some suggestions for renaming/refactoring.

crates/subspace-networking/src/constructor.rs Outdated Show resolved Hide resolved
crates/subspace-networking/src/constructor.rs Outdated Show resolved Hide resolved
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll squash-merge once CI passes

@nazar-pc nazar-pc merged commit 5be3c33 into main Dec 14, 2023
10 checks passed
@nazar-pc nazar-pc deleted the new-networking-metrics branch December 14, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking Subspace networking (DSN)
Projects
Development

Successfully merging this pull request may close these issues.

2 participants