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

initial draft of an introspection data model #9

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

raulk
Copy link
Member

@raulk raulk commented Mar 30, 2019

For Project {Phantom}Drift (libp2p network visualizer). Some of these stats can be sourced from OpenCensus, others can't, and many elements are state, not stats.

Right now I'm only modelling the Swarm subsystem. Just getting some initial ideas across.

@vasco-santos – have a look.


This definition does not model the interaction between the tooling and the observed system. Right now it only tackles the data model. The protocol will be interactive and will likely make heavy use of gRPC messages and streams.

@raulk raulk requested a review from vasco-santos March 30, 2019 15:11
@ghost ghost assigned raulk Mar 30, 2019
@ghost ghost added the in progress label Mar 30, 2019
// streams within this connection by inlining.
repeated Stream streams = 13;

reserved 14, 15;
Copy link
Member Author

Choose a reason for hiding this comment

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

Could probably group some fields to spare more 1-byte ids.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we can change the EndpointPair to Endpoint and add transport there. We can also create a new message Traffic to handle the DataGauge traffic_in and traffic_out on both Connection and Stream. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the Traffic idea! About the transport inside Endpoint, it sounds weird.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

This is shaping well raul! Great work!

I provided some feedback that I would like your input

}

// PeerId represents a peer ID.
// TODO: At this point I'm unsure if we'll return the string or the bytes representation.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep the id_bytes for now. However, as we intend to provide the necessary data to the UI with no computations in the libp2p context, id_bytes should not be necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree it should be a string only.

// streams within this connection by inlining.
repeated Stream streams = 13;

reserved 14, 15;
Copy link
Member

Choose a reason for hiding this comment

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

Well, we can change the EndpointPair to Endpoint and add transport there. We can also create a new message Traffic to handle the DataGauge traffic_in and traffic_out on both Connection and Stream. What do you think?

// the status of this connection.
Status status = 3;
// the transport managing this connection.
Transport transport = 4;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be just the transport name? I think the Transport type has a lot of redundant information

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to a transport_id, and added a bytes id to Transport. How does that sound?

// connection listing.
repeated Connection conns = 4;
// streams listing.
repeated Stream streams = 5;
Copy link
Member

Choose a reason for hiding this comment

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

We have the StreamList in each Connection. Shouldn't it be enough?

}

// Host is a top-level object conveying a cross-cutting view of the entire system, including all subsystems.
// TODO: WIP right now we're only covering the Swarm subsystem.
Copy link
Member

Choose a reason for hiding this comment

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

move this to the Subsystems definition

@vasco-santos
Copy link
Member

@raulk FYI I added the DHT subsystem to the protobuf definition


message Bucket {
// bucket peers.
repeated bytes peers = 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

Things missing here:

  • latency of each peer (we track this)
  • average latency of the bucket
  • distance of the bucket
  • peer age in bucket

// latency for provide operations.
repeated uint64 latency_provide_ns = 6;
// counter for provide operations.
ResultCounter operation_provide = 7;
Copy link
Member Author

Choose a reason for hiding this comment

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

typo, field duplicated.

repeated Bucket buckets = 2;
}

message Operations {
Copy link
Member Author

Choose a reason for hiding this comment

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

call this OperationStatistics?

// type of the query.
Type type = 5;
// rpc message of the query.
bytes rpc = 6;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure this is needed.

}

// id of the query.
bytes id = 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

ID of the target of the query, right?

Copy link

Choose a reason for hiding this comment

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

Since the type is bytes and elsewhere peer Ids are string, I suspect this is intended for internal referencing, like connection and stream ids? In which case, we should add a string field like target_peer_id?

(related: should the peer lists in Query and Bucket in the DHT section be repeated string not repeated bytes?)

Copy link
Member Author

Choose a reason for hiding this comment

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

This ID can represent a peer ID or a CID. I'm fine making these strings for now, so that they're presentable on UI immediately. If we make them bytes, the consumer would be burdened with converting to a string prior to presenting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this object can represent three types of queries: peer, provider, key/value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants